-
Notifications
You must be signed in to change notification settings - Fork 622
ActionList: Add role="option"
if role="listbox"
is present
#6049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ActionList: Add role="option"
if role="listbox"
is present
#6049
Conversation
🦋 Changeset detectedLatest commit: 7e4a18e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/378331 |
🔴 golden-jobs completed with status |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds automatic role="option"
for ActionList.Item
when its parent ActionList
is a listbox
, and adjusts focus logic and tests accordingly.
- Extends inferred item role logic to detect any
listRole === 'listbox'
. - Simplifies disabled/inactive focusable branch.
- Updates tests and stories to validate and demonstrate the new behavior.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/react/src/ActionList/Item.tsx | Simplify and extend role inference logic for listbox contexts |
packages/react/src/ActionList/Item.test.tsx | Remove hardcoded role="option" and add assertion for inferred option roles |
packages/react/src/ActionList/ActionList.test.tsx | Update disabled item focus expectation in keyboard navigation tests |
packages/react/src/ActionList/ActionList.features.stories.tsx | Rename story, show explicit role="option" (to illustrate) under listbox |
.changeset/soft-webs-study.md | Update changelog with new inference and focusable behavior |
Comments suppressed due to low confidence (2)
packages/react/src/ActionList/ActionList.features.stories.tsx:332
- [nitpick] The story name uses inconsistent casing for 'Listbox'. Consider renaming to
ListBoxSingleSelect
to match the established PascalCase convention (ListBoxMultiSelect
).
export const ListboxSingleSelect = () => {
packages/react/src/ActionList/Item.test.tsx:425
- Consider adding a corresponding test for the multiple
selectionVariant="multiple"
case to ensurerole="option"
is inferred correctly for multi-select listboxes.
it('should add `role="option"` if `role="listbox"` and `selectionVariant` is present', async () => {
@@ -141,8 +141,8 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |||
if (selectionVariant === 'single') inferredItemRole = 'menuitemradio' | |||
else if (selectionVariant === 'multiple') inferredItemRole = 'menuitemcheckbox' | |||
else inferredItemRole = 'menuitem' | |||
} else if (container === 'SelectPanel' && listRole === 'listbox') { | |||
if (selectionVariant !== undefined) inferredItemRole = 'option' | |||
} else if ((container === 'SelectPanel' && listRole === 'listbox') || listRole === 'listbox') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional can be simplified to if (listRole === 'listbox')
, since the extra SelectPanel
check is redundant.
} else if ((container === 'SelectPanel' && listRole === 'listbox') || listRole === 'listbox') { | |
} else if (listRole === 'listbox') { |
Copilot uses AI. Check for mistakes.
aria-checked={selectedIndice === index} | ||
onSelect={() => handleSelect(index)} | ||
disabled={index === 3 ? true : undefined} | ||
role="option" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Since role="option"
is now inferred when ActionList
has role="listbox"
, this explicit prop in the story is redundant and could be removed for clarity.
role="option" | |
Copilot uses AI. Check for mistakes.
Closes https://github.com/github/primer/issues/5083
Adds
role="option"
ifrole="listbox"
is applied toActionList
. This stemmed from some instances ofActionList
utilizingrole="listbox"
without addingrole="option"
. This broke functionality, as usage ofselectionVariant
requires eitheraria-checked
being present, orrole="option"
.Since we can infer what child roles should be based on the parent role, this PR adds
role="option"
ifrole="listbox"
is presentChangelog
New
role="option"
ifrole="listbox"
is present inActionList
Changed
ActionList.Item
(s) to be focused even ifinferredItemRole
is presentRemoved
Rollout strategy
Testing & Reviewing
Merge checklist