ActionMenu: Make sure event handlers on trigger are called (#4648)

* wip: add mergeAnchorHandlers

* merged onClick and onKeyDown in ActionMenu.Anchor

* improve types

* Create green-schools-smell.md

* cover cases with Tooltip

.changeset/green-schools-smell.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+ActionMenu: Make sure event handlers on ActionMenu.Button and ActionMenu.Anchor are called

packages/react/src/ActionMenu/ActionMenu.tsx

@@ -43,6 +43,31 @@ export type ActionMenuProps = {
   onOpenChange?: (s: boolean) => void
 } & Pick<AnchoredOverlayProps, 'anchorRef'>
 
+// anchorProps adds onClick and onKeyDown, so we need to merge them with buttonProps
+const mergeAnchorHandlers = (anchorProps: React.HTMLAttributes<HTMLElement>, buttonProps: ButtonProps) => {
+  const mergedAnchorProps = {...anchorProps}
+
+  if (typeof buttonProps.onClick === 'function') {
+    const anchorOnClick = anchorProps.onClick
+    const mergedOnClick = (event: React.MouseEvent<HTMLButtonElement>) => {
+      buttonProps.onClick?.(event)
+      anchorOnClick?.(event)
+    }
+    mergedAnchorProps.onClick = mergedOnClick
+  }
+
+  if (typeof buttonProps.onKeyDown === 'function') {
+    const anchorOnKeyDown = anchorProps.onKeyDown
+    const mergedOnAnchorKeyDown = (event: React.KeyboardEvent<HTMLButtonElement>) => {
+      buttonProps.onKeyDown?.(event)
+      anchorOnKeyDown?.(event)
+    }
+    mergedAnchorProps.onKeyDown = mergedOnAnchorKeyDown
+  }
+
+  return mergedAnchorProps
+}
+
 const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
   anchorRef: externalAnchorRef,
   open,
@@ -87,7 +112,10 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
       if (anchorChildren.type === MenuButton) {
         renderAnchor = anchorProps => {
           // We need to attach the anchor props to the tooltip trigger (ActionMenu.Button's grandchild) not the tooltip itself.
-          const triggerButton = React.cloneElement(anchorChildren, {...anchorProps})
+          const triggerButton = React.cloneElement(
+            anchorChildren,
+            mergeAnchorHandlers({...anchorProps}, anchorChildren.props),
+          )
           return React.cloneElement(child, {children: triggerButton, ref: anchorRef})
         }
       }
@@ -101,7 +129,10 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
             // ActionMenu.Anchor's children can be wrapped with Tooltip. If this is the case, our anchor is the tooltip's trigger
             const tooltipTrigger = anchorChildren.props.children
             // We need to attach the anchor props to the tooltip trigger not the tooltip itself.
-            const tooltipTriggerEl = React.cloneElement(tooltipTrigger, {...anchorProps})
+            const tooltipTriggerEl = React.cloneElement(
+              tooltipTrigger,
+              mergeAnchorHandlers({...anchorProps}, tooltipTrigger.props),
+            )
             const tooltip = React.cloneElement(anchorChildren, {children: tooltipTriggerEl})
             return React.cloneElement(child, {children: tooltip, ref: anchorRef})
           }
@@ -111,7 +142,7 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
       }
       return null
     } else if (child.type === MenuButton) {
-      renderAnchor = anchorProps => React.cloneElement(child, anchorProps)
+      renderAnchor = anchorProps => React.cloneElement(child, mergeAnchorHandlers(anchorProps, child.props))
       return null
     } else {
       return child
@@ -136,18 +167,28 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
   )
 }
 
