ActionMenu: Remove focus trap (#1984)

* 1. disable focus trap

* add failing tests

* add more failing tests

* handle click in useMenuInitialFocus

* Fix useMenuInitialFocus test

* Add test for click with ActionMenu

* use updated menu focus

* add the option of rendering inline instead of portal

* handle tab press inside open menu

* Repurpose openWithFocus for initialising keyboard interaction

* Add anchorRef to useTypeaheadFocus

* only activate keyboard activation if the menu is open

* add tab handler to Anchor as well

* ActionMenu inside Overlay - overflow:visible, zIndex

* merge sx prop on overlay props

* update snapshot

* make anchorRef optional for typeahead

* use userEvent for tests

* Add story for Tab testing

* improve types for handler

* continue to improve types

* clean up hooks

* move effects to one big hook

* gesture type should not be in AnchoredOverlay

* add changelog

* Add docs for usePortal

* add comments to overlayProps

* remove todo

* Add overlay story with overflow

* tiny bit more coverage

* rename usePortal to renderInPortal

* Add story for position relative + overflow

* change z-index to 11, add comment why

* undo render inline

* remove story

* wrap async test in waitFor

* unused variable

* remove overlay overflow

* Delete overlay-use-portal.md

* Delete overlay-overflow.md

* remove duplicate sx row

* update snapshots

* remove irrelevant story

* remove unused imports

Author: siddharthkp

.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}
 }