Revert "ActionMenu: Remove focus trap (#1984)" (#2023)

This reverts commit e219598.

Author: siddharthkp

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

@@ -3,19 +3,17 @@ import {useSSRSafeId} from '@react-aria/ssr'
 import {TriangleDownIcon} from '@primer/octicons-react'
 import {AnchoredOverlay, AnchoredOverlayProps} from './AnchoredOverlay'
 import {OverlayProps} from './Overlay'
-import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuKeyboardNavigation} from './hooks'
+import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuInitialFocus, useTypeaheadFocus} 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'
 
-export type MenuContextProps = Pick<
+type MenuContextProps = Pick<
   AnchoredOverlayProps,
-  'anchorRef' | 'renderAnchor' | 'open' | 'onOpen' | 'anchorId'
-> & {
-  onClose?: (gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab') => void
-}
+  'anchorRef' | 'renderAnchor' | 'open' | 'onOpen' | 'onClose' | 'anchorId'
+>
 const MenuContext = React.createContext<MenuContextProps>({renderAnchor: null, open: false})
 
 export type ActionMenuProps = {
@@ -113,7 +111,8 @@ const Overlay: React.FC<MenuOverlayProps> = ({children, align = 'start', ...over
   >
 
   const containerRef = React.createRef<HTMLDivElement>()
-  const {openWithFocus} = useMenuKeyboardNavigation(open, onOpen, onClose, containerRef, anchorRef)
+  const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef)
+  useTypeaheadFocus(open, containerRef)
 
   return (
     <AnchoredOverlay
@@ -126,7 +125,6 @@ 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/ActionMenu.tsx

@@ -1,5 +1,4 @@
 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'
@@ -59,7 +58,9 @@ describe('ActionMenu', () => {
     const component = HTMLRender(<Example />)
     const button = component.getByText('Toggle Menu')
 
-    fireEvent.keyDown(button, {key: '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()
   })
@@ -82,9 +83,6 @@ 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()
 
@@ -138,86 +136,6 @@ 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__/ActionMenu.test.tsx

@@ -7,16 +7,11 @@ const Component = () => {
   const onOpen = () => setOpen(!open)
 
   const containerRef = React.createRef<HTMLDivElement>()
-  const anchorRef = React.createRef<HTMLButtonElement>()
-  const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef, anchorRef)
+  const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef)
 
   return (
     <>
-      <button
-        ref={anchorRef}
-        onClick={() => openWithFocus('anchor-click')}
-        onKeyDown={event => openWithFocus('anchor-key-press', event)}
-      >
+      <button onClick={() => setOpen(true)} onKeyDown={event => openWithFocus('anchor-key-press', event)}>
         open container
       </button>
       {open && (
@@ -88,17 +83,4 @@ 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/__tests__/hooks/useMenuInitialFocus.test.tsx

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

src/hooks/index.ts

@@ -5,25 +5,18 @@ 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,
-  providedContainerRef?: React.RefObject<HTMLElement>,
-  providedAnchorRef?: React.RefObject<HTMLElement>
-) => {
-  const containerRef = useProvidedRefOrCreate(providedContainerRef)
-  const anchorRef = useProvidedRefOrCreate(providedAnchorRef)
+export const useMenuInitialFocus = (open: boolean, onOpen?: Callback, providedRef?: React.RefObject<HTMLElement>) => {
+  const containerRef = useProvidedRefOrCreate(providedRef)
   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
    */
@@ -32,11 +25,7 @@ export const useMenuInitialFocus = (
     if (!openingKey || !containerRef.current) return
 
     const iterable = iterateFocusableElements(containerRef.current)
-
-    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)) {
+    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())
@@ -45,7 +34,7 @@ export const useMenuInitialFocus = (
       const lastElement = elements[elements.length - 1]
       setTimeout(() => lastElement.focus())
     }
-  }, [open, openingKey, containerRef, anchorRef])
+  }, [open, openingKey, containerRef])
 
-  return {containerRef, anchorRef, openWithFocus}
+  return {containerRef, openWithFocus}
 }

src/hooks/useMenuInitialFocus.ts

@@ -4,20 +4,12 @@ import {useProvidedRefOrCreate} from './useProvidedRefOrCreate'
 
 export const TYPEAHEAD_TIMEOUT = 1000
 
-export const useTypeaheadFocus = (
-  open: boolean,
-  providedContainerRef: React.RefObject<HTMLElement>,
-  providedAnchorRef?: React.RefObject<HTMLElement>
-) => {
-  const containerRef = useProvidedRefOrCreate(providedContainerRef)
-  const anchorRef = useProvidedRefOrCreate(providedAnchorRef)
+export const useTypeaheadFocus = (open: boolean, providedRef?: React.RefObject<HTMLElement>) => {
+  const containerRef = useProvidedRefOrCreate(providedRef)
 
   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
@@ -91,16 +83,12 @@ export const useTypeaheadFocus = (
     }
 
     container.addEventListener('keydown', handler)
-    anchor?.addEventListener('keydown', handler)
-    return () => {
-      container.removeEventListener('keydown', handler)
-      anchor?.removeEventListener('keydown', handler)
-    }
-  }, [open, containerRef, anchorRef])
+    return () => container.removeEventListener('keydown', handler)
+  }, [open, containerRef])
 
   const isAlphabetKey = (event: KeyboardEvent) => {
     return event.key.length === 1 && /[a-z\d]/i.test(event.key)
   }
 
-  return {containerRef, anchorRef}
+  return {containerRef}
 }