Skip to content

Commit

Permalink
fix(menu): include leave intent delay for sub menus (#16466)
Browse files Browse the repository at this point in the history
* fix(menu): protect access to child refs when looking for activeElement

* fix(menu): include leave delay to avoid accidental closure of sub menu

The sub menu can accidentally trigger a premature closure of the sub menu
in the following cases:
- the mouse moves across and down as the user selects a lower sub menu,
causing the mouse to leave the menuItem.
- the menu has a scroll bar, as the mouse crosses this to enter the sub menu
 it triggers the closure before the user can access the sub menu.
Including a delay in the closure allows for the users mouse to reenter the
submenu and cancel the closure of the menu.

* chore(contributor): add Kilian-Collender as contributor

* fix(menu): clean up test aria-label

* feat(menu): add aria-label option for menu items

* fix(menu): remove aria-label to make AVT tests happy
  • Loading branch information
Kilian-Collender authored May 15, 2024
1 parent b62c6b6 commit 1771cb6
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 4 deletions.
9 changes: 9 additions & 0 deletions .all-contributorsrc
Original file line number Diff line number Diff line change
Expand Up @@ -1524,6 +1524,15 @@
"contributions": [
"code"
]
},
{
"login": "Kilian-Collender",
"name": "Kilian Collender",
"avatar_url": "https://avatars.githubusercontent.com/u/37899503?v=4",
"profile": "https://github.com/Kilian-Collender",
"contributions": [
"code"
]
}
],
"commitConvention": "none"
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ check out our [Contributing Guide](/.github/CONTRIBUTING.md) and our
<td align="center"><a href="https://github.com/2nikhiltom"><img src="https://avatars.githubusercontent.com/2nikhiltom?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Nikhil Tomar</b></sub></a><br /><a href="https://github.com/carbon-design-system/carbon/commits?author=2nikhiltom" title="Code">💻</a></td>
<td align="center"><a href="https://github.com/aninaantony"><img src="https://avatars.githubusercontent.com/u/164350784?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Anina Antony</b></sub></a><br /><a href="https://github.com/carbon-design-system/carbon/commits?author=aninaantony" title="Code">💻</a></td>
<td align="center"><a href="https://github.com/ychavoya"><img src="https://avatars.githubusercontent.com/u/7907338?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Yael Chavoya</b></sub></a><br /><a href="https://github.com/carbon-design-system/carbon/commits?author=ychavoya" title="Code">💻</a></td>
<td align="center"><a href="https://github.com/Kilian-Collender"><img src="https://avatars.githubusercontent.com/u/37899503?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Kilian Collender</b></sub></a><br /><a href="https://github.com/carbon-design-system/carbon/commits?author=Kilian-Collender" title="Code">💻</a></td>
</tr>
</table>

