Fix ActionMenu bugs

.changeset/fix-action-menu-split-button-click.md

@@ -0,0 +1,10 @@
+---
+'@primer/react-brand': minor
+---
+
+Multiple `ActionMenu` component updates:
+
+- Calls consumer-provided `onClick` handlers in default and split-button modes.
+- ⚠️ Breaking change to `ActionMenu.Button` in split-button mode. Custom HTML attributes are now correctly forwarded to the primary action instead of the menu toggle. This was the originally intended behavior.
+- Forwards the `variant` prop in `default` mode correctly. `ActionMenu` can now render in all available `Button` variants.
+- Forwards the `leadingVisual` prop in `default` mode.

packages/react/src/ActionMenu/ActionMenu.features.stories.tsx

@@ -61,6 +61,36 @@ export const SingleSelectionSmallOpen = () => {
   )
 }
 
+export const DefaultModeAllVariants = () => {
+  const {t} = useTranslation('ActionMenu')
+
+  return (
+    <Stack direction="horizontal" gap="condensed">
+      {ButtonVariants.map(variant => (
+        <ActionMenu key={variant}>
+          <ActionMenu.Button variant={variant}>{t('open_menu')}</ActionMenu.Button>
+          <ActionMenu.Overlay aria-label={t('actions')}>
+            <ActionMenu.Item value="Copy link">{t('copy_link')}</ActionMenu.Item>
+          </ActionMenu.Overlay>
+        </ActionMenu>
+      ))}
+    </Stack>
+  )
+}
+
+export const DefaultModeLeadingVisual = () => {
+  const {t} = useTranslation('ActionMenu')
+
+  return (
+    <ActionMenu>
+      <ActionMenu.Button leadingVisual={<VisualStudioCodeLogo />}>{t('open_menu')}</ActionMenu.Button>
+      <ActionMenu.Overlay aria-label={t('actions')}>
+        <ActionMenu.Item value="Copy link">{t('copy_link')}</ActionMenu.Item>
+      </ActionMenu.Overlay>
+    </ActionMenu>
+  )
+}
+
 export const SplitButtonMode = () => {
   const {t} = useTranslation('ActionMenu')
 

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

@@ -1,6 +1,7 @@
 import React, {useState} from 'react'
 import {render, cleanup, fireEvent, waitFor} from '@testing-library/react'
 import '@testing-library/jest-dom'
+import userEvent from '@testing-library/user-event'
 import {axe, toHaveNoViolations} from 'jest-axe'
 
 import {ActionMenu} from './ActionMenu'
@@ -111,6 +112,81 @@ describe('ActionMenu', () => {
     )
   })
 
