-
Notifications
You must be signed in to change notification settings - Fork 15
fix(container-menu)!: improves logic for returning focus to trigger #659
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
Changes from all commits
c57d490
41bba23
10f54ca
ddf93ef
d2b60f1
8f9f0d4
d7a27c0
ac1f4b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,6 @@ import React, { | |
useEffect, | ||
useMemo, | ||
useReducer, | ||
useRef, | ||
useState | ||
} from 'react'; | ||
import { useSelection } from '@zendeskgarden/container-selection'; | ||
|
@@ -49,6 +48,7 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
onChange = () => undefined, | ||
isExpanded, | ||
defaultExpanded = false, | ||
restoreFocus = true, | ||
selectedItems, | ||
focusedValue, | ||
defaultFocusedValue | ||
|
@@ -94,8 +94,6 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
*/ | ||
|
||
const [menuVisible, setMenuVisible] = useState<boolean>(false); | ||
const focusTriggerRef = useRef<boolean>(false); | ||
|
||
const [state, dispatch] = useReducer(stateReducer, { | ||
focusedValue, | ||
isExpanded: isExpanded || defaultExpanded, | ||
|
@@ -135,6 +133,15 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
|
||
// Internal | ||
|
||
const returnFocusToTrigger = useCallback( | ||
(skip?: boolean) => { | ||
if (!skip && restoreFocus && triggerRef.current) { | ||
triggerRef.current.focus(); | ||
} | ||
}, | ||
[triggerRef, restoreFocus] | ||
); | ||
|
||
const closeMenu = useCallback( | ||
(changeType: string) => { | ||
dispatch({ | ||
|
@@ -281,13 +288,22 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
} | ||
}); | ||
|
||
// Skip focus return when isExpanded === true | ||
returnFocusToTrigger(!controlledIsExpanded); | ||
|
||
onChange({ | ||
type: changeType, | ||
focusedValue: null, | ||
isExpanded: !controlledIsExpanded | ||
}); | ||
}, | ||
[controlledIsExpanded, isFocusedValueControlled, isExpandedControlled, onChange] | ||
[ | ||
isFocusedValueControlled, | ||
isExpandedControlled, | ||
controlledIsExpanded, | ||
returnFocusToTrigger, | ||
onChange | ||
] | ||
); | ||
|
||
const handleTriggerKeyDown = useCallback( | ||
|
@@ -321,14 +337,23 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
} | ||
}); | ||
|
||
returnFocusToTrigger(); | ||
|
||
onChange({ | ||
type: changeType, | ||
focusedValue: defaultFocusedValue || nextFocusedValue, | ||
isExpanded: true | ||
}); | ||
} | ||
}, | ||
[isExpandedControlled, isFocusedValueControlled, defaultFocusedValue, onChange, values] | ||
[ | ||
values, | ||
isFocusedValueControlled, | ||
defaultFocusedValue, | ||
isExpandedControlled, | ||
returnFocusToTrigger, | ||
onChange | ||
] | ||
); | ||
|
||
const handleMenuKeyDown = useCallback( | ||
|
@@ -341,25 +366,25 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
|
||
const type = StateChangeTypes[key === KEYS.ESCAPE ? 'MenuKeyDownEscape' : 'MenuKeyDownTab']; | ||
|
||
if (KEYS.ESCAPE === key) { | ||
focusTriggerRef.current = true; | ||
} | ||
// TODO: Investigate why focus goes to body instead of next interactive element on TAB keydown. Meanwhile, returning focus to trigger. | ||
geotrev marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are the current results of this investigation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mystery prevails. 🤔 I verified that The browser first sets the |
||
returnFocusToTrigger(); | ||
|
||
closeMenu(type); | ||
} | ||
}, | ||
[closeMenu, focusTriggerRef] | ||
[closeMenu, returnFocusToTrigger] | ||
); | ||
|
||
const handleMenuBlur = useCallback( | ||
(event: MouseEvent) => { | ||
const path = event.composedPath(); | ||
|
||
if (!path.includes(menuRef.current!) && !path.includes(triggerRef.current!)) { | ||
returnFocusToTrigger(); | ||
closeMenu(StateChangeTypes.MenuBlur); | ||
} | ||
}, | ||
[closeMenu, menuRef, triggerRef] | ||
[closeMenu, menuRef, returnFocusToTrigger, triggerRef] | ||
geotrev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
|
||
const handleMenuMouseLeave = useCallback(() => { | ||
|
@@ -395,6 +420,8 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
} | ||
}); | ||
|
||
returnFocusToTrigger(isTransitionItem); | ||
|
||
onChange({ | ||
type: changeType, | ||
value: item.value, | ||
|
@@ -403,10 +430,11 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
}); | ||
}, | ||
[ | ||
getSelectedItems, | ||
state.nestedPathIds, | ||
isExpandedControlled, | ||
isSelectedItemsControlled, | ||
getSelectedItems, | ||
returnFocusToTrigger, | ||
onChange | ||
] | ||
); | ||
|
@@ -448,19 +476,13 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
...(nextSelection && { selectedItems: nextSelection }) | ||
}; | ||
|
||
if (item.href) { | ||
if (key === KEYS.SPACE) { | ||
event.preventDefault(); | ||
geotrev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
event.preventDefault(); | ||
ze-flo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
triggerLink(event.target as HTMLAnchorElement, environment || window); | ||
} | ||
} else { | ||
event.preventDefault(); | ||
if (item.href) { | ||
triggerLink(event.target as HTMLAnchorElement, environment || window); | ||
} | ||
|
||
if (!isTransitionItem) { | ||
focusTriggerRef.current = true; | ||
} | ||
returnFocusToTrigger(isTransitionItem); | ||
} else if (key === KEYS.RIGHT) { | ||
if (rtl && isPrevious) { | ||
changeType = StateChangeTypes.MenuItemKeyDownPrevious; | ||
|
@@ -529,15 +551,15 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
} | ||
}, | ||
[ | ||
rtl, | ||
state.nestedPathIds, | ||
getSelectedItems, | ||
isExpandedControlled, | ||
isFocusedValueControlled, | ||
isSelectedItemsControlled, | ||
focusTriggerRef, | ||
returnFocusToTrigger, | ||
environment, | ||
rtl, | ||
getNextFocusedValue, | ||
getSelectedItems, | ||
isFocusedValueControlled, | ||
state.nestedPathIds, | ||
onChange | ||
] | ||
); | ||
|
@@ -594,12 +616,8 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
}, [controlledIsExpanded, handleMenuBlur, handleMenuKeyDown, environment]); | ||
|
||
/** | ||
* Handles focus depending on menu state: | ||
* - When opened, focus the menu via `focusedValue` | ||
* - When closed, focus the trigger via `triggerRef` | ||
* | ||
* This effect is intended to prevent focusing the trigger or menu | ||
* unless the menu is in the right expansion state. | ||
* When the menu is opened, this effect sets focus on the current menu item using `focusedValue` | ||
* or on the first menu item. | ||
*/ | ||
useEffect(() => { | ||
if (state.focusOnOpen && menuVisible && controlledFocusedValue && controlledIsExpanded) { | ||
|
@@ -614,13 +632,7 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme | |
|
||
ref && ref.focus(); | ||
} | ||
|
||
if (!menuVisible && !controlledIsExpanded && focusTriggerRef.current) { | ||
triggerRef?.current?.focus(); | ||
focusTriggerRef.current = false; | ||
} | ||
}, [ | ||
focusTriggerRef, | ||
values, | ||
menuVisible, | ||
itemRefs, | ||
|
Uh oh!
There was an error while loading. Please reload this page.