ActionMenu: Remove focus trap + Fix initial focus

.changeset/actionmenu-remove-focus-trap.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+ActionMenu: Remove focus trap to enable Tab and Shift+Tab behavior and fix initial focus on click

src/ActionMenu.tsx

@@ -3,17 +3,19 @@ import {useSSRSafeId} from '@react-aria/ssr'
 import {TriangleDownIcon} from '@primer/octicons-react'
 import {AnchoredOverlay, AnchoredOverlayProps} from './AnchoredOverlay'
 import {OverlayProps} from './Overlay'
-import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuInitialFocus, useMnemonics} from './hooks'
+import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuKeyboardNavigation} from './hooks'
 import {Divider} from './ActionList/Divider'
 import {ActionListContainerContext} from './ActionList/ActionListContainerContext'
 import {Button, ButtonProps} from './Button'
 import {MandateProps} from './utils/types'
 import {SxProp, merge} from './sx'
 
-type MenuContextProps = Pick<
+export type MenuContextProps = Pick<
   AnchoredOverlayProps,
-  'anchorRef' | 'renderAnchor' | 'open' | 'onOpen' | 'onClose' | 'anchorId'
->
+  'anchorRef' | 'renderAnchor' | 'open' | 'onOpen' | 'anchorId'
+> & {
+  onClose?: (gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab') => void
+}
 const MenuContext = React.createContext<MenuContextProps>({renderAnchor: null, open: false})
 
 export type ActionMenuProps = {
@@ -111,20 +113,20 @@ const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({children,
   >
 
   const containerRef = React.createRef<HTMLDivElement>()
-  const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef)
-  useMnemonics(open, containerRef)
+  useMenuKeyboardNavigation(open, onClose, containerRef, anchorRef)
 
   return (
     <AnchoredOverlay
       anchorRef={anchorRef}
       renderAnchor={renderAnchor}
       anchorId={anchorId}
       open={open}
-      onOpen={openWithFocus}
+      onOpen={onOpen}
       onClose={onClose}
       align={align}
       overlayProps={overlayProps}
       focusZoneSettings={{focusOutBehavior: 'wrap'}}
+      focusTrapSettings={{disabled: true}}
     >
       <div ref={containerRef}>
         <ActionListContainerContext.Provider

src/__tests__/ActionMenu.test.tsx

@@ -1,4 +1,5 @@
-import {cleanup, render as HTMLRender, waitFor, fireEvent} from '@testing-library/react'
+import {cleanup, render as HTMLRender, waitFor} from '@testing-library/react'
+import userEvent from '@testing-library/user-event'
 import 'babel-polyfill'
 import {axe, toHaveNoViolations} from 'jest-axe'
 import React from 'react'
@@ -48,54 +49,66 @@ describe('ActionMenu', () => {
 
   it('should open Menu on MenuButton click', async () => {
     const component = HTMLRender(<Example />)
-    const button = component.getByText('Toggle Menu')
-    fireEvent.click(button)
+    const button = component.getByRole('button')
+
+    const user = userEvent.setup()
+    await user.click(button)
+
     expect(component.getByRole('menu')).toBeInTheDocument()
     cleanup()
   })
 
   it('should open Menu on MenuButton keypress', async () => {
     const component = HTMLRender(<Example />)
-    const button = component.getByText('Toggle Menu')
+    const button = component.getByRole('button')
+
+    const user = userEvent.setup()
+    button.focus()
+    await user.keyboard('{Enter}')
 
-    // We pass keycode here to navigate a implementation detail in react-testing-library
-    // https://github.com/testing-library/react-testing-library/issues/269#issuecomment-455854112
-    fireEvent.keyDown(button, {key: 'Enter', charCode: 13})
     expect(component.getByRole('menu')).toBeInTheDocument()
     cleanup()
   })
 
   it('should close Menu on selecting an action with click', async () => {
     const component = HTMLRender(<Example />)
-    const button = component.getByText('Toggle Menu')
+    const button = component.getByRole('button')
 
-    fireEvent.click(button)
-    const menuItems = await waitFor(() => component.getAllByRole('menuitem'))
-    fireEvent.click(menuItems[0])
-    expect(component.queryByRole('menu')).toBeNull()
+    const user = userEvent.setup()
+    await user.click(button)
+
+    const menuItems = component.getAllByRole('menuitem')
+    await user.click(menuItems[0])
 
+    expect(component.queryByRole('menu')).toBeNull()
     cleanup()
   })
 
   it('should close Menu on selecting an action with Enter', async () => {
     const component = HTMLRender(<Example />)
-    const button = component.getByText('Toggle Menu')
+    const button = component.getByRole('button')
 
-    fireEvent.click(button)
-    const menuItems = await waitFor(() => component.getAllByRole('menuitem'))
-    fireEvent.keyPress(menuItems[0], {key: 'Enter', charCode: 13})
-    expect(component.queryByRole('menu')).toBeNull()
+    const user = userEvent.setup()
+    await user.click(button)
+
+    const menuItems = component.getAllByRole('menuitem')
+    menuItems[0].focus()
+    await user.keyboard('{Enter}')
 
+    expect(component.queryByRole('menu')).toBeNull()
     cleanup()
   })
 
   it('should not close Menu if event is prevented', async () => {
     const component = HTMLRender(<Example />)
-    const button = component.getByText('Toggle Menu')
+    const button = component.getByRole('button')
+
+    const user = userEvent.setup()
+    await user.click(button)
+
+    const menuItems = component.getAllByRole('menuitem')
+    await user.click(menuItems[3])
 
-    fireEvent.click(button)
-    const menuItems = await waitFor(() => component.getAllByRole('menuitem'))
-    fireEvent.click(menuItems[3])
     // menu should still be open
     expect(component.getByRole('menu')).toBeInTheDocument()
 
@@ -109,14 +122,16 @@ describe('ActionMenu', () => {
       </ThemeProvider>
     )
     const button = component.getByLabelText('Field type')
-    fireEvent.click(button)
+
+    const user = userEvent.setup()
+    await user.click(button)
 
     // select first item by role, that would close the menu
-    fireEvent.click(component.getAllByRole('menuitemradio')[0])
+    await user.click(component.getAllByRole('menuitemradio')[0])
     expect(component.queryByRole('menu')).not.toBeInTheDocument()
 
     // open menu again and check if the first option is checked
-    fireEvent.click(button)
+    await user.click(button)
     expect(component.getAllByRole('menuitemradio')[0]).toHaveAttribute('aria-checked', 'true')
     cleanup()
   })
@@ -128,14 +143,92 @@ describe('ActionMenu', () => {
       </ThemeProvider>
     )
     const button = component.getByLabelText('Group by')
-    fireEvent.click(button)
+
+    const user = userEvent.setup()
+    await user.click(button)
 
     expect(component.getByLabelText('Status')).toHaveAttribute('role', 'menuitemradio')
     expect(component.getByLabelText('Clear Group by')).toHaveAttribute('role', 'menuitem')
 
     cleanup()
   })
 
+  it('should keep focus on Button when menu is opened with click', async () => {
+    const component = HTMLRender(<Example />)
+    const button = component.getByRole('button')
+
+    const user = userEvent.setup()
+    await user.tab() // tab into the story, this should focus on the first button
+    expect(button).toEqual(document.activeElement) // trust, but verify
+
+    await user.click(button)
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+    expect(document.activeElement).toEqual(button)
+
+    cleanup()
+  })
+
+  it('should select first element when ArrowDown is pressed after opening Menu with click', async () => {
+    const component = HTMLRender(<Example />)
+    const button = component.getByRole('button')
+
+    const user = userEvent.setup()
+    await user.click(button)
+
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+
+    // assumes button is the active element at this point
+    await user.keyboard('{ArrowDown}')
+
+    expect(component.getAllByRole('menuitem')[0]).toEqual(document.activeElement)
+    cleanup()
+  })
+
+  it('should select last element when ArrowUp is pressed after opening Menu with click', async () => {
+    const component = HTMLRender(<Example />)
+
+    const button = component.getByRole('button')
+
+    const user = userEvent.setup()
+    await user.click(button)
+
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+
+    // button should be the active element
+    // assumes button is the active element at this point
+    await user.keyboard('{ArrowUp}')
+
+    expect(component.getAllByRole('menuitem').pop()).toEqual(document.activeElement)
+    cleanup()
+  })
+
+  it('should close the menu if Tab is pressed and move to next element', async () => {
+    const component = HTMLRender(
+      <>
+        <Example />
+        <input type="text" placeholder="next focusable element" />
+      </>
+    )
+    const anchor = component.getByRole('button')
+
+    const user = userEvent.setup()
+    await user.tab() // tab into the story, this should focus on the first button
+    expect(anchor).toEqual(document.activeElement) // trust, but verify
+
+    await user.keyboard('{Enter}')
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+    expect(component.getAllByRole('menuitem')[0]).toEqual(document.activeElement)
+
+    // TODO: revisit why we need this waitFor
+    await waitFor(async () => {
+      await user.tab()
+      expect(document.activeElement).toEqual(component.getByPlaceholderText('next focusable element'))
+      expect(component.queryByRole('menu')).not.toBeInTheDocument()
+    })
+
+    cleanup()
+  })
+
   it('should have no axe violations', async () => {
     const {container} = HTMLRender(<Example />)
     const results = await axe(container)

src/__tests__/hooks/useMenuInitialFocus.test.tsx

@@ -7,11 +7,12 @@ const Component = () => {
   const onOpen = () => setOpen(!open)
 
   const containerRef = React.createRef<HTMLDivElement>()
-  const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef)
+  const anchorRef = React.createRef<HTMLButtonElement>()
+  useMenuInitialFocus(open, containerRef, anchorRef)
 
   return (
     <>
-      <button onClick={() => setOpen(true)} onKeyDown={event => openWithFocus('anchor-key-press', event)}>
+      <button ref={anchorRef} onClick={() => onOpen()} onKeyDown={() => onOpen()}>
         open container
       </button>
       {open && (
@@ -83,4 +84,17 @@ describe('useMenuInitialFocus', () => {
       expect(document.body).toEqual(document.activeElement)
     })
   })
+
+  it('should keep focus on trigger when opened with click', async () => {
+    const {getByText} = render(<Component />)
+
+    const button = getByText('open container')
+    button.focus() // browsers do this automatically on click, but tests don't
+    expect(button).toEqual(document.activeElement)
+    fireEvent.click(button)
+
+    await waitFor(() => {
+      expect(button).toEqual(document.activeElement)
+    })
+  })
 })

src/hooks/index.ts

@@ -11,4 +11,5 @@ export type {UseOverlaySettings} from './useOverlay'
 export {useRenderForcingRef} from './useRenderForcingRef'
 export {useProvidedStateOrCreate} from './useProvidedStateOrCreate'
 export {useMenuInitialFocus} from './useMenuInitialFocus'
+export {useMenuKeyboardNavigation} from './useMenuKeyboardNavigation'
 export {useMnemonics} from './useMnemonics'