Skip to content

fix(container-menu): improve menu dropdown expansion and focus behavior on blur #691

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

Merged
merged 5 commits into from
Apr 8, 2025
Merged

fix(container-menu): improve menu dropdown expansion and focus behavior on blur #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d90d78
fix(container-menu): improve menu expansion and focus behavior on blur
ze-flo Mar 27, 2025
6bccd75
fix(useMenu): restore natural tab order
ze-flo Apr 2, 2025
b586c41
fix(useMenu): return focus to trigger once on outside menu click
ze-flo Apr 2, 2025
fcb4579
refactor(useMenu): restore previous behavior on TAB keydown
ze-flo Apr 3, 2025
16da5cf
chore: fix typo
ze-flo Apr 8, 2025
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
49 changes: 45 additions & 4 deletions packages/menu/src/MenuContainer.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,16 +241,45 @@ describe('MenuContainer', () => {
expect(menu).not.toBeVisible();
});

it('closes menu on blur', async () => {
it('closes the menu on blur due to a body click, returns focus to the trigger the first time, and focuses the body on following clicks.', async () => {
const { getByTestId } = render(<TestMenu items={ITEMS} />);
const trigger = getByTestId('trigger');
const menu = getByTestId('menu');

await waitFor(async () => {
await user.click(trigger);
});
expect(menu).toBeVisible();

await waitFor(async () => {
await user.click(document.body);
});
expect(trigger).toHaveFocus();
expect(menu).not.toBeVisible();

await waitFor(async () => {
await user.click(document.body);
});

expect(document.body).toHaveFocus();
});

it('closes menu on blur and moves focus to focusable element', async () => {
const { getByTestId } = render(
<>
<TestMenu items={ITEMS} />
<button data-test-id="focusable">Click me</button>
</>
);
const trigger = getByTestId('trigger');
const menu = getByTestId('menu');
const button = getByTestId('focusable');

await waitFor(async () => {
await user.click(trigger);
await user.click(button);
});
expect(button).toHaveFocus();
expect(menu).not.toBeVisible();
});

Expand Down Expand Up @@ -887,21 +916,33 @@ describe('MenuContainer', () => {
expect(fruit1).toHaveAttribute('aria-checked', 'true');
});

it('returns normal keyboard navigation after menu closes', async () => {
it('returns focus to trigger before resuming normal tab navigation after menu closes', async () => {
const { getByText, getByTestId } = render(
<>
<TestMenu items={ITEMS} />
<button>focus me</button>
</>
);
const trigger = getByTestId('trigger');
const menu = getByTestId('menu');
const nextFocusedElement = getByText('focus me');

trigger.focus();

await waitFor(async () => {
await user.keyboard('{Enter}'); // select trigger
await user.keyboard('{Enter}'); // select first item
await user.keyboard('{ArrowDown}');
});

expect(menu).toBeVisible();

await waitFor(async () => {
await user.keyboard('{Tab}');
});

expect(menu).not.toBeVisible();
expect(trigger).toHaveFocus();

await waitFor(async () => {
await user.keyboard('{Tab}');
});

Expand Down
88 changes: 69 additions & 19 deletions packages/menu/src/useMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,16 +375,52 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
[closeMenu, returnFocusToTrigger]
);

const handleMenuBlur = useCallback(
(event: MouseEvent) => {
const path = event.composedPath();
/**
* 1. Determine if the next element receiving focus is focusable
* (event.relatedTarget is null when focus moves to non-focusable elements or body).
*
* 2. When an element loses focus (on blur), and focus moves to a non-focusable element
* like <body>, `event.relatedTarget` should be `null`. However, due to a bug in jsdom
* (prior to version 24.1.2), `relatedTarget` is incorrectly set to the `Document` node
* (`nodeName === '#document'`).
*
* Currently, `jest-environment-jsdom` (v29.7.0) uses jsdom@20.0.3, which still has this issue.
* Until Jest updates its jsdom dependency, this workaround ensures accurate
* testing of focus behavior.
*
* @see https://github.com/jsdom/jsdom/pull/3767
* @see https://github.com/jsdom/jsdom/releases/tag/24.1.2
* @see https://github.com/jestjs/jest/blob/v29.7.0/packages/jest-environment-jsdom/package.json
*
* 3. Skip focus-return to trigger in these scenarios:
* a. Focus is moving to another focusable element
* b. Menu is closed and focus would naturally go to body
*/
const handleBlur = useCallback(
(event: React.FocusEvent) => {
const win = environment || window;

if (!path.includes(menuRef.current!) && !path.includes(triggerRef.current!)) {
returnFocusToTrigger();
closeMenu(StateChangeTypes.MenuBlur);
}
setTimeout(() => {
// Timeout is required to ensure blur is handled after focus
const activeElement = win.document.activeElement;
const isMenuOrTriggerFocused =
menuRef.current?.contains(activeElement) || triggerRef.current?.contains(activeElement);

if (!isMenuOrTriggerFocused) {
const nextElementIsFocusable =
!!event.relatedTarget /* [1] */ &&
event.relatedTarget?.nodeName !== '#document'; /* [2] */
Copy link
Contributor Author

@ze-flo ze-flo Apr 4, 2025

Choose a reason for hiding this comment

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

On our Gatsby site, when a menu dropdown is open and you click outside, focus shifts to a <div> with tabIndex="-1" instead of the trigger. Should I handle that case?

Screenshot 2025-04-03 at 4 53 53 PM

Copy link
Member

@jzempel jzempel Apr 8, 2025

Choose a reason for hiding this comment

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

Hmm, I doubt it, because the updated behavior is so much better than current https://garden.zendesk.com/components/menu – which jumps to the currently expanded menu when you trigger a menu further down the page.


const shouldSkipFocusReturn =
nextElementIsFocusable || (!controlledIsExpanded && !nextElementIsFocusable); /* [3] */

returnFocusToTrigger(shouldSkipFocusReturn);

closeMenu(StateChangeTypes.MenuBlur);
}
});
},
[closeMenu, menuRef, returnFocusToTrigger, triggerRef]
[closeMenu, controlledIsExpanded, environment, menuRef, returnFocusToTrigger, triggerRef]
);

const handleMenuMouseLeave = useCallback(() => {
Expand Down Expand Up @@ -602,18 +638,15 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
const win = environment || window;

if (controlledIsExpanded) {
win.document.addEventListener('click', handleMenuBlur, true);
win.document.addEventListener('keydown', handleMenuKeyDown, true);
} else if (!controlledIsExpanded) {
win.document.removeEventListener('click', handleMenuBlur, true);
win.document.removeEventListener('keydown', handleMenuKeyDown, true);
}

return () => {
win.document.removeEventListener('click', handleMenuBlur, true);
win.document.removeEventListener('keydown', handleMenuKeyDown, true);
};
}, [controlledIsExpanded, handleMenuBlur, handleMenuKeyDown, environment]);
}, [controlledIsExpanded, handleMenuKeyDown, environment]);

/**
* When the menu is opened, this effect sets focus on the current menu item using `focusedValue`
Expand Down Expand Up @@ -690,7 +723,15 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
*/

const getTriggerProps = useCallback<IUseMenuReturnValue['getTriggerProps']>(
({ onClick, onKeyDown, type = 'button', role = 'button', disabled, ...other } = {}) => ({
({
onBlur,
onClick,
onKeyDown,
type = 'button',
role = 'button',
disabled,
...other
} = {}) => ({
...other,
'data-garden-container-id': 'containers.menu.trigger',
'data-garden-container-version': PACKAGE_VERSION,
Expand All @@ -702,14 +743,22 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
tabIndex: disabled ? -1 : 0,
type: type === null ? undefined : type,
role: role === null ? undefined : role,
onKeyDown: composeEventHandlers(onKeyDown, handleTriggerKeyDown),
onClick: composeEventHandlers(onClick, handleTriggerClick)
onBlur: composeEventHandlers(onBlur, handleBlur),
onClick: composeEventHandlers(onClick, handleTriggerClick),
onKeyDown: composeEventHandlers(onKeyDown, handleTriggerKeyDown)
}),
[triggerRef, controlledIsExpanded, handleTriggerClick, handleTriggerKeyDown, triggerId]
[
controlledIsExpanded,
handleBlur,
handleTriggerClick,
handleTriggerKeyDown,
triggerId,
triggerRef
]
);

const getMenuProps = useCallback<IUseMenuReturnValue['getMenuProps']>(
({ role = 'menu', onMouseLeave, ...other } = {}) => ({
({ role = 'menu', onBlur, onMouseLeave, ...other } = {}) => ({
...other,
...getGroupProps({
onMouseLeave: composeEventHandlers(onMouseLeave, handleMenuMouseLeave)
Expand All @@ -719,9 +768,10 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
'aria-labelledby': triggerId,
tabIndex: -1,
role: role === null ? undefined : role,
ref: menuRef as any
ref: menuRef as any,
onBlur: composeEventHandlers(onBlur, handleBlur)
}),
[triggerId, menuRef, getGroupProps, handleMenuMouseLeave]
[getGroupProps, handleBlur, handleMenuMouseLeave, menuRef, triggerId]
);

const getSeparatorProps = useCallback<IUseMenuReturnValue['getSeparatorProps']>(
Expand Down