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 userEvent from '@testing-library/user-event'
 import 'babel-polyfill'
 import {axe, toHaveNoViolations} from 'jest-axe'
 import React from 'react'
@@ -58,9 +59,7 @@ describe('ActionMenu', () => {
     const component = HTMLRender(<Example />)
     const button = component.getByText('Toggle Menu')
 
-    // 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})
+    fireEvent.keyDown(button, {key: 'Enter'})
     expect(component.getByRole('menu')).toBeInTheDocument()
     cleanup()
   })
@@ -83,6 +82,9 @@ describe('ActionMenu', () => {
 
     fireEvent.click(button)
     const menuItems = await waitFor(() => component.getAllByRole('menuitem'))
+
+    // 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.keyPress(menuItems[0], {key: 'Enter', charCode: 13})
     expect(component.queryByRole('menu')).toBeNull()
 
@@ -136,6 +138,86 @@ describe('ActionMenu', () => {
     cleanup()
   })
 
+  it('should keep focus on Button when menu is opened with click', async () => {
+    const component = HTMLRender(<Example />)
+    const button = component.getByRole('button')
+
+    userEvent.tab() // tab into the story, this should focus on the first button
+    expect(button).toEqual(document.activeElement) // trust, but verify
+
+    userEvent.click(button)
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+
+    /** We use waitFor because the hook uses an effect with setTimeout
+     *  and we need to wait for that to happen in the next tick
+     */
+    await waitFor(() => 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.getByText('Toggle Menu')
+    button.focus() // browsers do this automatically on click, but tests don't
+    fireEvent.click(button)
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+
+    // button should be the active element
+    fireEvent.keyDown(document.activeElement!, {key: 'ArrowDown', code: 'ArrowDown'})
+
+    await waitFor(() => {
+      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.getByText('Toggle Menu')
+    button.focus() // browsers do this automatically on click, but tests don't
+    fireEvent.click(button)
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+
+    // button should be the active element
+    fireEvent.keyDown(document.activeElement!, {key: 'ArrowUp', code: 'ArrowUp'})
+
+    await waitFor(() => {
+      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')
+
+    userEvent.tab() // tab into the story, this should focus on the first button
+    expect(anchor).toEqual(document.activeElement) // trust, but verify
+
+    fireEvent.keyDown(anchor, {key: 'Enter'})
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+
+    expect(component.getAllByRole('menuitem')[0]).toEqual(document.activeElement)
+
+    await waitFor(() => {
+      userEvent.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'

src/hooks/useMenuInitialFocus.ts

@@ -1,40 +1,77 @@
 import React from 'react'
 import {iterateFocusableElements} from '@primer/behaviors/utils'
-import {useProvidedRefOrCreate} from './useProvidedRefOrCreate'
 
-type Gesture = 'anchor-click' | 'anchor-key-press'
-type Callback = (gesture: Gesture, event?: React.KeyboardEvent<HTMLElement>) => unknown
+export const useMenuInitialFocus = (
+  open: boolean,
+  containerRef: React.RefObject<HTMLElement>,
+  anchorRef: React.RefObject<HTMLElement>
+) => {
+  /**
+   * We need to pick the first element to focus based on how the menu was opened,
+   * however, we need to wait for the menu to be open to set focus.
+   * This is why we use set openingKey in state and have 2 effects
+   */
+  const [openingGesture, setOpeningGesture] = React.useState<string | undefined>(undefined)
+
+  React.useEffect(
+    function inferOpeningKey() {
+      const anchorElement = anchorRef.current
 
-export const useMenuInitialFocus = (open: boolean, onOpen?: Callback, providedRef?: React.RefObject<HTMLElement>) => {
-  const containerRef = useProvidedRefOrCreate(providedRef)
-  const [openingKey, setOpeningKey] = React.useState<string | undefined>(undefined)
+      const clickHandler = (event: MouseEvent) => {
+        // event.detail === 0 means onClick was fired by Enter/Space key
+        // https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/detail
+        if (event.detail !== 0) setOpeningGesture('mouse-click')
+      }
+      const keydownHandler = (event: KeyboardEvent) => {
+        if (['Space', 'Enter', 'ArrowDown', 'ArrowUp'].includes(event.code)) {
+          setOpeningGesture(event.code)
+        }
+      }
 
-  const openWithFocus: Callback = (gesture, event) => {
-    if (gesture === 'anchor-key-press' && event) setOpeningKey(event.code)
-    else setOpeningKey(undefined)
-    if (typeof onOpen === 'function') onOpen(gesture, event)
-  }
+      anchorElement?.addEventListener('click', clickHandler)
+      anchorElement?.addEventListener('keydown', keydownHandler)
+      return () => {
+        anchorElement?.removeEventListener('click', clickHandler)
+        anchorElement?.removeEventListener('keydown', keydownHandler)
+      }
+    },
+    [anchorRef]
+  )
 
   /**
    * Pick the first element to focus based on the key used to open the Menu
+   * Click: anchor
    * ArrowDown | Space | Enter: first element
    * ArrowUp: last element
    */
-  React.useEffect(() => {
-    if (!open) return
-    if (!openingKey || !containerRef.current) return
+  React.useEffect(
+    function moveFocusOnOpen() {
+      if (!open || !containerRef.current) return // wait till the menu is open
 
-    const iterable = iterateFocusableElements(containerRef.current)
-    if (['ArrowDown', 'Space', 'Enter'].includes(openingKey)) {
-      const firstElement = iterable.next().value
-      /** We push imperative focus to the next tick to prevent React's batching */
-      setTimeout(() => firstElement?.focus())
-    } else if (['ArrowUp'].includes(openingKey)) {
-      const elements = [...iterable]
-      const lastElement = elements[elements.length - 1]
-      setTimeout(() => lastElement.focus())
-    }
-  }, [open, openingKey, containerRef])
+      const iterable = iterateFocusableElements(containerRef.current)
 
-  return {containerRef, openWithFocus}
+      if (openingGesture === 'mouse-click') {
+        if (anchorRef.current) anchorRef.current.focus()
+        else throw new Error('For focus management, please attach anchorRef')
+      } else if (openingGesture && ['ArrowDown', 'Space', 'Enter'].includes(openingGesture)) {
+        const firstElement = iterable.next().value
+        /** We push imperative focus to the next tick to prevent React's batching */
+        setTimeout(() => firstElement?.focus())
+      } else if ('ArrowUp' === openingGesture) {
+        const elements = [...iterable]
+        const lastElement = elements[elements.length - 1]
+        setTimeout(() => lastElement.focus())
+      } else {
+        /** if the menu was not opened with the anchor, we default to the first element
+         *  for example: with keyboard shortcut (see stories/fixtures)
+         */
+        const firstElement = iterable.next().value
+        setTimeout(() => firstElement?.focus())
+      }
+    },
+    // we don't want containerRef in dependencies
+    // because re-renders to containerRef while it's open should not fire initialMenuFocus
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [open, openingGesture, anchorRef]
+  )
 }