-
Notifications
You must be signed in to change notification settings - Fork 625
Feat(SelectPanel) remove aria active-descendant pattern in favor of roving tab index under FF #6330
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
Feat(SelectPanel) remove aria active-descendant pattern in favor of roving tab index under FF #6330
Conversation
🦋 Changeset detectedLatest commit: dd82743 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 📦
|
e01ad94
to
5b99a1d
Compare
54538c5
to
847426e
Compare
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
This PR removes the aria-activedescendant
accessibility pattern from SelectPanel and replaces it with a roving tabindex approach to improve accessibility. The changes only affect SelectPanel when the primer_react_select_panel_with_modern_action_list
feature flag is enabled.
Key changes:
- Replace aria-activedescendant focus management with roving tabindex behavior
- Simplify announcement logic since roving tabindex provides automatic focus announcements
- Update keyboard navigation to directly focus list items instead of using virtual focus
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/react/src/SelectPanel/SelectPanel.tsx | Fix logical operator and add item IDs for accessibility |
packages/react/src/SelectPanel/SelectPanel.test.tsx | Update tests to verify roving tabindex behavior and different announcement patterns |
packages/react/src/FilteredActionList/useAnnouncements.tsx | Simplify announcements for item count instead of active descendant tracking |
packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx | Implement roving tabindex with focus management and active indicator styling |
packages/react/src/FilteredActionList/FilteredActionListEntry.tsx | Remove unused onListContainerRefChanged prop from modern implementation |
packages/react/src/FilteredActionList/FilteredActionList.module.css | Add CSS for active item indicator |
packages/react/src/ActionList/List.tsx | Enable focus wrapping for FilteredActionList container |
packages/react/src/ActionList/Item.tsx | Add FilteredActionList to menu context and option role inference |
packages/react/src/ActionList/ActionListContainerContext.tsx | Add radio selection variant type |
packages/react/.changeset/four-cars-attend.md | Document the breaking change for SelectPanel accessibility |
Comments suppressed due to low confidence (1)
packages/react/src/SelectPanel/SelectPanel.test.tsx:300
- This test case for keyboard navigation is incomplete - it only checks that the combobox has focus after clicking, but doesn't actually test arrow key navigation as the name suggests.
it('should support navigating through items with ArrowUp and ArrowDown', async () => {
@@ -350,7 +349,7 @@ function Panel({ | |||
if (open) { | |||
if (items.length === 0 && !(isLoading || loading)) { | |||
// we need to wait for the listContainerElement to disappear before announcing no items, otherwise it will be interrupted | |||
if (!listContainerElement || !usingModernActionList) { | |||
if (!listContainerElement && !usingModernActionList) { |
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.
The logical operator change from ||
to &&
appears to be a bug fix, but this changes the condition semantics significantly. The original condition !listContainerElement || !usingModernActionList
would announce when either condition was true, but the new condition !listContainerElement && !usingModernActionList
only announces when both conditions are true. This could prevent announcements in valid scenarios.
if (!listContainerElement && !usingModernActionList) { | |
if (!listContainerElement || !usingModernActionList) { |
Copilot uses AI. Check for mistakes.
@@ -85,10 +79,12 @@ export function FilteredActionList({ | |||
[onFilterChange, setInternalFilterValue], | |||
) | |||
|
|||
const [enableAnnouncements, setEnableAnnouncements] = useState(false) | |||
const [selectedItems] = useState<(string | number | undefined)[]>([]) |
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.
The selectedItems
state is initialized as an empty array and never updated, making it effectively unused. This variable should either be removed or properly maintained to track selected items.
Copilot uses AI. Check for mistakes.
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.
Visuals look good! have some change requests and questions
packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx
Outdated
Show resolved
Hide resolved
…b.com:primer/react into recreate-pr-5801-remove-aria-activedescendant
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/394030 |
🟢 golden-jobs completed with status |
Closes https://github.com/github/primer/issues/5303
Changelog
Rollout strategy
Testing & Reviewing
Merge checklist