From 2747745891ea68e01b034de8baf3cd0df3a0823b Mon Sep 17 00:00:00 2001
From: Robin Malfait <malfait.robin@gmail.com>
Date: Fri, 25 Sep 2020 16:09:40 +0200
Subject: [PATCH] ensure that you can not click disabled items

---
 .../src/components/menu/menu.test.tsx         | 68 +++++++++++++++++--
 .../@headlessui-react/src/utils/render.ts     | 39 ++++++++++-
 .../src/components/menu/menu.test.tsx         |  8 ++-
 3 files changed, 108 insertions(+), 7 deletions(-)

diff --git a/packages/@headlessui-react/src/components/menu/menu.test.tsx b/packages/@headlessui-react/src/components/menu/menu.test.tsx
index 9733e95..11bdf3d 100644
--- a/packages/@headlessui-react/src/components/menu/menu.test.tsx
+++ b/packages/@headlessui-react/src/components/menu/menu.test.tsx
@@ -643,7 +643,9 @@ describe('Keyboard interactions', () => {
             <Menu.Item as="button" onClick={clickHandler}>
               Item B
             </Menu.Item>
-            <Menu.Item as="a">Item C</Menu.Item>
+            <Menu.Item>
+              <button onClick={clickHandler}>Item C</button>
+            </Menu.Item>
           </Menu.Items>
         </Menu>
       )
@@ -673,6 +675,18 @@ describe('Keyboard interactions', () => {
 
       // Verify the button got "clicked"
       expect(clickHandler).toHaveBeenCalledTimes(1)
+
+      // Click the menu button again
+      await click(getMenuButton())
+
+      // Active the last menu item
+      await hover(getMenuItems()[2])
+
+      // Close menu, and invoke the item
+      await press(Keys.Enter)
+
+      // Verify the button got "clicked"
+      expect(clickHandler).toHaveBeenCalledTimes(2)
     })
   )
 
@@ -2467,6 +2481,48 @@ describe('Mouse interactions', () => {
     })
   )
 
