fix(menu-button): close menus containing selected items when re-click…

…ing the menu button (#1193)

* fix(menu-button): don't re-open <Menu> with selected items when clicking the original <MenuButton>

* fix(menu-button): prevent mouse propagation on menu button whilst menu is open

cypress/e2e/ui/menuButton.cy.ts

@@ -77,4 +77,23 @@ context('Components/MenuButton', () => {
     cy.get('#menu-item-2').focus()
     cy.get('#menu-button[aria-expanded="true"]').should('exist')
   })
+
+  it('clicking should open/close menu (with selected items)', () => {
+    cy.visit('/frame/?path=/components/menu/selected-item')
+
+    // click button
+    cy.get('#menu-button').click()
+    cy.get('#menu-button[aria-expanded="true"]').should('exist')
+
+    // click the same button again
+    cy.get('#menu-button').click()
+    cy.get('#menu-button[aria-expanded="false"]').should('exist')
+  })
+
+  it('should show the selected menu item when opened', () => {
+    cy.visit('/frame/?path=/components/menu/selected-item')
+
+    cy.get('#menu-button').click()
+    cy.get('#menu-item-2').should('be.focused')
+  })
 })

src/core/components/menu/__workshop__/selectedItem.tsx

@@ -16,8 +16,10 @@ const POPOVER_PROPS: MenuButtonProps['popover'] = {
   matchReferenceWidth: true,
 }
 
+const INITIAL_INDEX = 1
+
 export default function SelectedItemStory() {
-  const [selectedIndex, setSelectedIndex] = useState(0)
+  const [selectedIndex, setSelectedIndex] = useState(INITIAL_INDEX)
 
   return (
     <Box padding={[4, 5, 6]}>
@@ -26,12 +28,13 @@ export default function SelectedItemStory() {
 
         <MenuButton
           button={<Button text="Open menu" />}
-          id="selected-item-example"
+          id="menu-button"
           menu={
             <Menu>
               <MenuItem
                 icon={SearchIcon}
                 iconRight={selectedIndex === 0 ? CheckmarkIcon : undefined}
+                id="menu-item-1"
                 onClick={() => setSelectedIndex(0)}
                 pressed={selectedIndex === 0}
                 selected={selectedIndex === 0}
@@ -40,6 +43,7 @@ export default function SelectedItemStory() {
               <MenuItem
                 icon={ClockIcon}
                 iconRight={selectedIndex === 1 ? CheckmarkIcon : undefined}
+                id="menu-item-2"
                 onClick={() => setSelectedIndex(1)}
                 pressed={selectedIndex === 1}
                 selected={selectedIndex === 1}
@@ -49,6 +53,7 @@ export default function SelectedItemStory() {
               <MenuItem
                 icon={ExpandIcon}
                 iconRight={selectedIndex === 2 ? CheckmarkIcon : undefined}
+                id="menu-item-3"
                 onClick={() => setSelectedIndex(2)}
                 pressed={selectedIndex === 2}
                 selected={selectedIndex === 2}

src/core/components/menu/menuButton.tsx

@@ -99,6 +99,16 @@ export const MenuButton = forwardRef(function MenuButton(
     setShouldFocus(null)
   }, [])
 
+  // Prevent mouse event propagation when the menu is open.
+  // This is to ensure that `handleBlur` isn't triggered when clicking the menu button whilst open,
+  // which can lead to `setOpen` being triggered multiple times (once by `handleBlur`, and again by `handleButtonClick`).
+  const handleMouseDown = useCallback(
+    (event: PointerEvent) => {
+      if (open) event.preventDefault()
+    },
+    [open],
+  )
+
   const handleButtonKeyDown = useCallback((event: React.KeyboardEvent<HTMLButtonElement>) => {
     // On `ArrowDown`, `Enter` and `Space`
     // - Opens menu and moves focus to first menuitem
@@ -231,13 +241,14 @@ export const MenuButton = forwardRef(function MenuButton(
             id,
             onClick: handleButtonClick,
             onKeyDown: handleButtonKeyDown,
+            onMouseDown: handleMouseDown,
             'aria-haspopup': true,
             'aria-expanded': open,
             ref: setButtonRef,
             selected: open,
           })
         : null,
-    [buttonProp, handleButtonClick, handleButtonKeyDown, id, open, setButtonRef],
+    [buttonProp, handleButtonClick, handleButtonKeyDown, handleMouseDown, id, open, setButtonRef],
   )
 
   const popoverProps: MenuButtonProps['popover'] = useMemo(

stories/components/MenuButton.stories.tsx

@@ -1,11 +1,13 @@
 import {ClockIcon, CommentIcon, ExpandIcon, SearchIcon} from '@sanity/icons'
+import {expect} from '@storybook/jest'
 import type {Meta, StoryObj} from '@storybook/react'
+import {userEvent, within} from '@storybook/testing-library'
 import {Menu, MenuButton, MenuDivider, MenuGroup, MenuItem} from '../../src/core/components'
 import {Button, Flex} from '../../src/core/primitives'
 
 const meta: Meta<typeof MenuButton> = {
   args: {
-    button: <Button tone="primary" text="Open" />,
+    button: <Button text="Open" />,
     menu: (
       <Menu>
         <MenuItem icon={SearchIcon} id="menu-item-1" text="Search" />
@@ -69,3 +71,33 @@ export const WithMenuGroup: Story = {
     return <MenuButton {...props} />
   },
 }
+
+export const WithSelectedItem: Story = {
+  args: {
+    menu: (
+      <Menu data-testid="menu">
+        <MenuItem id="menu-item-1" selected text="Search" />
+        <MenuItem id="menu-item-2" text="Clock" />
+        <MenuDivider />
+        <MenuItem id="menu-item-3" text="Comment" />
+        <MenuItem id="menu-item-4" text="Expand" />
+      </Menu>
+    ),
+  },
+  render: (props) => {
+    return <MenuButton {...props} />
+  },
+  play: async ({canvasElement}) => {
+    const canvas = within(canvasElement)
+
+    const button = canvas.getByRole('button', {name: 'Open'})
+
+    await userEvent.click(button)
+    await userEvent.click(button)
+
+    const menu = within(document.documentElement).queryByTestId('menu')
+
+    // Assertion: <Menu> with a selected item should not be visible when clicking the original <MenuButton> to close
+    expect(menu).toBeNull()
+  },
+}