Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix!(container-menu): improves logic for returning focus to trigger #659

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+74 −39
Open

fix!(container-menu): improves logic for returning focus to trigger #659

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c57d490
fix(container-menu): improves logic for returning focus to trigger
ze-flo Aug 30, 2024
41bba23
fix: improve focus handling for controlled menu
ze-flo Sep 11, 2024
10f54ca
refactor: PR feedback
ze-flo Sep 11, 2024
ddf93ef
Merge branch 'main' of github.com:zendeskgarden/react-containers into…
ze-flo Sep 13, 2024
d2b60f1
feat(menu): add props.restoreFocus
ze-flo Sep 13, 2024
8f9f0d4
docs: document restoreFocus prop + set default props
ze-flo Sep 13, 2024
d7a27c0
test: add restoreFocus spec
ze-flo Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 57 additions & 39 deletions packages/menu/src/useMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import React, {
useEffect,
geotrev marked this conversation as resolved.
Show resolved Hide resolved
useMemo,
useReducer,
useRef,
useState
} from 'react';
import { useSelection } from '@zendeskgarden/container-selection';
Expand Down Expand Up @@ -94,8 +93,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: focusedValue || defaultFocusedValue,
isExpanded: isExpanded || defaultExpanded,
Expand Down Expand Up @@ -135,6 +132,15 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme

// Internal

const returnFocusToTrigger = useCallback(
(skip?: boolean) => {
if (!skip && triggerRef.current) {
triggerRef.current.focus();
}
},
[triggerRef]
);

const closeMenu = useCallback(
(changeType: string) => {
dispatch({
Expand Down Expand Up @@ -272,22 +278,32 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
event.stopPropagation();

const changeType = StateChangeTypes.TriggerClick;
const nextIsExpanded = !controlledIsExpanded;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think the code below reads better if we use !controlledIsExpanded. I confused nextisExpanded as referring to the next / nested menu state.


dispatch({
type: changeType,
payload: {
...(!isFocusedValueControlled && { focusedValue: null }),
...(!isExpandedControlled && { isExpanded: !controlledIsExpanded })
...(!isExpandedControlled && { isExpanded: nextIsExpanded })
}
});

// Skip focus return when isExpanded === true
returnFocusToTrigger(nextIsExpanded && isExpandedControlled);

onChange({
type: changeType,
focusedValue: null,
isExpanded: !controlledIsExpanded
});
},
[controlledIsExpanded, isFocusedValueControlled, isExpandedControlled, onChange]
[
isFocusedValueControlled,
isExpandedControlled,
controlledIsExpanded,
returnFocusToTrigger,
onChange
]
);

const handleTriggerKeyDown = useCallback(
Expand Down Expand Up @@ -321,14 +337,23 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
}
});

returnFocusToTrigger(isExpandedControlled);

onChange({
type: changeType,
focusedValue: defaultFocusedValue || nextFocusedValue,
isExpanded: true
});
}
},
[isExpandedControlled, isFocusedValueControlled, defaultFocusedValue, onChange, values]
[
values,
isFocusedValueControlled,
defaultFocusedValue,
isExpandedControlled,
returnFocusToTrigger,
onChange
]
);

const handleMenuKeyDown = useCallback(
Expand All @@ -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
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(() => {
Expand Down Expand Up @@ -395,6 +420,8 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
}
});

returnFocusToTrigger(isTransitionItem && isExpandedControlled);

onChange({
type: changeType,
value: item.value,
Expand All @@ -403,10 +430,11 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
});
},
[
getSelectedItems,
state.nestedPathIds,
isExpandedControlled,
isSelectedItemsControlled,
getSelectedItems,
returnFocusToTrigger,
onChange
]
);
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad this worked wtih anchors. For some reason I thought it wouldn't.


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 && isExpandedControlled);
} else if (key === KEYS.RIGHT) {
if (rtl && isPrevious) {
changeType = StateChangeTypes.MenuItemKeyDownPrevious;
Expand Down Expand Up @@ -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
]
);
Expand Down Expand Up @@ -573,6 +595,12 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
setMenuVisible(controlledIsExpanded);
}, [controlledIsExpanded]);

useEffect(() => {
if (isExpandedControlled && isExpanded === false) {
returnFocusToTrigger();
}
}, [isExpandedControlled, isExpanded, returnFocusToTrigger]);

/**
* Respond to clicks outside the open menu
*/
Expand All @@ -594,12 +622,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) {
Expand All @@ -614,13 +638,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,
Expand Down