From c07b30c5b0b995e3457f070e5febb85a6b8334e6 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Thu, 31 Aug 2023 11:53:33 -0400 Subject: [PATCH] MM-54191 Change how menu item click handlers are triggered after menus fully close (#24335) * MM-54191 Change how menu item click handlers are triggered after menus fully close * Switch menu items to use onClick instead of onMouseDown * Add unit tests for menu keyboard navigation and opening modals * Fix missing use of useMenuContextValue * Remove unneeded ?. * Fix unrelated test broken by @testing-library/user-event update --- webapp/channels/package.json | 2 +- .../src/components/menu/menu.test.tsx | 258 ++++++++++++++++++ webapp/channels/src/components/menu/menu.tsx | 13 +- .../src/components/menu/menu_context.ts | 23 +- .../src/components/menu/menu_item.tsx | 47 +--- .../channels/src/components/menu/sub_menu.tsx | 9 +- .../new_channel_modal.test.tsx | 1 - webapp/package-lock.json | 20 +- 8 files changed, 305 insertions(+), 68 deletions(-) create mode 100644 webapp/channels/src/components/menu/menu.test.tsx diff --git a/webapp/channels/package.json b/webapp/channels/package.json index 6e372e2235..fcac8306da 100644 --- a/webapp/channels/package.json +++ b/webapp/channels/package.json @@ -106,7 +106,7 @@ "@redux-devtools/extension": "3.2.3", "@testing-library/jest-dom": "5.16.4", "@testing-library/react": "12.1.4", - "@testing-library/user-event": "12.1.4", + "@testing-library/user-event": "13.5.0", "@types/bootstrap": "4.5.0", "@types/country-list": "2.1.0", "@types/enzyme": "3.10.11", diff --git a/webapp/channels/src/components/menu/menu.test.tsx b/webapp/channels/src/components/menu/menu.test.tsx new file mode 100644 index 0000000000..b628e957f7 --- /dev/null +++ b/webapp/channels/src/components/menu/menu.test.tsx @@ -0,0 +1,258 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React, {useState} from 'react'; + +import {DotsVerticalIcon} from '@mattermost/compass-icons/components'; +import {GenericModal} from '@mattermost/components'; + +import { + renderWithFullContext, + screen, + userEvent, + waitForElementToBeRemoved, +} from 'tests/react_testing_utils'; + +import {Menu} from './menu'; +import {MenuItem} from './menu_item'; +import {SubMenu} from './sub_menu'; + +describe('menu click handlers', () => { + test('should be able to open a React Bootstrap modal with the mouse', async () => { + renderWithFullContext( + , + ); + + expect(screen.queryByText('Open modal')).not.toBeInTheDocument(); + expect(screen.queryByText('A Modal')).not.toBeInTheDocument(); + + // Click to open the menu + userEvent.click(screen.getByLabelText('menu with modal button')); + + expect(screen.getByText('Open modal')).toBeInTheDocument(); + + // Click to open the modal + userEvent.click(screen.getByText('Open modal')); + + // Wait for the menu to close before the modal will be opened + await waitForElementToBeRemoved(() => screen.queryByText('Open modal')); + + expect(screen.getByText('A Modal')).toBeInTheDocument(); + }); + + for (const enterOrSpace of ['enter', 'space']) { + test(`should be able to open a React Bootstrap modal with the keyboard using the ${enterOrSpace} key`, async () => { + renderWithFullContext( + , + ); + + expect(screen.queryByText('Open modal')).not.toBeInTheDocument(); + expect(screen.queryByText('A Modal')).not.toBeInTheDocument(); + + expect(document.body).toHaveFocus(); + + // Tab to select the menu button + userEvent.tab(); + + expect(screen.getByLabelText('menu with modal button')).toHaveFocus(); + + // Press the key to open the menu + userEvent.keyboard('{' + enterOrSpace + '}'); + + expect(screen.getByText('Open modal')).toBeInTheDocument(); + + // Press the down arrow twice to select the menu item we want + userEvent.keyboard('{arrowdown}{arrowdown}'); + + expect(screen.getByText('Open modal').closest('li')).toHaveFocus(); + + // Press the key to open the modal + userEvent.keyboard('{' + enterOrSpace + '}'); + + // Wait for the menu to close before the modal will be opened + await waitForElementToBeRemoved(() => screen.queryByText('Open modal')); + + expect(screen.getByText('A Modal')).toBeInTheDocument(); + }); + } + + test('should be able to open a React Bootstrap modal from a submenu with the mouse', async () => { + renderWithFullContext( + , + ); + + expect(screen.queryByText('Open submenu')).not.toBeInTheDocument(); + expect(screen.queryByText('Open modal')).not.toBeInTheDocument(); + expect(screen.queryByText('A Modal')).not.toBeInTheDocument(); + + // Click to open the menu + userEvent.click(screen.getByLabelText('menu with modal button')); + + expect(screen.getByText('Open submenu')).toBeInTheDocument(); + expect(screen.queryByText('Open model from submenu')).not.toBeInTheDocument(); + + // Hover to open the submenu + userEvent.hover(screen.getByText('Open submenu')); + + expect(screen.getByText('Open modal from submenu')).toBeInTheDocument(); + + // Click to open the modal + userEvent.click(screen.getByText('Open modal from submenu')); + + // Wait for the menu and submenu to close before the modal will be opened + await waitForElementToBeRemoved(() => screen.queryByText('Open modal from submenu')); + await waitForElementToBeRemoved(() => screen.queryByText('Open submenu')); + + expect(screen.getByText('A Modal')).toBeInTheDocument(); + }); + + for (const enterOrSpace of ['enter', 'space']) { + test(`should be able to open a React Bootstrap modal with the keyboard using the ${enterOrSpace} key`, async () => { + renderWithFullContext( + , + ); + + expect(screen.queryByText('Open submenu')).not.toBeInTheDocument(); + expect(screen.queryByText('Open modal')).not.toBeInTheDocument(); + expect(screen.queryByText('A Modal')).not.toBeInTheDocument(); + + expect(document.body).toHaveFocus(); + + // Tab to select the menu button + userEvent.tab(); + + expect(screen.getByLabelText('menu with modal button')).toHaveFocus(); + + // Press the key to open the menu + userEvent.keyboard('{' + enterOrSpace + '}'); + + expect(screen.getByText('Open submenu')).toBeInTheDocument(); + expect(screen.queryByText('Open model from submenu')).not.toBeInTheDocument(); + + // Press the down arrow to select the submenu item + userEvent.keyboard('{arrowdown}'); + + expect(screen.getByText('Open submenu').closest('li')).toHaveFocus(); + + // Press the right arrow to open the submenu + userEvent.keyboard('{arrowright}'); + + expect(screen.getByText('Open modal from submenu')).toBeInTheDocument(); + + // Press the down arrow once to focus first submenu item and then twice more to select the one we want + userEvent.keyboard('{arrowdown}{arrowdown}{arrowdown}'); + + expect(screen.getByText('Open modal from submenu').closest('li')).toHaveFocus(); + + // Press the key to open the modal + userEvent.keyboard('{' + enterOrSpace + '}'); + + // Wait for the menu and submenu to close before the modal will be opened + await waitForElementToBeRemoved(() => screen.queryByText('Open submenu')); + + expect(screen.getByText('A Modal')).toBeInTheDocument(); + }); + } +}); + +function MenuWithModal() { + const [showModal, setShowModal] = useState(false); + + let modal; + if (showModal) { + modal = ( + setShowModal(false)} + > + {'The contents of A Modal'} + + ); + } + + return ( + <> + , + }} + > + + + {'Open modal'}} + onClick={() => setShowModal(true)} + /> + + {modal} + + ); +} + +function MenuWithSubMenuModal() { + const [showModal, setShowModal] = useState(false); + + let modal; + if (showModal) { + modal = ( + setShowModal(false)} + > + {'The contents of A Modal'} + + ); + } + + return ( + <> + , + }} + > + + {'Open submenu'}} + menuId='Menu-SubMenu-Menu' + > + + + {'Open modal from submenu'}} + onClick={() => setShowModal(true)} + /> + + + + {modal} + + ); +} + +function OtherMenuItem(props: any) { + return ( + {'Some menu item'}} + onClick={() => { + throw new Error("don't click me"); + }} + /> + ); +} diff --git a/webapp/channels/src/components/menu/menu.tsx b/webapp/channels/src/components/menu/menu.tsx index 06c6ac2c79..6b8b8642ba 100644 --- a/webapp/channels/src/components/menu/menu.tsx +++ b/webapp/channels/src/components/menu/menu.tsx @@ -8,7 +8,6 @@ import React, { useEffect, KeyboardEvent, SyntheticEvent, - useMemo, useCallback, } from 'react'; import {useDispatch, useSelector} from 'react-redux'; @@ -29,11 +28,11 @@ import OverlayTrigger from 'components/overlay_trigger'; import {GenericModal} from '@mattermost/components'; import {MuiMenuStyled} from './menu_styled'; -import {MenuContext} from './menu_context'; +import {MenuContext, useMenuContextValue} from './menu_context'; const OVERLAY_TIME_DELAY = 500; const MENU_OPEN_ANIMATION_DURATION = 150; -export const MENU_CLOSE_ANIMATION_DURATION = 100; +const MENU_CLOSE_ANIMATION_DURATION = 100; type MenuButtonProps = { id: string; @@ -211,12 +210,7 @@ export function Menu(props: Props) { } }, [isMenuOpen]); - const providerValue = useMemo(() => { - return { - close: closeMenu, - isOpen: Boolean(anchorElement), - }; - }, [anchorElement, closeMenu]); + const providerValue = useMenuContextValue(closeMenu, Boolean(anchorElement)); if (isMobileView) { // In mobile view, the menu is rendered as a modal @@ -232,6 +226,7 @@ export function Menu(props: Props) { open={isMenuOpen} onClose={handleMenuClose} onClick={handleMenuClick} + onTransitionExited={providerValue.handleClosed} onKeyDown={handleMenuKeyDown} className={A11yClassNames.POPUP} width={props.menu.width} diff --git a/webapp/channels/src/components/menu/menu_context.ts b/webapp/channels/src/components/menu/menu_context.ts index f4d5798976..33002b7b23 100644 --- a/webapp/channels/src/components/menu/menu_context.ts +++ b/webapp/channels/src/components/menu/menu_context.ts @@ -1,20 +1,41 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {createContext} from 'react'; +import {createContext, useMemo, useRef} from 'react'; interface MenuSubmenuContextType { close?: () => void; isOpen: boolean; + addOnClosedListener: (listener: () => void) => void; + handleClosed: () => void; } export const MenuContext = createContext({ isOpen: false, + addOnClosedListener: () => {}, + handleClosed: () => {}, }); MenuContext.displayName = 'MenuContext'; export const SubMenuContext = createContext({ isOpen: false, + addOnClosedListener: () => {}, + handleClosed: () => {}, }); SubMenuContext.displayName = 'SubMenuContext'; +export function useMenuContextValue(close: () => void, isOpen: boolean): MenuSubmenuContextType { + const onClosedListeners = useRef(new Set<() => void>()); + + return useMemo(() => ({ + close, + isOpen, + addOnClosedListener: (listener: () => void) => { + onClosedListeners.current.add(listener); + }, + handleClosed: () => { + onClosedListeners.current.forEach((listener) => listener()); + onClosedListeners.current.clear(); + }, + }), [close, isOpen]); +} diff --git a/webapp/channels/src/components/menu/menu_item.tsx b/webapp/channels/src/components/menu/menu_item.tsx index e966202dde..4273f131d9 100644 --- a/webapp/channels/src/components/menu/menu_item.tsx +++ b/webapp/channels/src/components/menu/menu_item.tsx @@ -8,8 +8,6 @@ import React, { KeyboardEvent, MouseEvent, useContext, - useRef, - useEffect, } from 'react'; import {styled} from '@mui/material/styles'; import {useSelector} from 'react-redux'; @@ -22,11 +20,8 @@ import {getIsMobileView} from 'selectors/views/browser'; import Constants, {EventTypes} from 'utils/constants'; import {isKeyPressed} from 'utils/keyboard'; -import {MENU_CLOSE_ANIMATION_DURATION} from './menu'; import {MenuContext, SubMenuContext} from './menu_context'; -const DELAY_CLICK_EVENT_EXECUTION_MODIFIER = 1.2; - export interface Props extends MuiMenuItemProps { /** @@ -124,8 +119,6 @@ export function MenuItem(props: Props) { const isMobileView = useSelector(getIsMobileView); - const onClickEventRef = useRef | KeyboardEvent>(); - function handleClick(event: MouseEvent | KeyboardEvent) { if (isCorrectKeyPressedOnMenuItem(event)) { // close submenu first if it is open @@ -143,41 +136,17 @@ export function MenuItem(props: Props) { // If the menu is in mobile view, we execute the click event immediately. onClick(event); } else { - // We set the ref of event here, see the `useEffect` hook below for more details. - onClickEventRef.current = cloneDeep(event); + // Clone the event since we delay the click handler until after the menu has closed. + const clonedEvent = cloneDeep(event); + + menuContext.addOnClosedListener(() => { + onClick(clonedEvent); + }); } } } } - // This `useEffect` hook is responsible for executing a click event (`onClick`). - // 1. If MenuItem was part of submenu then both menu and submenu should be closed before executing the click event. - // 2. If MenuItem was part of only Menu then only should be closed before executing the click event. - // After the conditions are met the delay is introduced to allow the menu to animate out properly before executing the click event. - // This delay also improves percieved UX as it gives the user a chance to see the menu close before the click event is executed. (eg in case of opening a modal) - useEffect(() => { - let shouldExecuteClick = false; - - if (subMenuContext.close) { - // This means that the menu item is a submenu item and both menu and submenu are closed. - shouldExecuteClick = subMenuContext.isOpen === false && menuContext.isOpen === false && Boolean(onClickEventRef.current); - } else { - shouldExecuteClick = menuContext.isOpen === false && Boolean(onClickEventRef.current); - } - - if (shouldExecuteClick) { - const delayExecutionTimeout = MENU_CLOSE_ANIMATION_DURATION * DELAY_CLICK_EVENT_EXECUTION_MODIFIER; - - setTimeout(() => { - if (onClick && onClickEventRef.current) { - onClick(onClickEventRef.current); - } - - onClickEventRef.current = undefined; - }, delayExecutionTimeout); - } - }, [menuContext.isOpen, subMenuContext.isOpen, subMenuContext.close, onClick]); - // When both primary and secondary labels are passed, we need to apply minor changes to the styling. Check below in styled component for more details. const hasSecondaryLabel = labels && labels.props && labels.props.children && Children.count(labels.props.children) === 2; @@ -188,8 +157,8 @@ export function MenuItem(props: Props) { isDestructive={isDestructive} hasSecondaryLabel={hasSecondaryLabel} isLabelsRowLayout={isLabelsRowLayout} + onClick={handleClick} onKeyDown={handleClick} - onMouseDown={handleClick} {...otherProps} > {leadingElement &&
{leadingElement}
} @@ -326,7 +295,7 @@ function isCorrectKeyPressedOnMenuItem(event: MouseEvent | Keyboa } return false; - } else if (event.type === EventTypes.MOUSE_DOWN) { + } else if (event.type === EventTypes.CLICK) { const mouseEvent = event as MouseEvent; if (mouseEvent.button === 0) { return true; diff --git a/webapp/channels/src/components/menu/sub_menu.tsx b/webapp/channels/src/components/menu/sub_menu.tsx index 38415663e4..20c35f864d 100644 --- a/webapp/channels/src/components/menu/sub_menu.tsx +++ b/webapp/channels/src/components/menu/sub_menu.tsx @@ -29,7 +29,7 @@ import {GenericModal} from '@mattermost/components'; import {MuiMenuStyled} from './menu_styled'; import {MenuItem, Props as MenuItemProps} from './menu_item'; -import {SubMenuContext} from './menu_context'; +import {SubMenuContext, useMenuContextValue} from './menu_context'; import './sub_menu.scss'; @@ -85,12 +85,7 @@ export function SubMenu(props: Props) { setAnchorElement(null); }, []); - const providerValue = useMemo(() => { - return { - close: closeSubMenu, - isOpen: Boolean(anchorElement), - }; - }, [anchorElement, closeSubMenu]); + const providerValue = useMenuContextValue(closeSubMenu, Boolean(anchorElement)); const hasSubmenuItems = Boolean(children); if (!hasSubmenuItems) { diff --git a/webapp/channels/src/components/new_channel_modal/new_channel_modal.test.tsx b/webapp/channels/src/components/new_channel_modal/new_channel_modal.test.tsx index 280c7bfdd1..a06837f6e3 100644 --- a/webapp/channels/src/components/new_channel_modal/new_channel_modal.test.tsx +++ b/webapp/channels/src/components/new_channel_modal/new_channel_modal.test.tsx @@ -328,7 +328,6 @@ describe('components/new_channel_modal', () => { // Change display name to invalid userEvent.clear(channelNameInput); - userEvent.type(channelNameInput, ''); // Confirm button should be disabled expect(createChannelButton).toBeDisabled(); diff --git a/webapp/package-lock.json b/webapp/package-lock.json index 55d4850dfa..f0c74a98da 100644 --- a/webapp/package-lock.json +++ b/webapp/package-lock.json @@ -152,7 +152,7 @@ "@redux-devtools/extension": "3.2.3", "@testing-library/jest-dom": "5.16.4", "@testing-library/react": "12.1.4", - "@testing-library/user-event": "12.1.4", + "@testing-library/user-event": "13.5.0", "@types/bootstrap": "4.5.0", "@types/country-list": "2.1.0", "@types/enzyme": "3.10.11", @@ -272,12 +272,12 @@ "integrity": "sha512-xWGDIW6x921xtzPkhiULtthJHoJvBbF3q26fzloPCK0hsvxtPVelvftw3zjbHWSkR2km9Z+4uxbDDK/6Zw9B8w==" }, "channels/node_modules/@testing-library/user-event": { - "version": "12.1.4", - "resolved": "https://registry.npmjs.org/@testing-library/user-event/-/user-event-12.1.4.tgz", - "integrity": "sha512-vd5s43lNfyq/JEr8ndQmS+An6dEVUmjW9zqtpmMHU+rrPMaHLrUIlWZ9wDE8ALS/dFMsu6U00A5X/7Dv/2tWnw==", + "version": "13.5.0", + "resolved": "https://registry.npmjs.org/@testing-library/user-event/-/user-event-13.5.0.tgz", + "integrity": "sha512-5Kwtbo3Y/NowpkbRuSepbyMFkZmHgD+vPzYB/RJ4oxt5Gj/avFFBYjhw27cqSVPVw/3a67NK1PbiIr9k4Gwmdg==", "dev": true, "dependencies": { - "@babel/runtime": "^7.10.2" + "@babel/runtime": "^7.12.5" }, "engines": { "node": ">=10", @@ -35656,7 +35656,7 @@ "@stripe/stripe-js": "1.41.0", "@testing-library/jest-dom": "5.16.4", "@testing-library/react": "12.1.4", - "@testing-library/user-event": "12.1.4", + "@testing-library/user-event": "13.5.0", "@tippyjs/react": "4.2.6", "@types/bootstrap": "4.5.0", "@types/color-hash": "1.0.2", @@ -35840,12 +35840,12 @@ } }, "@testing-library/user-event": { - "version": "12.1.4", - "resolved": "https://registry.npmjs.org/@testing-library/user-event/-/user-event-12.1.4.tgz", - "integrity": "sha512-vd5s43lNfyq/JEr8ndQmS+An6dEVUmjW9zqtpmMHU+rrPMaHLrUIlWZ9wDE8ALS/dFMsu6U00A5X/7Dv/2tWnw==", + "version": "13.5.0", + "resolved": "https://registry.npmjs.org/@testing-library/user-event/-/user-event-13.5.0.tgz", + "integrity": "sha512-5Kwtbo3Y/NowpkbRuSepbyMFkZmHgD+vPzYB/RJ4oxt5Gj/avFFBYjhw27cqSVPVw/3a67NK1PbiIr9k4Gwmdg==", "dev": true, "requires": { - "@babel/runtime": "^7.10.2" + "@babel/runtime": "^7.12.5" } }, "@types/react-select": {