From d9c627ea25a533bedfbe35638d55c0765b71e203 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 14 Apr 2022 11:56:01 +0200 Subject: [PATCH] Revert "ActionMenu: Remove focus trap (#1984)" This reverts commit e219598b10a1de538e1939efe390655eb47c53b7. --- .changeset/actionmenu-remove-focus-trap.md | 5 -- src/ActionMenu.tsx | 14 ++- src/__tests__/ActionMenu.test.tsx | 88 +------------------ .../hooks/useMenuInitialFocus.test.tsx | 22 +---- src/hooks/index.ts | 1 - src/hooks/useMenuInitialFocus.ts | 23 ++--- src/hooks/useMenuKeyboardNavigation.ts | 88 ------------------- src/hooks/useTypeaheadFocus.ts | 24 ++--- src/stories/ActionMenu/fixtures.stories.tsx | 24 ----- 9 files changed, 23 insertions(+), 266 deletions(-) delete mode 100644 .changeset/actionmenu-remove-focus-trap.md delete mode 100644 src/hooks/useMenuKeyboardNavigation.ts diff --git a/.changeset/actionmenu-remove-focus-trap.md b/.changeset/actionmenu-remove-focus-trap.md deleted file mode 100644 index d43f6668bf0..00000000000 --- a/.changeset/actionmenu-remove-focus-trap.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@primer/react': patch ---- - -ActionMenu: Remove focus trap to enable Tab and Shift+Tab behavior diff --git a/src/ActionMenu.tsx b/src/ActionMenu.tsx index 6f1a6574d35..fa07408c99e 100644 --- a/src/ActionMenu.tsx +++ b/src/ActionMenu.tsx @@ -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({renderAnchor: null, open: false}) export type ActionMenuProps = { @@ -113,7 +111,8 @@ const Overlay: React.FC = ({children, align = 'start', ...over > const containerRef = React.createRef() - const {openWithFocus} = useMenuKeyboardNavigation(open, onOpen, onClose, containerRef, anchorRef) + const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef) + useTypeaheadFocus(open, containerRef) return ( = ({children, align = 'start', ...over align={align} overlayProps={overlayProps} focusZoneSettings={{focusOutBehavior: 'wrap'}} - focusTrapSettings={{disabled: true}} >
{ const component = HTMLRender() 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() - 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() - - 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() - - 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( - <> - - - - ) - 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() const results = await axe(container) diff --git a/src/__tests__/hooks/useMenuInitialFocus.test.tsx b/src/__tests__/hooks/useMenuInitialFocus.test.tsx index 034f4d99bb9..bc077603ddf 100644 --- a/src/__tests__/hooks/useMenuInitialFocus.test.tsx +++ b/src/__tests__/hooks/useMenuInitialFocus.test.tsx @@ -7,16 +7,11 @@ const Component = () => { const onOpen = () => setOpen(!open) const containerRef = React.createRef() - const anchorRef = React.createRef() - const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef, anchorRef) + const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef) return ( <> - {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() - - 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) - }) - }) }) diff --git a/src/hooks/index.ts b/src/hooks/index.ts index 5538e41402d..a6dd3f13016 100644 --- a/src/hooks/index.ts +++ b/src/hooks/index.ts @@ -12,4 +12,3 @@ export {useRenderForcingRef} from './useRenderForcingRef' export {useProvidedStateOrCreate} from './useProvidedStateOrCreate' export {useMenuInitialFocus} from './useMenuInitialFocus' export {useTypeaheadFocus} from './useTypeaheadFocus' -export {useMenuKeyboardNavigation} from './useMenuKeyboardNavigation' diff --git a/src/hooks/useMenuInitialFocus.ts b/src/hooks/useMenuInitialFocus.ts index a5db601efac..e742e6b49fc 100644 --- a/src/hooks/useMenuInitialFocus.ts +++ b/src/hooks/useMenuInitialFocus.ts @@ -5,25 +5,18 @@ import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' type Gesture = 'anchor-click' | 'anchor-key-press' type Callback = (gesture: Gesture, event?: React.KeyboardEvent) => unknown -export const useMenuInitialFocus = ( - open: boolean, - onOpen?: Callback, - providedContainerRef?: React.RefObject, - providedAnchorRef?: React.RefObject -) => { - const containerRef = useProvidedRefOrCreate(providedContainerRef) - const anchorRef = useProvidedRefOrCreate(providedAnchorRef) +export const useMenuInitialFocus = (open: boolean, onOpen?: Callback, providedRef?: React.RefObject) => { + const containerRef = useProvidedRefOrCreate(providedRef) const [openingKey, setOpeningKey] = React.useState(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} } diff --git a/src/hooks/useMenuKeyboardNavigation.ts b/src/hooks/useMenuKeyboardNavigation.ts deleted file mode 100644 index 2a52472d5d2..00000000000 --- a/src/hooks/useMenuKeyboardNavigation.ts +++ /dev/null @@ -1,88 +0,0 @@ -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, - anchorRef: React.RefObject -) => { - 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, - anchorRef: React.RefObject -) => { - 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, - anchorRef: React.RefObject -) => { - 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]) -} diff --git a/src/hooks/useTypeaheadFocus.ts b/src/hooks/useTypeaheadFocus.ts index 0e894621a64..7cc20ab701f 100644 --- a/src/hooks/useTypeaheadFocus.ts +++ b/src/hooks/useTypeaheadFocus.ts @@ -4,20 +4,12 @@ import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' export const TYPEAHEAD_TIMEOUT = 1000 -export const useTypeaheadFocus = ( - open: boolean, - providedContainerRef: React.RefObject, - providedAnchorRef?: React.RefObject -) => { - const containerRef = useProvidedRefOrCreate(providedContainerRef) - const anchorRef = useProvidedRefOrCreate(providedAnchorRef) +export const useTypeaheadFocus = (open: boolean, providedRef?: React.RefObject) => { + 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} } diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 2f56c87fe40..8e74c3677f9 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -678,27 +678,3 @@ export function TypeaheadTest(): JSX.Element { ) } - -export function TabTest(): JSX.Element { - return ( - <> -

Story to test Tab

- - - Toggle Menu - - - New file - - Copy link - Edit file - event.preventDefault()}> - Delete file - - - - - - - ) -}