diff --git a/.changeset/actionmenu-remove-focus-trap.md b/.changeset/actionmenu-remove-focus-trap.md new file mode 100644 index 00000000000..92d765283b0 --- /dev/null +++ b/.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 diff --git a/src/ActionMenu.tsx b/src/ActionMenu.tsx index 329207d046f..a88513c7e1e 100644 --- a/src/ActionMenu.tsx +++ b/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({renderAnchor: null, open: false}) export type ActionMenuProps = { @@ -111,8 +113,7 @@ const Overlay: React.FC> = ({children, > const containerRef = React.createRef() - const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef) - useMnemonics(open, containerRef) + useMenuKeyboardNavigation(open, onClose, containerRef, anchorRef) return ( > = ({children, renderAnchor={renderAnchor} anchorId={anchorId} open={open} - onOpen={openWithFocus} + onOpen={onOpen} onClose={onClose} align={align} overlayProps={overlayProps} focusZoneSettings={{focusOutBehavior: 'wrap'}} + focusTrapSettings={{disabled: true}} >
{ it('should open Menu on MenuButton click', async () => { const component = HTMLRender() - const button = component.getByText('Toggle Menu') - fireEvent.click(button) + const button = component.getByRole('button') + + const user = userEvent.setup() + await user.click(button) + expect(component.getByRole('menu')).toBeInTheDocument() cleanup() }) it('should open Menu on MenuButton keypress', async () => { const component = HTMLRender() - const button = component.getByText('Toggle Menu') + const button = component.getByRole('button') + + const user = userEvent.setup() + button.focus() + await user.keyboard('{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() }) it('should close Menu on selecting an action with click', async () => { const component = HTMLRender() - const button = component.getByText('Toggle Menu') + const button = component.getByRole('button') - fireEvent.click(button) - const menuItems = await waitFor(() => component.getAllByRole('menuitem')) - fireEvent.click(menuItems[0]) - expect(component.queryByRole('menu')).toBeNull() + const user = userEvent.setup() + await user.click(button) + + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() cleanup() }) it('should close Menu on selecting an action with Enter', async () => { const component = HTMLRender() - const button = component.getByText('Toggle Menu') + const button = component.getByRole('button') - fireEvent.click(button) - const menuItems = await waitFor(() => component.getAllByRole('menuitem')) - fireEvent.keyPress(menuItems[0], {key: 'Enter', charCode: 13}) - expect(component.queryByRole('menu')).toBeNull() + const user = userEvent.setup() + await user.click(button) + + const menuItems = component.getAllByRole('menuitem') + menuItems[0].focus() + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeNull() cleanup() }) it('should not close Menu if event is prevented', async () => { const component = HTMLRender() - const button = component.getByText('Toggle Menu') + const button = component.getByRole('button') + + const user = userEvent.setup() + await user.click(button) + + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[3]) - fireEvent.click(button) - const menuItems = await waitFor(() => component.getAllByRole('menuitem')) - fireEvent.click(menuItems[3]) // menu should still be open expect(component.getByRole('menu')).toBeInTheDocument() @@ -109,14 +122,16 @@ describe('ActionMenu', () => { ) const button = component.getByLabelText('Field type') - fireEvent.click(button) + + const user = userEvent.setup() + await user.click(button) // select first item by role, that would close the menu - fireEvent.click(component.getAllByRole('menuitemradio')[0]) + await user.click(component.getAllByRole('menuitemradio')[0]) expect(component.queryByRole('menu')).not.toBeInTheDocument() // open menu again and check if the first option is checked - fireEvent.click(button) + await user.click(button) expect(component.getAllByRole('menuitemradio')[0]).toHaveAttribute('aria-checked', 'true') cleanup() }) @@ -128,7 +143,9 @@ describe('ActionMenu', () => { ) const button = component.getByLabelText('Group by') - fireEvent.click(button) + + const user = userEvent.setup() + await user.click(button) expect(component.getByLabelText('Status')).toHaveAttribute('role', 'menuitemradio') expect(component.getByLabelText('Clear Group by')).toHaveAttribute('role', 'menuitem') @@ -136,6 +153,82 @@ describe('ActionMenu', () => { cleanup() }) + it('should keep focus on Button when menu is opened with click', async () => { + const component = HTMLRender() + const button = component.getByRole('button') + + const user = userEvent.setup() + await user.tab() // tab into the story, this should focus on the first button + expect(button).toEqual(document.activeElement) // trust, but verify + + await user.click(button) + expect(component.queryByRole('menu')).toBeInTheDocument() + 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.getByRole('button') + + const user = userEvent.setup() + await user.click(button) + + expect(component.queryByRole('menu')).toBeInTheDocument() + + // assumes button is the active element at this point + await user.keyboard('{ArrowDown}') + + 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.getByRole('button') + + const user = userEvent.setup() + await user.click(button) + + expect(component.queryByRole('menu')).toBeInTheDocument() + + // button should be the active element + // assumes button is the active element at this point + await user.keyboard('{ArrowUp}') + + 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') + + const user = userEvent.setup() + await user.tab() // tab into the story, this should focus on the first button + expect(anchor).toEqual(document.activeElement) // trust, but verify + + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(component.getAllByRole('menuitem')[0]).toEqual(document.activeElement) + + // TODO: revisit why we need this waitFor + await waitFor(async () => { + await user.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 bc077603ddf..009698a94e4 100644 --- a/src/__tests__/hooks/useMenuInitialFocus.test.tsx +++ b/src/__tests__/hooks/useMenuInitialFocus.test.tsx @@ -7,11 +7,12 @@ const Component = () => { const onOpen = () => setOpen(!open) const containerRef = React.createRef() - const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef) + const anchorRef = React.createRef() + useMenuInitialFocus(open, containerRef, anchorRef) return ( <> - {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() + + 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 47ec8e00a6f..a12912fc1c8 100644 --- a/src/hooks/index.ts +++ b/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' diff --git a/src/hooks/useMenuInitialFocus.ts b/src/hooks/useMenuInitialFocus.ts index e742e6b49fc..13c9551f0cf 100644 --- a/src/hooks/useMenuInitialFocus.ts +++ b/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) => unknown +export const useMenuInitialFocus = ( + open: boolean, + containerRef: React.RefObject, + anchorRef: React.RefObject +) => { + /** + * 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(undefined) + + React.useEffect( + function inferOpeningKey() { + const anchorElement = anchorRef.current -export const useMenuInitialFocus = (open: boolean, onOpen?: Callback, providedRef?: React.RefObject) => { - const containerRef = useProvidedRefOrCreate(providedRef) - const [openingKey, setOpeningKey] = React.useState(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] + ) } diff --git a/src/hooks/useMenuKeyboardNavigation.ts b/src/hooks/useMenuKeyboardNavigation.ts new file mode 100644 index 00000000000..be957bc28df --- /dev/null +++ b/src/hooks/useMenuKeyboardNavigation.ts @@ -0,0 +1,89 @@ +import React from 'react' +import {iterateFocusableElements} from '@primer/behaviors/utils' +import {useMenuInitialFocus} from './useMenuInitialFocus' +import {useMnemonics} from './useMnemonics' +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, + onClose: MenuContextProps['onClose'], + containerRef: React.RefObject, + anchorRef: React.RefObject +) => { + useMenuInitialFocus(open, containerRef, anchorRef) + useMnemonics(open, containerRef) + useCloseMenuOnTab(open, onClose, containerRef, anchorRef) + useMoveFocusToMenuItem(open, containerRef, anchorRef) +} + +/** + * 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') { + // TODO: does commenting this out break anything? + // event.preventDefault() // prevent scroll event + 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') { + // TODO: does commenting this out break anything? + // event.preventDefault() // prevent scroll event + 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/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index b8819ce4e90..39a7379e43c 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -32,7 +32,8 @@ import { NumberIcon, SingleSelectIcon, TypographyIcon, - IconProps + IconProps, + IssueOpenedIcon } from '@primer/octicons-react' const meta: Meta = { @@ -107,7 +108,16 @@ export function ExternalAnchor(): JSX.Element {

External Open State: {open ? 'Open' : 'Closed'}

Last option activated: {actionFired}

-
@@ -240,15 +250,21 @@ export function CustomAnchor(): JSX.Element { export function MemexTableMenu(): JSX.Element { const [name, setName] = React.useState('Estimate') + const [wipName, setWipName] = React.useState(name) const inputRef = React.createRef() /** To add custom components to the Menu, * you need to switch to a controlled menu */ const [open, setOpen] = React.useState(false) + + const handleChange = (event: React.ChangeEvent) => { + setWipName(event.currentTarget.value) + } + const handleKeyPress = (event: React.KeyboardEvent) => { if (event.key === 'Enter') { - setName(event.currentTarget.value) + setName(wipName) setOpen(false) } } @@ -257,7 +273,7 @@ export function MemexTableMenu(): JSX.Element { * on the input, it doesn't work :( */ const handleClickOutside = () => { - if (inputRef.current) setName(inputRef.current.value) + if (inputRef.current) setWipName(inputRef.current.value) setOpen(false) } @@ -275,13 +291,14 @@ export function MemexTableMenu(): JSX.Element { }} > {name} + - + @@ -610,6 +627,57 @@ export function MemexAddColumn(): JSX.Element { ) } +export function MemexKeyboardShortcut(): JSX.Element { + const [open, setOpen] = React.useState(false) + const anchorRef = React.useRef(null) + + React.useEffect(() => { + const handler = (event: KeyboardEvent) => { + if (event.code === 'Backslash' && event.shiftKey) setOpen(true) + } + + window.addEventListener('keydown', handler) + return () => window.removeEventListener('keydown', handler) + }) + + return ( + <> +

Memex Keyboard Shortcut

+

The menu can also be opened with a keyboard shortcut - `Shift + \`

+ + setOpen(!open)} + icon={TriangleDownIcon} + aria-label="Open Estimate column options menu" + sx={{padding: 0}} + /> + + + + + Archive + Delete from project + + + + 1 + + Produce ag-Grid staging demo + + + ) +} + export function OverlayProps(): JSX.Element { const [open, setOpen] = React.useState(false) const inputRef = React.createRef() @@ -696,3 +764,27 @@ export function MnemonicsTest(): JSX.Element { ) } + +export function TabTest(): JSX.Element { + return ( + <> +

Story to test Tab

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