fix!(container-menu): improves logic for returning focus to trigger #659
Conversation
There was a problem hiding this comment.
I like this approach. To reiterate what we discussed, we agreed that deferring trigger focus didn't make sense as consumers can't (and shouldn't) control this behavior anyway. It also leaves room for folks to refocus another element in response to menu closing without affecting screen reader announcements.
There was a problem hiding this comment.
Quick status comment:
Flo and I met offline to go over the current approach and how we can maybe improve focus behavior between controlled and uncontrolled instances of isExpanded. Currently, this PR will only synchronously focus the button if isExpanded is uncontrolled.
Instead, we think there could be an opportunity here to give more direct trigger focus control through the use of a new parameter, which would also slightly change how Menu works in controlled scenarios by requiring consumers to focus their buttons when isExpanded is false.
To-be-discussed at tomorrow's meeting of the minds to get team consensus. 🧐
packages/menu/src/useMenu.ts
Outdated
| @@ -272,22 +278,32 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |||
| event.stopPropagation(); | |||
|
|
|||
| const changeType = StateChangeTypes.TriggerClick; | |||
| const nextIsExpanded = !controlledIsExpanded; | |||
There was a problem hiding this comment.
I actually think the code below reads better if we use !controlledIsExpanded. I confused nextisExpanded as referring to the next / nested menu state.
ze-flo
changed the title
| MenuContainer.defaultProps = { | ||
| defaultExpanded: false, | ||
| restoreFocus: true, | ||
| rtl: false |
There was a problem hiding this comment.
[nit] IIRC it would be acceptable to leave defaultExpanded and rtl as undefined since they should be read as falsey anyway.
| @@ -66,6 +66,8 @@ export interface IUseMenuProps<T = HTMLButtonElement, M = HTMLElement> { | |||
| focusedValue?: string | null; | |||
| /** Determines focused value on menu initialization */ | |||
| defaultFocusedValue?: string; | |||
| /** Returns focus to the trigger when the menu is collapsed */ | |||
There was a problem hiding this comment.
[nit] Maybe this can align with our description on the modal container prop:
Returns keyboard focus to the element that triggered the modal
| if (item.href) { | ||
| if (key === KEYS.SPACE) { | ||
| event.preventDefault(); | ||
| event.preventDefault(); |
There was a problem hiding this comment.
Glad this worked wtih anchors. For some reason I thought it wouldn't.
Description
Improve the logic for returning focus to trigger. Adds optional
restoreFocusprop to manager focus return to trigger.💥 BREAKING CHANGE:
A new
restoreFocusprop has been added. By defaultrestoreFocusistrueand will return the focus to the trigger on menu item selection,ESC/TABkey press, and clicking outside the menu dropdown. User who need to keep the dropdown open on selection must setrestoreFocus={false}and manually handle the focus return.Detail
onChangecallback. Now, when a menu item triggers Garden's Modal open, the focus will automatically be restored to the menu trigger after the modal has closed.Screen.Recording.2024-08-29.at.10.19.34.AM.mov
restoreFocusprop which defaults totrue. Disabling it gives users more flexibility with managing focus return to the trigger. See demo Story in docs: demo restoreFocus with controlled menu [ DO NOT MERGE! ] #661.Checklist
npm start)