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

@@ -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",

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(
+            <MenuWithModal/>,
+        );
+
+        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(
+                <MenuWithModal/>,
+            );
+
+            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(
+            <MenuWithSubMenuModal/>,
+        );
+
+        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(
+                <MenuWithSubMenuModal/>,
+            );
+
+            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 = (
+            <GenericModal
+                show={showModal}
+                confirmButtonText='Confirm button'
+                modalHeaderText='A Modal'
+                onExited={() => setShowModal(false)}
+            >
+                {'The contents of A Modal'}
+            </GenericModal>
+        );
+    }
+
+    return (
+        <>
+            <Menu
+                menu={{
+                    id: 'Menu',
+                }}
+                menuButton={{
+                    id: 'Menu-Button',
+                    'aria-label': 'menu with modal button',
+                    children: <DotsVerticalIcon size={16}/>,
+                }}
+            >
+                <OtherMenuItem/>
+                <OtherMenuItem/>
+                <MenuItem
+                    labels={<span>{'Open modal'}</span>}
+                    onClick={() => setShowModal(true)}
+                />
+            </Menu>
+            {modal}
+        </>
+    );
+}
+
+function MenuWithSubMenuModal() {
+    const [showModal, setShowModal] = useState(false);
+
+    let modal;
+    if (showModal) {
+        modal = (
+            <GenericModal
+                show={showModal}
+                confirmButtonText='Confirm button'
+                modalHeaderText='A Modal'
+                onExited={() => setShowModal(false)}
+            >
+                {'The contents of A Modal'}
+            </GenericModal>
+        );
+    }
+
+    return (
+        <>
+            <Menu
+                menu={{
+                    id: 'Menu',
+                }}
+                menuButton={{
+                    id: 'Menu-Button',
+                    'aria-label': 'menu with modal button',
+                    children: <DotsVerticalIcon size={16}/>,
+                }}
+            >
+                <OtherMenuItem/>
+                <SubMenu
+                    id='Menu-SubMenu'
+                    labels={<>{'Open submenu'}</>}
+                    menuId='Menu-SubMenu-Menu'
+                >
+                    <OtherMenuItem/>
+                    <OtherMenuItem/>
+                    <MenuItem
+                        labels={<span>{'Open modal from submenu'}</span>}
+                        onClick={() => setShowModal(true)}
+                    />
+                    <OtherMenuItem/>
+                </SubMenu>
+            </Menu>
+            {modal}
+        </>
+    );
+}
+
+function OtherMenuItem(props: any) {
+    return (
+        <MenuItem
+            {...props}
+            labels={<>{'Some menu item'}</>}
+            onClick={() => {
+                throw new Error("don't click me");
+            }}
+        />
+    );
+}

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}

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<MenuSubmenuContextType>({
     isOpen: false,
+    addOnClosedListener: () => {},
+    handleClosed: () => {},
 });
 MenuContext.displayName = 'MenuContext';
 
 export const SubMenuContext = createContext<MenuSubmenuContextType>({
     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]);
+}

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<MouseEvent<HTMLLIElement> | KeyboardEvent<HTMLLIElement>>();
-
     function handleClick(event: MouseEvent<HTMLLIElement> | KeyboardEvent<HTMLLIElement>) {
         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 && <div className='leading-element'>{leadingElement}</div>}
@@ -326,7 +295,7 @@ function isCorrectKeyPressedOnMenuItem(event: MouseEvent<HTMLLIElement> | Keyboa
         }
 
         return false;
-    } else if (event.type === EventTypes.MOUSE_DOWN) {
+    } else if (event.type === EventTypes.CLICK) {
         const mouseEvent = event as MouseEvent<HTMLLIElement>;
         if (mouseEvent.button === 0) {
             return true;

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) {

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();

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": {