-
Notifications
You must be signed in to change notification settings - Fork 646
FilteredActionList: Add opt-in props to give more flexibility on focus management with aria-activedescendant
#7240
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ff00f6d 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-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
aria-activedescendant
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 adds two new opt-in props to FilteredActionList (and by extension SelectPanel) to provide more flexibility in managing focus behavior with aria-activedescendant. The changes address accessibility issues and improve control over hover-based selection. The PR also bumps @primer/behaviors to version 1.9.0, which enables these new focus management features.
Key changes:
- Adds
disableSelectOnHoverprop to disable automatic selection on mouse hover - Adds
setInitialFocusprop to control initial focus behavior when the component becomes active - Updates
@primer/behaviorsdependency from 1.8.2 to 1.9.0
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/FilteredActionList/FilteredActionList.tsx | Implements the two new props (disableSelectOnHover and setInitialFocus) with corresponding focus zone configuration options and data attributes for items |
| packages/react/src/SelectPanel/SelectPanel.tsx | Adds disableSelectOnHover to SelectPanelBaseProps and passes it through to FilteredActionList |
| packages/react/src/SelectPanel/SelectPanel.dev.stories.tsx | Adds two new story examples demonstrating the usage of both new props |
| packages/react/package.json | Updates @primer/behaviors dependency to version 1.9.0 |
| package-lock.json | Updates lock file entries for @primer/behaviors version 1.9.0 |
| .changeset/many-planets-take.md | Documents the addition of disableSelectOnHover prop |
| .changeset/chubby-wombats-start.md | Documents the addition of setInitialFocus prop |
Comments suppressed due to low confidence (2)
packages/react/src/FilteredActionList/FilteredActionList.tsx:347
- The data attribute naming is confusing.
data-select-on-hover="true"whendisableSelectOnHoveristruesuggests that selection on hover is enabled, when it's actually disabled. Consider either:
- Renaming to
data-disable-select-on-hoverto match the prop name - Inverting the logic:
data-select-on-hover={!disableSelectOnHover ? 'true' : 'false'} - Or simply:
data-select-on-hover={String(!disableSelectOnHover)}
{...item}
packages/react/src/FilteredActionList/FilteredActionList.tsx:364
- The data attribute naming is confusing.
data-select-on-hover="true"whendisableSelectOnHoveristruesuggests that selection on hover is enabled, when it's actually disabled. Consider either:
- Renaming to
data-disable-select-on-hoverto match the prop name - Inverting the logic:
data-select-on-hover={!disableSelectOnHover ? 'true' : 'false'} - Or simply:
data-select-on-hover={String(!disableSelectOnHover)}
renderItem={listProps.renderItem}
Resolves issues in:
Adds the following:
setInitialFocuswhich allows consumers ofFilteredActionList/SelectPanelto disable the behavior where "focus" is set (viaaria-activedescendant) on the first item when the the component is initially active.disableSelectOnHoverthat allows indirect "selection" on hover to be disabled via activeOnHover.@primer/behaviorsto 1.9.0.Changelog
New
setInitialFocusanddisableSelectOnHoverRollout strategy
Testing & Reviewing
Merge checklist