ActionMenu: Remove focus trap

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

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

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, useTypeaheadFocus} 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,8 +113,7 @@ const Overlay: React.FC<MenuOverlayProps> = ({children, align = 'start', ...over
  >
 
  const containerRef = React.createRef<HTMLDivElement>()
- const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef)
- useTypeaheadFocus(open, containerRef)
+ const {openWithFocus} = useMenuKeyboardNavigation(open, onOpen, onClose, containerRef, anchorRef)
 
  return (
  <AnchoredOverlay
@@ -125,6 +126,7 @@ const Overlay: React.FC<MenuOverlayProps> = ({children, align = 'start', ...over
  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
+
+ fireEvent.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,16 @@ const Component = () => {
  const onOpen = () => setOpen(!open)
 
  const containerRef = React.createRef<HTMLDivElement>()
- const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef)
+ const anchorRef = React.createRef<HTMLButtonElement>()
+ const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef, anchorRef)
 
  return (
  <>
- <button onClick={() => setOpen(true)} onKeyDown={event => openWithFocus('anchor-key-press', event)}>
+ <button
+ ref={anchorRef}
+ onClick={() => openWithFocus('anchor-click')}
+ onKeyDown={event => openWithFocus('anchor-key-press', event)}
+ >
  open container
  </button>
  {open && (
@@ -83,4 +88,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

@@ -12,3 +12,4 @@ export {useRenderForcingRef} from './useRenderForcingRef'
 export {useProvidedStateOrCreate} from './useProvidedStateOrCreate'
 export {useMenuInitialFocus} from './useMenuInitialFocus'
 export {useTypeaheadFocus} from './useTypeaheadFocus'
+export {useMenuKeyboardNavigation} from './useMenuKeyboardNavigation'

src/hooks/useMenuInitialFocus.ts

@@ -5,18 +5,25 @@ 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, onOpen?: Callback, providedRef?: React.RefObject<HTMLElement>) => {
- const containerRef = useProvidedRefOrCreate(providedRef)
+export const useMenuInitialFocus = (
+ open: boolean,
+ onOpen?: Callback,
+ providedContainerRef?: React.RefObject<HTMLElement>,
+ providedAnchorRef?: React.RefObject<HTMLElement>
+) => {
+ const containerRef = useProvidedRefOrCreate(providedContainerRef)
+ const anchorRef = useProvidedRefOrCreate(providedAnchorRef)
  const [openingKey, setOpeningKey] = React.useState<string | undefined>(undefined)
 
  const openWithFocus: Callback = (gesture, event) => {
+ if (gesture === 'anchor-click') setOpeningKey('mouse-click')
  if (gesture === 'anchor-key-press' && event) setOpeningKey(event.code)
- else setOpeningKey(undefined)
  if (typeof onOpen === 'function') onOpen(gesture, event)
  }
 
  /**
  * 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
  */
@@ -25,7 +32,11 @@ export const useMenuInitialFocus = (open: boolean, onOpen?: Callback, providedRe
  if (!openingKey || !containerRef.current) return
 
  const iterable = iterateFocusableElements(containerRef.current)
- if (['ArrowDown', 'Space', 'Enter'].includes(openingKey)) {
+
+ if (openingKey === 'mouse-click') {
+ if (anchorRef.current) anchorRef.current.focus()
+ else throw new Error('For focus management, please attach anchorRef')
+ } else 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())
@@ -34,7 +45,7 @@ export const useMenuInitialFocus = (open: boolean, onOpen?: Callback, providedRe
  const lastElement = elements[elements.length - 1]
  setTimeout(() => lastElement.focus())
  }
- }, [open, openingKey, containerRef])
+ }, [open, openingKey, containerRef, anchorRef])
 
- return {containerRef, openWithFocus}
+ return {containerRef, anchorRef, openWithFocus}
 }

src/hooks/useMenuKeyboardNavigation.ts

@@ -0,0 +1,88 @@
+import React from 'react'
+import {iterateFocusableElements} from '@primer/behaviors/utils'
+import {useMenuInitialFocus} from './useMenuInitialFocus'
+import {useTypeaheadFocus} from './useTypeaheadFocus'
+import {MenuContextProps} from '../ActionMenu'
+
+/**
+ * Keyboard navigation is a mix of 4 hooks
+ * 1. useMenuInitialFocus
+ * 2. useTypeaheadFocus
+ * 3. useCloseMenuOnTab
+ * 4. useMoveFocusToMenuItem
+ */
+export const useMenuKeyboardNavigation = (
+ open: boolean,
+ onOpen: MenuContextProps['onOpen'],
+ onClose: MenuContextProps['onClose'],
+ containerRef: React.RefObject<HTMLElement>,
+ anchorRef: React.RefObject<HTMLElement>
+) => {
+ const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef, anchorRef)
+ useTypeaheadFocus(open, containerRef, anchorRef)
+ useCloseMenuOnTab(open, onClose, containerRef, anchorRef)
+ useMoveFocusToMenuItem(open, containerRef, anchorRef)
+
+ return {openWithFocus}
+}
+
+/**
+ * When Tab or Shift+Tab is pressed, the menu should close
+ * and the focus should naturally move to the next item
+ */
+const useCloseMenuOnTab = (
+ open: boolean,
+ onClose: MenuContextProps['onClose'],
+ containerRef: React.RefObject<HTMLElement>,
+ anchorRef: React.RefObject<HTMLElement>
+) => {
+ React.useEffect(() => {
+ const container = containerRef.current
+ const anchor = anchorRef.current
+
+ const handler = (event: KeyboardEvent) => {
+ if (open && event.key === 'Tab') onClose?.('tab')
+ }
+
+ container?.addEventListener('keydown', handler)
+ anchor?.addEventListener('keydown', handler)
+ return () => {
+ container?.removeEventListener('keydown', handler)
+ anchor?.removeEventListener('keydown', handler)
+ }
+ }, [open, onClose, containerRef, anchorRef])
+}
+
+/**
+ * When Arrow Keys are pressed and the focus is on the anchor,
+ * focus should move to a menu item
+ */
+const useMoveFocusToMenuItem = (
+ open: boolean,
+ containerRef: React.RefObject<HTMLElement>,
+ anchorRef: React.RefObject<HTMLElement>
+) => {
+ React.useEffect(() => {
+ const container = containerRef.current
+ const anchor = anchorRef.current
+
+ const handler = (event: KeyboardEvent) => {
+ if (!open || !container) return
+
+ const iterable = iterateFocusableElements(container)
+
+ if (event.key === 'ArrowDown') {
+ const firstElement = iterable.next().value
+ /** We push imperative focus to the next tick to prevent React's batching */
+ setTimeout(() => firstElement?.focus())
+ } else if (event.key === 'ArrowUp') {
+ const elements = [...iterable]
+ const lastElement = elements[elements.length - 1]
+ setTimeout(() => lastElement.focus())
+ }
+ }
+
+ anchor?.addEventListener('keydown', handler)
+ return () => anchor?.addEventListener('keydown', handler)
+ }, [open, containerRef, anchorRef])
+}

src/hooks/useTypeaheadFocus.ts

@@ -4,12 +4,20 @@ import {useProvidedRefOrCreate} from './useProvidedRefOrCreate'
 
 export const TYPEAHEAD_TIMEOUT = 1000
 
-export const useTypeaheadFocus = (open: boolean, providedRef?: React.RefObject<HTMLElement>) => {
- const containerRef = useProvidedRefOrCreate(providedRef)
+export const useTypeaheadFocus = (
+ open: boolean,
+ providedContainerRef: React.RefObject<HTMLElement>,
+ providedAnchorRef?: React.RefObject<HTMLElement>
+) => {
+ const containerRef = useProvidedRefOrCreate(providedContainerRef)
+ const anchorRef = useProvidedRefOrCreate(providedAnchorRef)
 
  React.useEffect(() => {
- if (!open || !containerRef.current) return
  const container = containerRef.current
+ const anchor = anchorRef.current
+
+ // anchor is optional, but container isn't
+ if (!open || !container) return
 
  let query = ''
  let timeout: number | undefined
@@ -83,12 +91,16 @@ export const useTypeaheadFocus = (open: boolean, providedRef?: React.RefObject<H
  }
 
  container.addEventListener('keydown', handler)
- return () => container.removeEventListener('keydown', handler)
- }, [open, containerRef])
+ anchor?.addEventListener('keydown', handler)
+ return () => {
+ container.removeEventListener('keydown', handler)
+ anchor?.removeEventListener('keydown', handler)
+ }
+ }, [open, containerRef, anchorRef])
 
  const isAlphabetKey = (event: KeyboardEvent) => {
  return event.key.length === 1 && /[a-z\d]/i.test(event.key)
  }
 
- return {containerRef}
+ return {containerRef, anchorRef}
 }