From 2436b762e64f42552dd584dfc8740af8eb94666f Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 14 Apr 2022 12:24:26 +0200 Subject: [PATCH 01/12] Revert "Revert "ActionMenu: Remove focus trap (#1984)" (#2023)" This reverts commit 866abc0da1d627a77c726b023aece616dd65eaca. --- .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, 266 insertions(+), 23 deletions(-) create mode 100644 .changeset/actionmenu-remove-focus-trap.md create mode 100644 src/hooks/useMenuKeyboardNavigation.ts diff --git a/.changeset/actionmenu-remove-focus-trap.md b/.changeset/actionmenu-remove-focus-trap.md new file mode 100644 index 00000000000..d43f6668bf0 --- /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 diff --git a/src/ActionMenu.tsx b/src/ActionMenu.tsx index fa07408c99e..6f1a6574d35 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, 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({renderAnchor: null, open: false}) export type ActionMenuProps = { @@ -111,8 +113,7 @@ const Overlay: React.FC = ({children, align = 'start', ...over > const containerRef = React.createRef() - const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef) - useTypeaheadFocus(open, containerRef) + const {openWithFocus} = useMenuKeyboardNavigation(open, onOpen, onClose, containerRef, anchorRef) return ( = ({children, align = 'start', ...over align={align} overlayProps={overlayProps} focusZoneSettings={{focusOutBehavior: 'wrap'}} + focusTrapSettings={{disabled: true}} >
{ const component = HTMLRender() 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() + 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 bc077603ddf..034f4d99bb9 100644 --- a/src/__tests__/hooks/useMenuInitialFocus.test.tsx +++ b/src/__tests__/hooks/useMenuInitialFocus.test.tsx @@ -7,11 +7,16 @@ const Component = () => { const onOpen = () => setOpen(!open) const containerRef = React.createRef() - const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef) + const anchorRef = React.createRef() + const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef, anchorRef) return ( <> - {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() + + 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 a6dd3f13016..5538e41402d 100644 --- a/src/hooks/index.ts +++ b/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' diff --git a/src/hooks/useMenuInitialFocus.ts b/src/hooks/useMenuInitialFocus.ts index e742e6b49fc..a5db601efac 100644 --- a/src/hooks/useMenuInitialFocus.ts +++ b/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) => unknown -export const useMenuInitialFocus = (open: boolean, onOpen?: Callback, providedRef?: React.RefObject) => { - const containerRef = useProvidedRefOrCreate(providedRef) +export const useMenuInitialFocus = ( + open: boolean, + onOpen?: Callback, + providedContainerRef?: React.RefObject, + providedAnchorRef?: React.RefObject +) => { + const containerRef = useProvidedRefOrCreate(providedContainerRef) + const anchorRef = useProvidedRefOrCreate(providedAnchorRef) 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 */ @@ -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} } diff --git a/src/hooks/useMenuKeyboardNavigation.ts b/src/hooks/useMenuKeyboardNavigation.ts new file mode 100644 index 00000000000..2a52472d5d2 --- /dev/null +++ b/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, + 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 7cc20ab701f..0e894621a64 100644 --- a/src/hooks/useTypeaheadFocus.ts +++ b/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) => { - const containerRef = useProvidedRefOrCreate(providedRef) +export const useTypeaheadFocus = ( + open: boolean, + providedContainerRef: React.RefObject, + providedAnchorRef?: React.RefObject +) => { + 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 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} } diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 8e74c3677f9..2f56c87fe40 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -678,3 +678,27 @@ 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 + + + + + + + ) +} From 928a7a6acc04a9baab8804354c8ad6fec32b06c9 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 21 Jun 2022 17:03:19 +0200 Subject: [PATCH 02/12] prevent scroll when moving from anchor to menu --- src/hooks/useMenuKeyboardNavigation.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/hooks/useMenuKeyboardNavigation.ts b/src/hooks/useMenuKeyboardNavigation.ts index 2a52472d5d2..e0390814020 100644 --- a/src/hooks/useMenuKeyboardNavigation.ts +++ b/src/hooks/useMenuKeyboardNavigation.ts @@ -72,10 +72,12 @@ const useMoveFocusToMenuItem = ( const iterable = iterateFocusableElements(container) if (event.key === 'ArrowDown') { + 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') { + event.preventDefault() // prevent scroll event const elements = [...iterable] const lastElement = elements[elements.length - 1] setTimeout(() => lastElement.focus()) From 705d684744d538f0b68148747a83095e026f6134 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 21 Jun 2022 17:46:00 +0200 Subject: [PATCH 03/12] Add shortcut example from memex --- src/stories/ActionMenu/fixtures.stories.tsx | 57 ++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 2f56c87fe40..0ef3c228fc6 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 = { @@ -610,6 +611,60 @@ 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) + anchorRef.current?.focus() // move focus to anchor button + } + } + + window.addEventListener('keydown', handler) + return () => window.removeEventListener('keydown', handler) + }) + + return ( + <> +

Memex Table Menu

+

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() From f5c0577e96623f4f983dd77482d424508a69c9f7 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 20 Jul 2022 12:39:44 +0200 Subject: [PATCH 04/12] progress! --- src/ActionMenu.tsx | 4 +- .../hooks/useMenuInitialFocus.test.tsx | 8 +- src/hooks/useMenuInitialFocus.ts | 78 +++++++++++-------- src/hooks/useMenuKeyboardNavigation.ts | 7 +- src/stories/ActionMenu/fixtures.stories.tsx | 9 ++- 5 files changed, 61 insertions(+), 45 deletions(-) diff --git a/src/ActionMenu.tsx b/src/ActionMenu.tsx index 6f1a6574d35..8e8f6817aef 100644 --- a/src/ActionMenu.tsx +++ b/src/ActionMenu.tsx @@ -113,7 +113,7 @@ const Overlay: React.FC = ({children, align = 'start', ...over > const containerRef = React.createRef() - const {openWithFocus} = useMenuKeyboardNavigation(open, onOpen, onClose, containerRef, anchorRef) + useMenuKeyboardNavigation(open, onClose, containerRef, anchorRef) return ( = ({children, align = 'start', ...over renderAnchor={renderAnchor} anchorId={anchorId} open={open} - onOpen={openWithFocus} + onOpen={onOpen} onClose={onClose} align={align} overlayProps={overlayProps} diff --git a/src/__tests__/hooks/useMenuInitialFocus.test.tsx b/src/__tests__/hooks/useMenuInitialFocus.test.tsx index 034f4d99bb9..009698a94e4 100644 --- a/src/__tests__/hooks/useMenuInitialFocus.test.tsx +++ b/src/__tests__/hooks/useMenuInitialFocus.test.tsx @@ -8,15 +8,11 @@ const Component = () => { const containerRef = React.createRef() const anchorRef = React.createRef() - const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef, anchorRef) + useMenuInitialFocus(open, containerRef, anchorRef) return ( <> - {open && ( diff --git a/src/hooks/useMenuInitialFocus.ts b/src/hooks/useMenuInitialFocus.ts index a5db601efac..f0616ef883c 100644 --- a/src/hooks/useMenuInitialFocus.ts +++ b/src/hooks/useMenuInitialFocus.ts @@ -2,24 +2,39 @@ 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, - onOpen?: Callback, providedContainerRef?: React.RefObject, providedAnchorRef?: React.RefObject ) => { const containerRef = useProvidedRefOrCreate(providedContainerRef) const anchorRef = useProvidedRefOrCreate(providedAnchorRef) - 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) - if (typeof onOpen === 'function') onOpen(gesture, event) - } + /** + * 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 clickHandler = () => setOpeningGesture('mouse-click') + const keydownHandler = (event: KeyboardEvent) => { + if (['ArrowDown', 'ArrowUp', 'Space', 'Enter'].includes(event.code)) setOpeningGesture(event.key) + } + + const anchorElement = anchorRef.current + + 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 @@ -27,25 +42,26 @@ export const useMenuInitialFocus = ( * ArrowDown | Space | Enter: first element * ArrowUp: last element */ - React.useEffect(() => { - if (!open) return - 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)) { - 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, anchorRef]) - - return {containerRef, anchorRef, openWithFocus} + React.useEffect( + function moveFocusOnOpen() { + if (!open) return + if (!openingGesture || !containerRef.current) return + + const iterable = iterateFocusableElements(containerRef.current) + + if (openingGesture === 'mouse-click') { + if (anchorRef.current) anchorRef.current.focus() + else throw new Error('For focus management, please attach anchorRef') + } else if (['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()) + } + }, + [open, openingGesture, containerRef, anchorRef] + ) } diff --git a/src/hooks/useMenuKeyboardNavigation.ts b/src/hooks/useMenuKeyboardNavigation.ts index e0390814020..92c4ea4dfa5 100644 --- a/src/hooks/useMenuKeyboardNavigation.ts +++ b/src/hooks/useMenuKeyboardNavigation.ts @@ -13,17 +13,16 @@ import {MenuContextProps} from '../ActionMenu' */ export const useMenuKeyboardNavigation = ( open: boolean, - onOpen: MenuContextProps['onOpen'], onClose: MenuContextProps['onClose'], containerRef: React.RefObject, anchorRef: React.RefObject ) => { - const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef, anchorRef) + useMenuInitialFocus(open, containerRef, anchorRef) useTypeaheadFocus(open, containerRef, anchorRef) useCloseMenuOnTab(open, onClose, containerRef, anchorRef) - useMoveFocusToMenuItem(open, containerRef, anchorRef) - return {openWithFocus} + // TODO: menu only opens once, this is the problem + useMoveFocusToMenuItem(open, containerRef, anchorRef) } /** diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 0ef3c228fc6..0c2a1488a77 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -108,7 +108,12 @@ export function ExternalAnchor(): JSX.Element {

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

Last option activated: {actionFired}

-
@@ -629,7 +634,7 @@ export function MemexKeyboardShortcut(): JSX.Element { return ( <> -

Memex Table Menu

+

Memex Keyboard Shortcut

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

Date: Wed, 20 Jul 2022 12:58:19 +0200 Subject: [PATCH 05/12] make refs required --- src/hooks/useMenuInitialFocus.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/hooks/useMenuInitialFocus.ts b/src/hooks/useMenuInitialFocus.ts index f0616ef883c..e17db315de5 100644 --- a/src/hooks/useMenuInitialFocus.ts +++ b/src/hooks/useMenuInitialFocus.ts @@ -1,15 +1,11 @@ import React from 'react' import {iterateFocusableElements} from '@primer/behaviors/utils' -import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' export const useMenuInitialFocus = ( open: boolean, - providedContainerRef?: React.RefObject, - providedAnchorRef?: React.RefObject + containerRef: React.RefObject, + anchorRef: React.RefObject ) => { - const containerRef = useProvidedRefOrCreate(providedContainerRef) - const anchorRef = useProvidedRefOrCreate(providedAnchorRef) - /** * 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. @@ -19,13 +15,13 @@ export const useMenuInitialFocus = ( React.useEffect( function inferOpeningKey() { + const anchorElement = anchorRef.current + const clickHandler = () => setOpeningGesture('mouse-click') const keydownHandler = (event: KeyboardEvent) => { if (['ArrowDown', 'ArrowUp', 'Space', 'Enter'].includes(event.code)) setOpeningGesture(event.key) } - const anchorElement = anchorRef.current - anchorElement?.addEventListener('click', clickHandler) anchorElement?.addEventListener('keydown', keydownHandler) return () => { From 1e515f1a7845e1ec335ecc1538daf665c1272a2b Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 20 Jul 2022 14:10:27 +0200 Subject: [PATCH 06/12] onclick should not interfere with keyboard --- src/hooks/useMenuInitialFocus.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/hooks/useMenuInitialFocus.ts b/src/hooks/useMenuInitialFocus.ts index e17db315de5..4285d2efafc 100644 --- a/src/hooks/useMenuInitialFocus.ts +++ b/src/hooks/useMenuInitialFocus.ts @@ -17,9 +17,15 @@ export const useMenuInitialFocus = ( function inferOpeningKey() { const anchorElement = anchorRef.current - const clickHandler = () => setOpeningGesture('mouse-click') + 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 (['ArrowDown', 'ArrowUp', 'Space', 'Enter'].includes(event.code)) setOpeningGesture(event.key) + if (['Space', 'Enter', 'ArrowDown', 'ArrowUp'].includes(event.code)) { + setOpeningGesture(event.code) + } } anchorElement?.addEventListener('click', clickHandler) From 5eea443d2bb2a857a008905dc6cba3275972c400 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 20 Jul 2022 14:49:38 +0200 Subject: [PATCH 07/12] default to first item if anchor is not used --- src/hooks/useMenuInitialFocus.ts | 11 ++++++++--- src/hooks/useMenuKeyboardNavigation.ts | 8 ++++---- src/stories/ActionMenu/fixtures.stories.tsx | 13 +++++++------ 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/hooks/useMenuInitialFocus.ts b/src/hooks/useMenuInitialFocus.ts index 4285d2efafc..0b25703ac41 100644 --- a/src/hooks/useMenuInitialFocus.ts +++ b/src/hooks/useMenuInitialFocus.ts @@ -46,15 +46,14 @@ export const useMenuInitialFocus = ( */ React.useEffect( function moveFocusOnOpen() { - if (!open) return - if (!openingGesture || !containerRef.current) return + if (!open || !containerRef.current) return // wait till the menu is open const iterable = iterateFocusableElements(containerRef.current) if (openingGesture === 'mouse-click') { if (anchorRef.current) anchorRef.current.focus() else throw new Error('For focus management, please attach anchorRef') - } else if (['ArrowDown', 'Space', 'Enter'].includes(openingGesture)) { + } 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()) @@ -62,6 +61,12 @@ export const useMenuInitialFocus = ( 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()) } }, [open, openingGesture, containerRef, anchorRef] diff --git a/src/hooks/useMenuKeyboardNavigation.ts b/src/hooks/useMenuKeyboardNavigation.ts index 92c4ea4dfa5..10dd6dbb584 100644 --- a/src/hooks/useMenuKeyboardNavigation.ts +++ b/src/hooks/useMenuKeyboardNavigation.ts @@ -20,8 +20,6 @@ export const useMenuKeyboardNavigation = ( useMenuInitialFocus(open, containerRef, anchorRef) useTypeaheadFocus(open, containerRef, anchorRef) useCloseMenuOnTab(open, onClose, containerRef, anchorRef) - - // TODO: menu only opens once, this is the problem useMoveFocusToMenuItem(open, containerRef, anchorRef) } @@ -71,12 +69,14 @@ const useMoveFocusToMenuItem = ( const iterable = iterateFocusableElements(container) if (event.key === 'ArrowDown') { - event.preventDefault() // prevent scroll event + // 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') { - event.preventDefault() // prevent scroll event + // TODO: does commenting this out break anything? + // event.preventDefault() // prevent scroll event const elements = [...iterable] const lastElement = elements[elements.length - 1] setTimeout(() => lastElement.focus()) diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 0c2a1488a77..6a4c230df97 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -111,8 +111,12 @@ export function ExternalAnchor(): JSX.Element { @@ -622,10 +626,7 @@ export function MemexKeyboardShortcut(): JSX.Element { React.useEffect(() => { const handler = (event: KeyboardEvent) => { - if (event.code === 'Backslash' && event.shiftKey) { - setOpen(true) - anchorRef.current?.focus() // move focus to anchor button - } + if (event.code === 'Backslash' && event.shiftKey) setOpen(true) } window.addEventListener('keydown', handler) From 1ad96819abeb8fdae007210bf101e770fe7ef76f Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 20 Jul 2022 15:00:46 +0200 Subject: [PATCH 08/12] use userEvent instead of fireEvent for test to get full event --- src/__tests__/ActionMenu.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/ActionMenu.test.tsx b/src/__tests__/ActionMenu.test.tsx index 01324e6a99e..6214d1f5b61 100644 --- a/src/__tests__/ActionMenu.test.tsx +++ b/src/__tests__/ActionMenu.test.tsx @@ -145,7 +145,7 @@ describe('ActionMenu', () => { userEvent.tab() // tab into the story, this should focus on the first button expect(button).toEqual(document.activeElement) // trust, but verify - fireEvent.click(button) + userEvent.click(button) expect(component.queryByRole('menu')).toBeInTheDocument() /** We use waitFor because the hook uses an effect with setTimeout From 884cde80f494659e2c3511be504de71e4841bf40 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 20 Jul 2022 15:08:31 +0200 Subject: [PATCH 09/12] remedy merge --- src/hooks/useMenuKeyboardNavigation.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/hooks/useMenuKeyboardNavigation.ts b/src/hooks/useMenuKeyboardNavigation.ts index f334be0cdea..be957bc28df 100644 --- a/src/hooks/useMenuKeyboardNavigation.ts +++ b/src/hooks/useMenuKeyboardNavigation.ts @@ -1,7 +1,6 @@ import React from 'react' import {iterateFocusableElements} from '@primer/behaviors/utils' import {useMenuInitialFocus} from './useMenuInitialFocus' -import {useTypeaheadFocus} from './useTypeaheadFocus' import {useMnemonics} from './useMnemonics' import {MenuContextProps} from '../ActionMenu' @@ -19,10 +18,9 @@ export const useMenuKeyboardNavigation = ( anchorRef: React.RefObject ) => { useMenuInitialFocus(open, containerRef, anchorRef) - useTypeaheadFocus(open, containerRef, anchorRef) + useMnemonics(open, containerRef) useCloseMenuOnTab(open, onClose, containerRef, anchorRef) useMoveFocusToMenuItem(open, containerRef, anchorRef) - useMnemonics(open, containerRef) } /** From a1d9b7cb83754902c1964830cb560204bd834aea Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 27 Jul 2022 22:52:01 +0200 Subject: [PATCH 10/12] ignore containerRef for initial focus --- src/hooks/useMenuInitialFocus.ts | 5 ++++- src/stories/ActionMenu/fixtures.stories.tsx | 13 ++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/hooks/useMenuInitialFocus.ts b/src/hooks/useMenuInitialFocus.ts index 0b25703ac41..13c9551f0cf 100644 --- a/src/hooks/useMenuInitialFocus.ts +++ b/src/hooks/useMenuInitialFocus.ts @@ -69,6 +69,9 @@ export const useMenuInitialFocus = ( setTimeout(() => firstElement?.focus()) } }, - [open, openingGesture, containerRef, anchorRef] + // 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/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 5aad794cdab..a7c0a95a029 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -250,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) } } @@ -267,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) } @@ -285,13 +291,14 @@ export function MemexTableMenu(): JSX.Element { }} > {name} + - + From 709e16476b76dbadd474e0c5055b214fe5027820 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 28 Jul 2022 14:16:17 +0200 Subject: [PATCH 11/12] update changeset to include initial focus on click --- .changeset/actionmenu-remove-focus-trap.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/actionmenu-remove-focus-trap.md b/.changeset/actionmenu-remove-focus-trap.md index d43f6668bf0..92d765283b0 100644 --- a/.changeset/actionmenu-remove-focus-trap.md +++ b/.changeset/actionmenu-remove-focus-trap.md @@ -2,4 +2,4 @@ '@primer/react': patch --- -ActionMenu: Remove focus trap to enable Tab and Shift+Tab behavior +ActionMenu: Remove focus trap to enable Tab and Shift+Tab behavior and fix initial focus on click From a2d0cb6f8505d70bf718fc732119f7fc49fddac1 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 1 Aug 2022 15:30:12 +0200 Subject: [PATCH 12/12] migrate to user-event@14 --- src/__tests__/ActionMenu.test.tsx | 119 ++++++++++++++++-------------- 1 file changed, 65 insertions(+), 54 deletions(-) diff --git a/src/__tests__/ActionMenu.test.tsx b/src/__tests__/ActionMenu.test.tsx index 7c9938ba27e..a28befbfc7d 100644 --- a/src/__tests__/ActionMenu.test.tsx +++ b/src/__tests__/ActionMenu.test.tsx @@ -1,4 +1,4 @@ -import {cleanup, render as HTMLRender, waitFor, fireEvent} from '@testing-library/react' +import {cleanup, render as HTMLRender, waitFor} from '@testing-library/react' import userEvent from '@testing-library/user-event' import 'babel-polyfill' import {axe, toHaveNoViolations} from 'jest-axe' @@ -49,55 +49,66 @@ describe('ActionMenu', () => { 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}') - fireEvent.keyDown(button, {key: 'Enter'}) 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')) + const user = userEvent.setup() + await user.click(button) - // 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() + 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() @@ -111,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() }) @@ -130,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') @@ -142,53 +157,48 @@ describe('ActionMenu', () => { const component = HTMLRender() const button = component.getByRole('button') - userEvent.tab() // tab into the story, this should focus on the first 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 - userEvent.click(button) + await user.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)) + 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 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() + const user = userEvent.setup() + await user.click(button) - // button should be the active element - fireEvent.keyDown(document.activeElement!, {key: 'ArrowDown', code: 'ArrowDown'}) + expect(component.queryByRole('menu')).toBeInTheDocument() - await waitFor(() => { - expect(component.getAllByRole('menuitem')[0]).toEqual(document.activeElement) - }) + // 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.getByText('Toggle Menu') - button.focus() // browsers do this automatically on click, but tests don't - fireEvent.click(button) + const button = component.getByRole('button') + + const user = userEvent.setup() + await user.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) - }) + // assumes button is the active element at this point + await user.keyboard('{ArrowUp}') + expect(component.getAllByRole('menuitem').pop()).toEqual(document.activeElement) cleanup() }) @@ -201,16 +211,17 @@ describe('ActionMenu', () => { ) const anchor = component.getByRole('button') - userEvent.tab() // tab into the story, this should focus on the first 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 - fireEvent.keyDown(anchor, {key: 'Enter'}) + await user.keyboard('{Enter}') expect(component.queryByRole('menu')).toBeInTheDocument() - expect(component.getAllByRole('menuitem')[0]).toEqual(document.activeElement) - await waitFor(() => { - userEvent.tab() + // 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() })