Expand Down
84 changes: 83 additions & 1 deletion packages/react/src/components/Menu/Menu-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import React from 'react';
import { Menu, MenuItem, MenuItemSelectable, MenuItemRadioGroup } from './';
import { render, screen } from '@testing-library/react';
import { act, fireEvent, render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

describe('Menu', () => {
Expand Down Expand Up @@ -98,6 +98,88 @@ describe('Menu', () => {
spy.mockRestore();
});
});

describe('Submenu behavior', () => {
beforeEach(() => {
jest.useFakeTimers();
render(
<Menu open>
<MenuItem label="Submenu">
<MenuItem label="Item" />
</MenuItem>
</Menu>
);
});
afterEach(() => {
jest.useRealTimers();
});
it('should only show parent and not then submenu when not hovered', () => {
const menus = screen.getAllByRole('menu');
expect(menus.length).toBe(2);
expect(menus[0]).toHaveClass('cds--menu--open');
expect(menus[1]).not.toHaveClass('cds--menu--open');
});

it('should show sub menu when hovered for hoverIntentDelay', async () => {
const menus = screen.getAllByRole('menu');
await act(() =>
fireEvent.mouseEnter(
screen.getByRole('menuitem', { name: 'Submenu Submenu' })
)
);
expect(menus[0]).toHaveClass('cds--menu--open');
expect(menus[1]).not.toHaveClass('cds--menu--open');

await act(() => jest.runOnlyPendingTimers());
expect(menus[0]).toHaveClass('cds--menu--open');
expect(menus[1]).toHaveClass('cds--menu--open');
});

it('should close sub menu on leave after leaveIntentDelay', async () => {
const menus = screen.getAllByRole('menu');
await act(() => {
fireEvent.mouseEnter(
screen.getByRole('menuitem', { name: 'Submenu Submenu' })
);
jest.runOnlyPendingTimers();
});
expect(menus[0]).toHaveClass('cds--menu--open');
expect(menus[1]).toHaveClass('cds--menu--open');

await act(() => {
fireEvent.mouseLeave(
screen.getByRole('menuitem', { name: 'Submenu Submenu' })
);
jest.runOnlyPendingTimers();
});
expect(menus[0]).toHaveClass('cds--menu--open');
expect(menus[1]).not.toHaveClass('cds--menu--open');
});

it('should cancel close sub menu on leave and reenter before leaveIntentDelay', async () => {
const menus = screen.getAllByRole('menu');
await act(() => {
fireEvent.mouseEnter(
screen.getByRole('menuitem', { name: 'Submenu Submenu' })
);
jest.runOnlyPendingTimers();
});
expect(menus[0]).toHaveClass('cds--menu--open');
expect(menus[1]).toHaveClass('cds--menu--open');

await act(() => {
fireEvent.mouseLeave(
screen.getByRole('menuitem', { name: 'Submenu Submenu' })
);
fireEvent.mouseEnter(
screen.getByRole('menuitem', { name: 'Submenu Submenu' })
);
jest.runOnlyPendingTimers();
});
expect(menus[0]).toHaveClass('cds--menu--open');
expect(menus[1]).toHaveClass('cds--menu--open');
});
});
});

describe('MenuItem', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/components/Menu/Menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ const Menu = forwardRef<HTMLUListElement, MenuProps>(function Menu(

function focusItem(e?: React.KeyboardEvent<HTMLUListElement>) {
const currentItem = focusableItems.findIndex((item) =>
item.ref.current.contains(document.activeElement)
item.ref?.current?.contains(document.activeElement)
);
let indexToFocus = currentItem;

Expand Down
18 changes: 16 additions & 2 deletions packages/react/src/components/Menu/MenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export interface MenuItemProps extends LiHTMLAttributes<HTMLLIElement> {
}

const hoverIntentDelay = 150; // in ms
const leaveIntentDelay = 300; // in ms

export const MenuItem = forwardRef<HTMLLIElement, MenuItemProps>(
function MenuItem(
Expand Down Expand Up @@ -113,6 +114,10 @@ export const MenuItem = forwardRef<HTMLLIElement, MenuItemProps>(
null
);

const leaveIntentTimeout = useRef<ReturnType<typeof setTimeout> | null>(
null
);

const isDisabled = disabled && !hasChildren;
const isDanger = kind === 'danger' && !hasChildren;

Expand Down Expand Up @@ -169,6 +174,11 @@ export const MenuItem = forwardRef<HTMLLIElement, MenuItemProps>(
}

function handleMouseEnter() {
if (leaveIntentTimeout.current) {
// When mouse reenters before closing keep sub menu open
clearTimeout(leaveIntentTimeout.current);
leaveIntentTimeout.current = null;
}
hoverIntentTimeout.current = setTimeout(() => {
openSubmenu();
}, hoverIntentDelay);
Expand All @@ -177,8 +187,12 @@ export const MenuItem = forwardRef<HTMLLIElement, MenuItemProps>(
function handleMouseLeave() {
if (hoverIntentTimeout.current) {
clearTimeout(hoverIntentTimeout.current);
closeSubmenu();
menuItem.current?.focus();
// Avoid closing the sub menu as soon as mouse leaves
// prevents accidental closure due to scroll bar
leaveIntentTimeout.current = setTimeout(() => {
closeSubmenu();
menuItem.current?.focus();
}, leaveIntentDelay);
}
}

Expand Down

0 comments on commit 1771cb6

Please sign in to comment.