-
Notifications
You must be signed in to change notification settings - Fork 97
fix(dropdowns): ensure focus is returned to menu trigger before calling onChange
#1930
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
Conversation
| const trigger = getByRole('button'); | ||
|
|
||
| // open menu with onClick | ||
| await user.click(trigger); | ||
| const item = getByTestId('item-02'); | ||
| // focus remains on trigger with mouseEvents | ||
| expect(trigger).toHaveFocus(); | ||
|
|
||
| // close menu | ||
| await user.click(trigger); | ||
| expect(item).not.toBeVisible(); | ||
|
|
||
| // open menu with keyboard | ||
| trigger.focus(); | ||
| await user.keyboard(' '); | ||
| // focus is on the focused item associated with `defaultFocusedValue` |
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.
zendeskgarden/react-containers#660 changed the focus behavior when using defaultFocusedValue.
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 Menu API (and Storybook controls) are missing the new restoreFocus prop. This needs to be specified under /types.
packages/dropdowns/package.json
Outdated
| "@floating-ui/react-dom": "^2.0.0", | ||
| "@zendeskgarden/container-combobox": "^2.0.0", | ||
| "@zendeskgarden/container-menu": "^0.4.0", | ||
| "@zendeskgarden/container-menu": "0.5.0", |
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.
| "@zendeskgarden/container-menu": "0.5.0", | |
| "@zendeskgarden/container-menu": "^0.5.0", |
These dependencies use range notation
docs/migration.md
Outdated
| after menu interaction. To keep the dropdown open on selection, | ||
| set `restoreFocus={false}` and manage focus manually. |
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.
Not sure I understand this. Wouldn't the isExpanded controlled prop be the mechanism for keeping the menu open?
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.
I was referring to cases when a users chooses to keep the menu open after selecting an item within an ItemGroup of type checkbox. This demo showcases this pattern.
It probably makes more sense to rephrase it as such:
`Menu`: new `restoreFocus` prop (default: `true`) returns focus to trigger
after menu interaction. If keeping the dropdown open on selection is required,
set `restoreFocus={false}` and manage focus manually.
What do you think?
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 still makes it sound like there is a causal relationship between keeping the menu open and setting restoreFocus to false.
docs/migration.md
Outdated
| - `Menu`: value `auto` is no longer valid for the `fallbackPlacements` prop. | ||
| - `Menu`: new `restoreFocus` prop (default: `true`) returns focus to trigger | ||
| after menu interaction. To keep the dropdown open on selection, | ||
| set `restoreFocus={false}` and manage focus manually. |
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.
| - `Menu`: value `auto` is no longer valid for the `fallbackPlacements` prop. | |
| - `Menu`: new `restoreFocus` prop (default: `true`) returns focus to trigger | |
| after menu interaction. To keep the dropdown open on selection, | |
| set `restoreFocus={false}` and manage focus manually. | |
| - `Menu` | |
| - value `auto` is no longer valid for the `fallbackPlacements` prop. | |
| - new `restoreFocus` prop (default: `true`) returns focus to trigger | |
| after menu interaction. When menu expansion is controlled to allow | |
| multiple item selection, set `restoreFocus={false}` and manage trigger | |
| focus manually on close. |
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.
Couldn't have written it better myself. Thank you! 💯
Co-authored-by: Jonathan Zempel <[email protected]>
42126b1 to
4f0549c
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.
🚢
Description
This PR bumps
@zendeskgarden/container-menutov0.5.0with two key changes:Menunow returns focus to the trigger before callingonChange, improving A11y for implementations that open modals after selection (see chore: focus return demo - ⚠️ DO NOT MERGE! ⚠️ #1931)defaultFocusedValueonly applies when opening the menu via keyboard, aligning with APG examplesThese changes improve accessibility and user experience, especially for keyboard navigation and complex UI interactions.
💥 BREAKING CHANGE:
Menu: newrestoreFocusprop (default:true) returns focus to trigger after menu interaction. When menu expansion is controlled to allow multiple item selection, setrestoreFocus={false}and manage trigger focus manually on close.Detail
@zendeskgarden/container-menutov0.5.0defaultFocusedValueChecklist
👌 design updates will be Garden Designer approved (add the designer as a reviewer)npm start)⬅️ renders as expected with reversed (RTL) direction⚫ renders as expected in dark mode🤘 renders as expected with Bedrock CSS (?bedrock)