fix: outside click behaviour (#19)

* add failing tests to prove the outside-click issue

* fix outside click when we have multiple menu's

- We removed the `toggleMenu` since we only used it in a single spot,
  and had to do some side effect logic (focus & event.preventDefault).
  Wanted to make this consistent between React and Vue.
- If, in the "outside click" logic we detect that we clicked on the
  button, we also ignore it.
- If, we click on the button we will toggle the menu.

Fixes: #18

Author: RobinMalfait

packages/@headlessui-react/src/components/menu/menu.test.tsx

@@ -35,12 +35,24 @@ function getMenuButton(): HTMLElement | null {
   return document.querySelector('[role="button"],button')
 }
 
+function getMenuButtons(): HTMLElement[] {
+  // This is just an assumption for our tests. We assume that we only have 1 button. And if we have
+  // more, than we assume that it is the first one.
+  return Array.from(document.querySelectorAll('[role="button"],button'))
+}
+
 function getMenu(): HTMLElement | null {
   // This is just an assumption for our tests. We assume that our menu has this role and that it is
   // the first item in the DOM.
   return document.querySelector('[role="menu"]')
 }
 
+function getMenus(): HTMLElement[] {
+  // This is just an assumption for our tests. We assume that our menu has this role and that it is
+  // the first item in the DOM.
+  return Array.from(document.querySelectorAll('[role="menu"]'))
+}
+
 function getMenuItems(): HTMLElement[] {
   // This is just an assumption for our tests. We assume that all menu items have this role.
   return Array.from(document.querySelectorAll('[role="menuitem"]'))
@@ -2248,6 +2260,50 @@ describe('Mouse interactions', () => {
     })
   )
 
+  it(
+    'should be possible to click outside of the menu on another menu button which should close the current menu and open the new menu',
+    suppressConsoleLogs(async () => {
+      render(
+        <div>
+          <Menu>
+            <Menu.Button>Trigger</Menu.Button>
+            <Menu.Items>
+              <Menu.Item as="a">alice</Menu.Item>
+              <Menu.Item as="a">bob</Menu.Item>
+              <Menu.Item as="a">charlie</Menu.Item>
+            </Menu.Items>
+          </Menu>
+
+          <Menu>
+            <Menu.Button>Trigger</Menu.Button>
+            <Menu.Items>
+              <Menu.Item as="a">alice</Menu.Item>
+              <Menu.Item as="a">bob</Menu.Item>
+              <Menu.Item as="a">charlie</Menu.Item>
+            </Menu.Items>
+          </Menu>
+        </div>
+      )
+
+      const [button1, button2] = getMenuButtons()
+
+      // Click the first menu button
+      await click(button1)
+      expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible
+
+      // Ensure the open menu is linked to the first button
+      assertMenuButtonLinkedWithMenu(button1, getMenu())
+
+      // Click the second menu button
+      await click(button2)
+
+      expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible
+
+      // Ensure the open menu is linked to the second button
+      assertMenuButtonLinkedWithMenu(button2, getMenu())
+    })
+  )
+
   it(
     'should be possible to hover an item and make it active',
     suppressConsoleLogs(async () => {

packages/@headlessui-react/src/components/menu/menu.tsx

@@ -47,7 +47,6 @@ type StateDefinition = {
 }
 
 enum ActionTypes {
-  ToggleMenu,
   OpenMenu,
   CloseMenu,
 
@@ -114,7 +113,6 @@ function calculateActiveItemIndex(
 }
 
 type Actions =
-  | { type: ActionTypes.ToggleMenu }
   | { type: ActionTypes.CloseMenu }
   | { type: ActionTypes.OpenMenu }
   | { type: ActionTypes.GoToItem; focus: Focus; id?: string }
@@ -129,13 +127,6 @@ const reducers: {
     action: Extract<Actions, { type: P }>
   ) => StateDefinition
 } = {
-  [ActionTypes.ToggleMenu]: state => ({
-    ...state,
-    menuState: match(state.menuState, {
-      [MenuStates.Open]: MenuStates.Closed,
-      [MenuStates.Closed]: MenuStates.Open,
-    }),
-  }),
   [ActionTypes.CloseMenu]: state => ({ ...state, menuState: MenuStates.Closed }),
   [ActionTypes.OpenMenu]: state => ({ ...state, menuState: MenuStates.Open }),
   [ActionTypes.GoToItem]: (state, action) => {
@@ -237,17 +228,17 @@ export function Menu<TTag extends React.ElementType = typeof DEFAULT_MENU_TAG>(
 
   React.useEffect(() => {
     function handler(event: PointerEvent) {
-      if (event.defaultPrevented) return
       if (menuState !== MenuStates.Open) return
+      if (buttonRef.current?.contains(event.target as HTMLElement)) return
 
       if (!itemsRef.current?.contains(event.target as HTMLElement)) {
         dispatch({ type: ActionTypes.CloseMenu })
-        d.nextFrame(() => buttonRef.current?.focus())
+        if (!event.defaultPrevented) buttonRef.current?.focus()
       }
     }
 
-    window.addEventListener('pointerdown', handler)
-    return () => window.removeEventListener('pointerdown', handler)
+    window.addEventListener('pointerup', handler)
+    return () => window.removeEventListener('pointerup', handler)
   }, [menuState, itemsRef, buttonRef, d, dispatch])
 
   const propsBag = React.useMemo(() => ({ open: menuState === MenuStates.Open }), [menuState])
@@ -272,7 +263,6 @@ type ButtonPropsWeControl =
   | 'onFocus'
   | 'onBlur'
   | 'onPointerUp'
-  | 'onPointerDown'
 
 const DEFAULT_BUTTON_TAG = 'button'
 
@@ -320,16 +310,18 @@ const Button = forwardRefWithAs(function Button<
     [dispatch, state, d]
   )
 
-  const handlePointerDown = React.useCallback((event: React.PointerEvent<HTMLButtonElement>) => {
-    // We have a `pointerdown` event listener in the menu for the 'outside click', so we just want
-    // to prevent going there if we happen to click this button.
-    event.preventDefault()
-  }, [])
-
-  const handlePointerUp = React.useCallback(() => {
-    dispatch({ type: ActionTypes.ToggleMenu })
-    d.nextFrame(() => state.itemsRef.current?.focus())
-  }, [dispatch, d, state])
+  const handlePointerUp = React.useCallback(
+    (event: MouseEvent) => {
+      if (state.menuState === MenuStates.Open) {
+        dispatch({ type: ActionTypes.CloseMenu })
+      } else {
+        event.preventDefault()
+        dispatch({ type: ActionTypes.OpenMenu })
+        d.nextFrame(() => state.itemsRef.current?.focus())
+      }
+    },
+    [dispatch, d, state]
+  )
 
   const handleFocus = React.useCallback(() => {
     if (state.menuState === MenuStates.Open) state.itemsRef.current?.focus()
@@ -354,7 +346,6 @@ const Button = forwardRefWithAs(function Button<
     onFocus: handleFocus,
     onBlur: handleBlur,
     onPointerUp: handlePointerUp,
-    onPointerDown: handlePointerDown,
   }
 
   return render({ ...passthroughProps, ...propsWeControl }, propsBag, DEFAULT_BUTTON_TAG)

packages/@headlessui-vue/src/components/menu/menu.test.tsx

@@ -50,12 +50,24 @@ function getMenuButton(): HTMLElement | null {
   return document.querySelector('button')
 }
 
+function getMenuButtons(): HTMLElement[] {
+  // This is just an assumption for our tests. We assume that we only have 1 button. And if we have
+  // more, than we assume that it is the first one.
+  return Array.from(document.querySelectorAll('[role="button"],button'))
+}
+
 function getMenu(): HTMLElement | null {
   // This is just an assumption for our tests. We assume that our menu has this role and that it is
   // the first item in the DOM.
   return document.querySelector('[role="menu"]')
 }
 
+function getMenus(): HTMLElement[] {
+  // This is just an assumption for our tests. We assume that our menu has this role and that it is
+  // the first item in the DOM.
+  return Array.from(document.querySelectorAll('[role="menu"]'))
+}
+
 function getMenuItems(): HTMLElement[] {
   // This is just an assumption for our tests. We assume that all menu items have this role.
   return Array.from(document.querySelectorAll('[role="menuitem"]'))
@@ -2207,6 +2219,50 @@ describe('Mouse interactions', () => {
     assertMenu(getMenu(), { state: MenuState.Closed })
   })
 
+  it(
+    'should be possible to click outside of the menu on another menu button which should close the current menu and open the new menu',
+    suppressConsoleLogs(async () => {
+      renderTemplate(`
+        <div>
+          <Menu>
+            <MenuButton>Trigger</MenuButton>
+            <MenuItems>
+              <MenuItem>alice</MenuItem>
+              <MenuItem>bob</MenuItem>
+              <MenuItem>charlie</MenuItem>
+            </MenuItems>
+          </Menu>
+
+          <Menu>
+            <MenuButton>Trigger</MenuButton>
+            <MenuItems>
+              <MenuItem>alice</MenuItem>
+              <MenuItem>bob</MenuItem>
+              <MenuItem>charlie</MenuItem>
+            </MenuItems>
+          </Menu>
+        </div>
+      `)
+
+      const [button1, button2] = getMenuButtons()
+
+      // Click the first menu button
+      await click(button1)
+      expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible
+
+      // Ensure the open menu is linked to the first button
+      assertMenuButtonLinkedWithMenu(button1, getMenu())
+
+      // Click the second menu button
+      await click(button2)
+
+      expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible
+
+      // Ensure the open menu is linked to the second button
+      assertMenuButtonLinkedWithMenu(button2, getMenu())
+    })
+  )
+
   it('should be possible to hover an item and make it active', async () => {
     renderTemplate(`
       <Menu>

packages/@headlessui-vue/src/components/menu/menu.ts

@@ -59,7 +59,6 @@ type StateDefinition = {
   activeItemIndex: Ref<number | null>
 
   // State mutators
-  toggleMenu(): void
   closeMenu(): void
   openMenu(): void
   goToItem(focus: Focus, id?: string): void
@@ -141,12 +140,6 @@ export const Menu = defineComponent({
       items,
       searchQuery,
       activeItemIndex,
-      toggleMenu() {
-        menuState.value = match(menuState.value, {
-          [MenuStates.Closed]: MenuStates.Open,
-          [MenuStates.Open]: MenuStates.Closed,
-        })
-      },
       closeMenu: () => (menuState.value = MenuStates.Closed),
       openMenu: () => (menuState.value = MenuStates.Open),
       goToItem(focus: Focus, id?: string) {
@@ -195,17 +188,17 @@ export const Menu = defineComponent({
 
     onMounted(() => {
       function handler(event: PointerEvent) {
-        if (event.defaultPrevented) return
         if (menuState.value !== MenuStates.Open) return
+        if (buttonRef.value?.contains(event.target as HTMLElement)) return
 
         if (!itemsRef.value?.contains(event.target as HTMLElement)) {
           api.closeMenu()
-          nextTick(() => buttonRef.value?.focus())
+          if (!event.defaultPrevented) buttonRef.value?.focus()
         }
       }
 
-      window.addEventListener('pointerdown', handler)
-      onUnmounted(() => window.removeEventListener('pointerdown', handler))
+      window.addEventListener('pointerup', handler)
+      onUnmounted(() => window.removeEventListener('pointerup', handler))
     })
 
     // @ts-expect-error Types of property 'dataRef' are incompatible.
@@ -234,7 +227,6 @@ export const MenuButton = defineComponent({
       onKeyDown: this.handleKeyDown,
       onFocus: this.handleFocus,
       onPointerUp: this.handlePointerUp,
-      onPointerDown: this.handlePointerDown,
     }
 
     return render({
@@ -274,15 +266,14 @@ export const MenuButton = defineComponent({
       }
     }
 
-    function handlePointerDown(event: PointerEvent) {
-      // We have a `pointerdown` event listener in the menu for the 'outside click', so we just want
-      // to prevent going there if we happen to click this button.
-      event.preventDefault()
-    }
-
-    function handlePointerUp() {
-      api.toggleMenu()
-      nextTick(() => api.itemsRef.value?.focus())
+    function handlePointerUp(event: MouseEvent) {
+      if (api.menuState.value === MenuStates.Open) {
+        api.closeMenu()
+      } else {
+        event.preventDefault()
+        api.openMenu()
+        nextTick(() => api.itemsRef.value?.focus())
+      }
     }
 
     function handleFocus() {
@@ -293,7 +284,6 @@ export const MenuButton = defineComponent({
       id,
       el: api.buttonRef,
       handleKeyDown,
-      handlePointerDown,
       handlePointerUp,
       handleFocus,
     }