+  it("should forward the user's onClick callback and toggle the menu", () => {
+    const mockOnClick = jest.fn()
+    const {getByRole, queryByLabelText} = render(
+      <ActionMenu>
+        <ActionMenu.Button onClick={mockOnClick}>Open menu</ActionMenu.Button>
+        <ActionMenu.Overlay aria-label="Actions">
+          <ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>,
+    )
+
+    fireEvent.click(getByRole('button', {name: 'Open menu'}))
+
+    expect(mockOnClick).toHaveBeenCalledTimes(1)
+    expect(queryByLabelText('Actions')).toBeInTheDocument()
+  })
+
+  it("should not toggle the menu when the user's onClick callback prevents the default action", () => {
+    const mockOnClick = jest.fn(event => event.preventDefault())
+    const {getByRole, queryByLabelText} = render(
+      <ActionMenu>
+        <ActionMenu.Button onClick={mockOnClick}>Open menu</ActionMenu.Button>
+        <ActionMenu.Overlay aria-label="Actions">
+          <ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>,
+    )
+
+    fireEvent.click(getByRole('button', {name: 'Open menu'}))
+
+    expect(mockOnClick).toHaveBeenCalledTimes(1)
+    expect(queryByLabelText('Actions')).not.toBeInTheDocument()
+  })
+
+  it('should apply the button variant in default mode', () => {
+    const {getByRole} = render(
+      <ActionMenu>
+        <ActionMenu.Button variant="subtle">Open menu</ActionMenu.Button>
+        <ActionMenu.Overlay aria-label="Actions">
+          <ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>,
+    )
+
+    expect(getByRole('button', {name: 'Open menu'})).toHaveClass('Button--subtle')
+  })
+
+  it('should use the secondary button variant by default in default mode', () => {
+    const {getByRole} = render(
+      <ActionMenu>
+        <ActionMenu.Button>Open menu</ActionMenu.Button>
+        <ActionMenu.Overlay aria-label="Actions">
+          <ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>,
+    )
+
+    expect(getByRole('button', {name: 'Open menu'})).toHaveClass('Button--secondary')
+  })
+
+  it('should render the leading visual in default mode', () => {
+    const accessibleText = 'Test icon'
+    const TestIcon = () => <svg aria-label={accessibleText} />
+    const {getByLabelText} = render(
+      <ActionMenu>
+        <ActionMenu.Button leadingVisual={<TestIcon />}>Open menu</ActionMenu.Button>
+        <ActionMenu.Overlay aria-label="Actions">
+          <ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>,
+    )
+
+    expect(getByLabelText(accessibleText)).toBeInTheDocument()
+  })
+
   it("should set aria-haspopup to 'true' and aria-expanded to 'false' by default", () => {
     const {getByRole} = render(
       <ActionMenu onSelect={jest.fn()}>
@@ -455,6 +531,24 @@ describe('ActionMenu', () => {
     expect(mainButton).toHaveAttribute('href', '#option1')
   })
 
+  it('should forward custom attributes to the primary action in split-button mode', () => {
+    const {getByRole, getByLabelText} = render(
+      <ActionMenu mode="split-button">
+        <ActionMenu.Button as="a" href="#option1" data-attribute="test">
+          Primary Action
+        </ActionMenu.Button>
+        <ActionMenu.Overlay aria-label="Additional options">
+          <ActionMenu.Item as="a" href="#option1">
+            Option 1
+          </ActionMenu.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>,
+    )
+
+    expect(getByRole('link', {name: 'Primary Action'})).toHaveAttribute('data-attribute', 'test')
+    expect(getByLabelText('Menu')).not.toHaveAttribute('data-attribute')
+  })
+
   it('should toggle menu when clicking the chevron button', async () => {
     const {getByLabelText, queryByLabelText} = render(
       <ActionMenu mode="split-button">
@@ -501,6 +595,40 @@ describe('ActionMenu', () => {
     )
   })
 
+  it('should keep the primary and menu actions independent in split-button mode', async () => {
+    const mockOnClick = jest.fn()
+    const user = userEvent.setup()
+    const {getByRole, getByLabelText, queryByLabelText} = render(
+      <ActionMenu mode="split-button">
+        <ActionMenu.Button onClick={mockOnClick}>Primary Action</ActionMenu.Button>
+        <ActionMenu.Overlay aria-label="Additional options">
+          <ActionMenu.Item as="a" href="#option1">
+            Option 1
+          </ActionMenu.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>,
+    )
+
+    const primaryButton = getByRole('button', {name: 'Primary Action'})
+    fireEvent.click(primaryButton)
+
+    expect(mockOnClick).toHaveBeenCalledTimes(1)
+    expect(queryByLabelText('Additional options')).not.toBeInTheDocument()
+
+    await user.tab()
+    expect(primaryButton).toHaveFocus()
+    await user.keyboard('{Enter}')
+    await user.keyboard(' ')
+
+    expect(mockOnClick).toHaveBeenCalledTimes(3)
+    expect(queryByLabelText('Additional options')).not.toBeInTheDocument()
+
+    fireEvent.click(getByLabelText('Menu'))
+
+    expect(mockOnClick).toHaveBeenCalledTimes(3)
+    expect(queryByLabelText('Additional options')).toBeInTheDocument()
+  })
+
   it('should render items as links with correct href attribute', async () => {
     const {getByLabelText, getAllByRole} = render(
       <ActionMenu mode="split-button" open>
@@ -574,6 +702,24 @@ describe('ActionMenu', () => {
     expect(variantButton).toBeInTheDocument()
   })
 
+  it('should use the primary button variant by default in split-button mode', () => {
+    const {getByRole, getByLabelText} = render(
+      <ActionMenu mode="split-button">
+        <ActionMenu.Button as="a" href="#option1">
+          Primary Action
+        </ActionMenu.Button>
+        <ActionMenu.Overlay aria-label="Additional options">
+          <ActionMenu.Item as="a" href="#option1">
+            Option 1
+          </ActionMenu.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>,
+    )
+
+    expect(getByRole('link', {name: 'Primary Action'})).toHaveClass('Button--primary')
+    expect(getByLabelText('Menu')).toHaveClass('Button--primary')
+  })
+
   it('should not change main button href when menu is toggled', async () => {
     const {getByText, getByLabelText} = render(
       <ActionMenu mode="split-button">

packages/react/src/ActionMenu/ActionMenu.tsx

@@ -123,6 +123,7 @@ export type ActionMenuProps = {
 type ActionMenuContextType = {
   size?: ActionMenuSizes
   setSize?: React.Dispatch<React.SetStateAction<ActionMenuSizes | undefined>>
+  onMenuToggle?: () => void
 }
 
 const ActionMenuContext = React.createContext<ActionMenuContextType>({})
@@ -131,10 +132,16 @@ export const useActionMenuContext = (): ActionMenuContextType => {
   return React.useContext(ActionMenuContext)
 }
 
-export const ActionMenuProvider: React.FC<ActionMenuProps> = ({size, children}) => {
+type ActionMenuProviderProps = ActionMenuProps & Pick<ActionMenuContextType, 'onMenuToggle'>
+
+export const ActionMenuProvider: React.FC<ActionMenuProviderProps> = ({size, children, onMenuToggle}) => {
   const [currentSize, setSize] = useState(size)
 
-  return <ActionMenuContext.Provider value={{size: currentSize, setSize}}>{children}</ActionMenuContext.Provider>
+  return (
+    <ActionMenuContext.Provider value={{size: currentSize, setSize, onMenuToggle}}>
+      {children}
+    </ActionMenuContext.Provider>
+  )
 }
 
 const _ActionMenuRoot = memo(
@@ -274,7 +281,6 @@ const _ActionMenuRoot = memo(
     }>((acc, child) => {
       if (isValidElement<ActionMenuButtonProps>(child) && child.type === ActionMenuButton) {
         acc.Button = cloneElement(child, {
-          onClick: toggleMenu,
           ref: anchorElementRef as React.RefObject<HTMLButtonElement>,
           className: clsx(child.props.className, styles[`ActionMenu__button--${mode}`], showMenu),
           menuOpen: showMenu,
@@ -313,7 +319,7 @@ const _ActionMenuRoot = memo(
     }, {})
 
     return (
-      <ActionMenuProvider size={size}>
+      <ActionMenuProvider size={size} onMenuToggle={toggleMenu}>
         <div
           id={instanceId}
           className={clsx(styles.ActionMenu, disabled && styles['ActionMenu--disabled'])}
@@ -345,6 +351,7 @@ const ActionMenuButton = forwardRef<HTMLButtonElement, ActionMenuButtonProps>(
     {
       as,
       href,
+      id,
       children,
       className,
       'data-testid': testId,
@@ -354,43 +361,59 @@ const ActionMenuButton = forwardRef<HTMLButtonElement, ActionMenuButtonProps>(
       _mode = 'default',
       onClick,
       leadingVisual,
-      variant = 'primary',
+      variant,
       ...props
     },
     ref,
   ) => {
+    const {onMenuToggle} = useActionMenuContext()
+    const handleClick = useCallback(
+      (event: React.MouseEvent<HTMLButtonElement>) => {
+        onClick?.(event)
+
+        if (!event.defaultPrevented) {
+          onMenuToggle?.()
+        }
+      },
+      [onClick, onMenuToggle],
+    )
+
     if (_mode === 'split-button') {
+      const splitButtonVariant = variant ?? 'primary'
+
       return (
         <div className={clsx(styles.ActionMenu__button, styles[`ActionMenu__button--${size}`], className)}>
           <Button
             as={as}
             href={href}
             className={clsx(
               styles['ActionMenu__innerButton--split-button'],
-              styles[`ActionMenu__innerButton--${variant}`],
+              styles[`ActionMenu__innerButton--${splitButtonVariant}`],
               styles[`ActionMenu__innerButton--${size}`],
               disabled && styles['ActionMenu__innerButton--disabled'],
             )}
-            variant={variant}
+            variant={splitButtonVariant}
             aria-disabled={disabled}
             data-testid={testId || testIds.button}
             size={size}
             leadingVisual={leadingVisual}
+            onClick={onClick}
+            {...props}
           >
             <span className={styles['ActionMenu__button-text']}>{children}</span>
           </Button>
           <Button
             ref={ref}
+            id={id}
             as="button"
             className={styles['ActionMenu__innerButton--split-button']}
-            variant={variant}
+            variant={splitButtonVariant}
             aria-haspopup="true"
             aria-label="Menu"
             size={size}
             aria-expanded={menuOpen ? 'true' : 'false'}
-            onClick={onClick}
+            onClick={onMenuToggle}
             disabled={disabled}
-            {...props}
           >
             <ChevronDownIcon className={styles['ActionMenu__inner-button-dropdown-icon']} />
           </Button>
@@ -401,14 +424,17 @@ const ActionMenuButton = forwardRef<HTMLButtonElement, ActionMenuButtonProps>(
     return (
       <Button
         ref={ref}
+        id={id}
         className={clsx(styles.ActionMenu__button, styles[`ActionMenu__button--${size}`], className)}
         aria-haspopup="true"
         aria-expanded={menuOpen ? 'true' : 'false'}
         disabled={disabled}
         data-testid={testId || testIds.button}
         size={size}
+        variant={variant}
+        leadingVisual={leadingVisual}
         trailingVisual={<ChevronDownIcon />}
-        onClick={onClick}
+        onClick={handleClick}
         {...props}
       >
         <span className={styles['ActionMenu__button-text']}>{children}</span>

packages/react/src/ActionMenu/ActionMenu.visual.spec.ts

@@ -28,6 +28,28 @@ test.describe('Visual Comparison: ActionMenu', () => {
     await expect(page).toHaveScreenshot({fullPage: true})
   })
 
+  test('ActionMenu / Default Mode All Variants', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?args=&id=components-actionmenu-features--default-mode-all-variants&viewMode=story',
+      {waitUntil: 'networkidle'},
+    )
+    await page.locator('body.sb-show-main').waitFor({state: 'visible'})
+
+    await page.waitForTimeout(500)
+    await expect(page).toHaveScreenshot({fullPage: true})
+  })
+
+  test('ActionMenu / Default Mode Leading Visual', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?args=&id=components-actionmenu-features--default-mode-leading-visual&viewMode=story',
+      {waitUntil: 'networkidle'},
+    )
+    await page.locator('body.sb-show-main').waitFor({state: 'visible'})
+
+    await page.waitForTimeout(500)
+    await expect(page).toHaveScreenshot({fullPage: true})
+  })
+
   test('ActionMenu / Split Button Mode', async ({page}) => {
     await page.goto(
       'http://localhost:6006/iframe.html?args=&id=components-actionmenu-features--split-button-mode&viewMode=story',