+  it(
+    'should be possible to click a menu item, which closes the menu and invokes the @click handler',
+    suppressConsoleLogs(async () => {
+      const clickHandler = jest.fn()
+      render(
+        <Menu>
+          <Menu.Button>Trigger</Menu.Button>
+          <Menu.Items>
+            <Menu.Item as="a">alice</Menu.Item>
+            <Menu.Item as="button" onClick={clickHandler}>
+              bob
+            </Menu.Item>
+            <Menu.Item>
+              <button onClick={clickHandler}>charlie</button>
+            </Menu.Item>
+          </Menu.Items>
+        </Menu>
+      )
+
+      // Open menu
+      await click(getMenuButton())
+      assertMenu(getMenu(), { state: MenuState.Open })
+
+      // We should be able to click the first item
+      await click(getMenuItems()[1])
+      assertMenu(getMenu(), { state: MenuState.Closed })
+
+      // Verify the callback has been called
+      expect(clickHandler).toHaveBeenCalledTimes(1)
+
+      // Let's re-open the window for now
+      await click(getMenuButton())
+
+      // Click the last item, which should close and invoke the handler
+      await click(getMenuItems()[2])
+      assertMenu(getMenu(), { state: MenuState.Closed })
+
+      // Verify the callback has been called
+      expect(clickHandler).toHaveBeenCalledTimes(2)
+    })
+  )
+
   it(
     'should be possible to click a disabled menu item, which is a no-op',
     suppressConsoleLogs(async () => {
@@ -2567,8 +2623,8 @@ describe('Mouse interactions', () => {
             <Menu.Item as="a" onClick={clickHandler} disabled>
               bob
             </Menu.Item>
-            <Menu.Item as="a" onClick={clickHandler}>
-              charlie
+            <Menu.Item disabled>
+              <button onClick={clickHandler}>charlie</button>
             </Menu.Item>
           </Menu.Items>
         </Menu>
@@ -2581,9 +2637,11 @@ describe('Mouse interactions', () => {
       const items = getMenuItems()
 
       await focus(items[0])
-      await focus(items[1])
-      await press(Keys.Enter)
+      await click(items[1])
+      expect(clickHandler).not.toHaveBeenCalled()
 
+      // Activate the last item
+      await click(getMenuItems()[2])
       expect(clickHandler).not.toHaveBeenCalled()
     })
   )
diff --git a/packages/@headlessui-react/src/utils/render.ts b/packages/@headlessui-react/src/utils/render.ts
index 60cf47f..342ab44 100644
--- a/packages/@headlessui-react/src/utils/render.ts
+++ b/packages/@headlessui-react/src/utils/render.ts
@@ -32,7 +32,7 @@ export function render<TTag extends React.ElementType, TBag>(
         resolvedChildren,
 
         // Filter out undefined values so that they don't override the existing values
-        compact(passThroughProps)
+        mergeEventFunctions(compact(passThroughProps), resolvedChildren.props, ['onClick'])
       )
     }
   }
@@ -40,6 +40,43 @@ export function render<TTag extends React.ElementType, TBag>(
   return React.createElement(Component, passThroughProps, resolvedChildren)
 }
 
+/**
+ * We can use this function for the following useCase:
+ *
+ * <Menu.Item> <button onClick={console.log} /> </Menu.Item>
+ *
+ * Our `Menu.Item` will have an internal `onClick`, if you passthrough an `onClick` to the actual
+ * `Menu.Item` component we will call it correctly. However, when we have an `onClick` on the actual
+ * first child, that one should _also_ be called (but before this implementation, it was just
+ * overriding the `onClick`). But it is only when we *render* that we have access to the existing
+ * props of this component.
+ *
+ * It's a bit hacky, and not that clean, but it is something internal and we have tests to rely on
+ * so that we can refactor this later (if needed).
+ */
+function mergeEventFunctions(
+  passThroughProps: Record<string, any>,
+  existingProps: Record<string, any>,
+  functionsToMerge: string[]
+) {
+  let clone = Object.assign({}, passThroughProps)
+  for (let func of functionsToMerge) {
+    if (passThroughProps[func] !== undefined && existingProps[func] !== undefined) {
+      Object.assign(clone, {
+        [func](event: { defaultPrevented: boolean }) {
+          // Props we control
+          if (!event.defaultPrevented) passThroughProps[func](event)
+
+          // Existing props on the component
+          if (!event.defaultPrevented) existingProps[func](event)
+        },
+      })
+    }
+  }
+
+  return clone
+}
+
 /**
  * This is a hack, but basically we want to keep the full 'API' of the component, but we do want to
  * wrap it in a forwardRef so that we _can_ passthrough the ref
diff --git a/packages/@headlessui-vue/src/components/menu/menu.test.tsx b/packages/@headlessui-vue/src/components/menu/menu.test.tsx
index 2757c0d..f80d03a 100644
--- a/packages/@headlessui-vue/src/components/menu/menu.test.tsx
+++ b/packages/@headlessui-vue/src/components/menu/menu.test.tsx
@@ -2526,7 +2526,9 @@ describe('Mouse interactions', () => {
             <MenuItem as="a" @click="clickHandler" disabled>
               bob
             </MenuItem>
-            <MenuItem as="a" @click="clickHandler">charlie</MenuItem>
+            <MenuItem>
+              <button @click="clickHandler">charlie</button>
+            </MenuItem>
           </MenuItems>
         </Menu>
       `,
@@ -2542,7 +2544,11 @@ describe('Mouse interactions', () => {
     await focus(items[0])
     await focus(items[1])
     await press(Keys.Enter)
+    expect(clickHandler).not.toHaveBeenCalled()
 
+    // Activate the last item
+    await focus(items[2])
+    await press(Keys.Enter)
     expect(clickHandler).not.toHaveBeenCalled()
   })
 })