-export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string}
-const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children, ...anchorProps}, anchorRef) => {
+export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string} & React.HTMLAttributes<HTMLElement>
+const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children: child, ...anchorProps}, anchorRef) => {
   const {onOpen, isSubmenu} = React.useContext(MenuContext)
 
   const openSubmenuOnRightArrow: React.KeyboardEventHandler<HTMLElement> = useCallback(
     event => {
-      children.props.onKeyDown?.(event)
       if (isSubmenu && event.key === 'ArrowRight' && !event.defaultPrevented) onOpen?.('anchor-key-press')
     },
-    [children, isSubmenu, onOpen],
+    [isSubmenu, onOpen],
   )
 
+  const onButtonClick = (event: React.MouseEvent<HTMLElement>) => {
+    child.props.onClick?.(event)
+    anchorProps.onClick?.(event) // onClick is passed from AnchoredOverlay
+  }
+
+  const onButtonKeyDown = (event: React.KeyboardEvent<HTMLElement>) => {
+    child.props.onKeyDown?.(event)
+    openSubmenuOnRightArrow(event)
+    anchorProps.onKeyDown?.(event) // onKeyDown is passed from AnchoredOverlay
+  }
+
   // Add right chevron icon to submenu anchors rendered using `ActionList.Item`
   const parentActionListContext = useContext(ActionListContainerContext)
   const thisActionListContext = useMemo(
@@ -165,10 +206,11 @@ const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children,
 
   return (
     <ActionListContainerContext.Provider value={thisActionListContext}>
-      {React.cloneElement(children, {
+      {React.cloneElement(child, {
         ...anchorProps,
         ref: anchorRef,
-        onKeyDown: openSubmenuOnRightArrow,
+        onClick: onButtonClick,
+        onKeyDown: onButtonKeyDown,
       })}
     </ActionListContainerContext.Provider>
   )

packages/react/src/__tests__/ActionMenu.test.tsx

@@ -641,4 +641,172 @@ describe('ActionMenu', () => {
       expect(baseAnchor).not.toHaveAttribute('aria-expanded', 'true')
     })
   })
+
+  describe('calls event handlers on trigger', () => {
+    it('should call onClick and onKeyDown passed to ActionMenu.Button', async () => {
+      const mockOnClick = jest.fn()
+      const mockOnKeyDown = jest.fn()
+
+      const component = HTMLRender(
+        <ThemeProvider theme={theme}>
+          <ActionMenu>
+            <ActionMenu.Button onClick={mockOnClick} onKeyDown={mockOnKeyDown}>
+              Open menu
+            </ActionMenu.Button>
+            <ActionMenu.Overlay>
+              <ActionList>
+                <ActionList.Item>New file</ActionList.Item>
+                <ActionList.Item>Copy link</ActionList.Item>
+              </ActionList>
+            </ActionMenu.Overlay>
+          </ActionMenu>
+        </ThemeProvider>,
+      )
+
+      const user = userEvent.setup()
+      const button = component.getByRole('button')
+      await user.click(button)
+
+      expect(component.getByRole('menu')).toBeInTheDocument()
+      expect(mockOnClick).toHaveBeenCalledTimes(1)
+
+      // select and close menu
+      const menuItems = component.getAllByRole('menuitem')
+      await user.click(menuItems[0])
+      expect(component.queryByRole('menu')).toBeNull()
+
+      expect(button).toEqual(document.activeElement)
+      await user.keyboard('{Enter}')
+      expect(component.queryByRole('menu')).toBeInTheDocument()
+      expect(mockOnKeyDown).toHaveBeenCalledTimes(1)
+    })
+
+    it('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor', async () => {
+      const mockOnClick = jest.fn()
+      const mockOnKeyDown = jest.fn()
+
+      const component = HTMLRender(
+        <ThemeProvider theme={theme}>
+          <ActionMenu>
+            <ActionMenu.Anchor>
+              <IconButton
+                icon={KebabHorizontalIcon}
+                aria-label="Open menu"
+                onClick={mockOnClick}
+                onKeyDown={mockOnKeyDown}
+              />
+            </ActionMenu.Anchor>
+            <ActionMenu.Overlay>
+              <ActionList>
+                <ActionList.Item>New file</ActionList.Item>
+                <ActionList.Item>Copy link</ActionList.Item>
+              </ActionList>
+            </ActionMenu.Overlay>
+          </ActionMenu>
+        </ThemeProvider>,
+      )
+
+      const user = userEvent.setup()
+      const button = component.getByRole('button')
+      await user.click(button)
+
+      expect(component.getByRole('menu')).toBeInTheDocument()
+      expect(mockOnClick).toHaveBeenCalledTimes(1)
+
+      // select and close menu
+      const menuItems = component.getAllByRole('menuitem')
+      await user.click(menuItems[0])
+      expect(component.queryByRole('menu')).toBeNull()
+
+      expect(button).toEqual(document.activeElement)
+      await user.keyboard('{Enter}')
+      expect(component.queryByRole('menu')).toBeInTheDocument()
+      expect(mockOnKeyDown).toHaveBeenCalledTimes(1)
+    })
+
+    it('should call onClick and onKeyDown passed to ActionMenu.Button with Tooltip', async () => {
+      const mockOnClick = jest.fn()
+      const mockOnKeyDown = jest.fn()
+
+      const component = HTMLRender(
+        <ThemeProvider theme={theme}>
+          <ActionMenu>
+            <TooltipV2 text="Additional context about the menu button" direction="s">
+              <ActionMenu.Button onClick={mockOnClick} onKeyDown={mockOnKeyDown}>
+                Open menu
+              </ActionMenu.Button>
+            </TooltipV2>
+            <ActionMenu.Overlay>
+              <ActionList>
+                <ActionList.Item>New file</ActionList.Item>
+                <ActionList.Item>Copy link</ActionList.Item>
+              </ActionList>
+            </ActionMenu.Overlay>
+          </ActionMenu>
+        </ThemeProvider>,
+      )
+
+      const user = userEvent.setup()
+      const button = component.getByRole('button')
+      await user.click(button)
+
+      expect(component.getByRole('menu')).toBeInTheDocument()
+      expect(mockOnClick).toHaveBeenCalledTimes(1)
+
+      // select and close menu
+      const menuItems = component.getAllByRole('menuitem')
+      await user.click(menuItems[0])
+      expect(component.queryByRole('menu')).toBeNull()
+
+      expect(button).toEqual(document.activeElement)
+      await user.keyboard('{Enter}')
+      expect(component.queryByRole('menu')).toBeInTheDocument()
+      expect(mockOnKeyDown).toHaveBeenCalledTimes(1)
+    })
+
+    it('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor with Tooltip', async () => {
+      const mockOnClick = jest.fn()
+      const mockOnKeyDown = jest.fn()
+
+      const component = HTMLRender(
+        <ThemeProvider theme={theme}>
+          <ActionMenu>
+            <ActionMenu.Anchor>
+              <TooltipV2 text="Additional context about the menu button" direction="s">
+                <IconButton
+                  icon={KebabHorizontalIcon}
+                  aria-label="Open menu"
+                  onClick={mockOnClick}
+                  onKeyDown={mockOnKeyDown}
+                />
+              </TooltipV2>
+            </ActionMenu.Anchor>
+            <ActionMenu.Overlay>
+              <ActionList>
+                <ActionList.Item>New file</ActionList.Item>
+                <ActionList.Item>Copy link</ActionList.Item>
+              </ActionList>
+            </ActionMenu.Overlay>
+          </ActionMenu>
+        </ThemeProvider>,
+      )
+
+      const user = userEvent.setup()
+      const button = component.getByRole('button')
+      await user.click(button)
+
+      expect(component.getByRole('menu')).toBeInTheDocument()
+      expect(mockOnClick).toHaveBeenCalledTimes(1)
+
+      // select and close menu
+      const menuItems = component.getAllByRole('menuitem')
+      await user.click(menuItems[0])
+      expect(component.queryByRole('menu')).toBeNull()
+
+      expect(button).toEqual(document.activeElement)
+      await user.keyboard('{Enter}')
+      expect(component.queryByRole('menu')).toBeInTheDocument()
+      expect(mockOnKeyDown).toHaveBeenCalledTimes(1)
+    })
+  })
 })