From 1771cb6c8ea67f33ac68f67722ee52b52597f33d Mon Sep 17 00:00:00 2001 From: Kilian Collender <37899503+Kilian-Collender@users.noreply.github.com> Date: Wed, 15 May 2024 16:25:56 +0100 Subject: [PATCH] fix(menu): include leave intent delay for sub menus (#16466) * 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 --- .all-contributorsrc | 9 ++ README.md | 1 + .../react/src/components/Menu/Menu-test.js | 84 ++++++++++++++++++- packages/react/src/components/Menu/Menu.tsx | 2 +- .../react/src/components/Menu/MenuItem.tsx | 18 +++- 5 files changed, 110 insertions(+), 4 deletions(-) diff --git a/.all-contributorsrc b/.all-contributorsrc index 498dc98e1004..7c2332da4168 100644 --- a/.all-contributorsrc +++ b/.all-contributorsrc @@ -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" diff --git a/README.md b/README.md index 747a79c89137..ee184317aaf8 100644 --- a/README.md +++ b/README.md @@ -290,6 +290,7 @@ check out our [Contributing Guide](/.github/CONTRIBUTING.md) and our
Nikhil Tomar
💻
Anina Antony
💻
Yael Chavoya
💻 +
Kilian Collender
💻 diff --git a/packages/react/src/components/Menu/Menu-test.js b/packages/react/src/components/Menu/Menu-test.js index a9c62c60edd7..43a5630180e9 100644 --- a/packages/react/src/components/Menu/Menu-test.js +++ b/packages/react/src/components/Menu/Menu-test.js @@ -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', () => { @@ -98,6 +98,88 @@ describe('Menu', () => { spy.mockRestore(); }); }); + + describe('Submenu behavior', () => { + beforeEach(() => { + jest.useFakeTimers(); + render( + + + + + + ); + }); + 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', () => { diff --git a/packages/react/src/components/Menu/Menu.tsx b/packages/react/src/components/Menu/Menu.tsx index 4d4dc86fb8f2..1e3d14f80305 100644 --- a/packages/react/src/components/Menu/Menu.tsx +++ b/packages/react/src/components/Menu/Menu.tsx @@ -234,7 +234,7 @@ const Menu = forwardRef(function Menu( function focusItem(e?: React.KeyboardEvent) { const currentItem = focusableItems.findIndex((item) => - item.ref.current.contains(document.activeElement) + item.ref?.current?.contains(document.activeElement) ); let indexToFocus = currentItem; diff --git a/packages/react/src/components/Menu/MenuItem.tsx b/packages/react/src/components/Menu/MenuItem.tsx index 295db5b8a5e1..1492a09f1283 100644 --- a/packages/react/src/components/Menu/MenuItem.tsx +++ b/packages/react/src/components/Menu/MenuItem.tsx @@ -80,6 +80,7 @@ export interface MenuItemProps extends LiHTMLAttributes { } const hoverIntentDelay = 150; // in ms +const leaveIntentDelay = 300; // in ms export const MenuItem = forwardRef( function MenuItem( @@ -113,6 +114,10 @@ export const MenuItem = forwardRef( null ); + const leaveIntentTimeout = useRef | null>( + null + ); + const isDisabled = disabled && !hasChildren; const isDanger = kind === 'danger' && !hasChildren; @@ -169,6 +174,11 @@ export const MenuItem = forwardRef( } 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); @@ -177,8 +187,12 @@ export const MenuItem = forwardRef( 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); } }