diff --git a/.changeset/quiet-action-menu-labels.md b/.changeset/quiet-action-menu-labels.md new file mode 100644 index 00000000000..8729591500a --- /dev/null +++ b/.changeset/quiet-action-menu-labels.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +ActionMenu: Avoid an extra render when closed menus mount while preserving accessible trigger labeling diff --git a/packages/react/src/ActionMenu/ActionMenu.test.tsx b/packages/react/src/ActionMenu/ActionMenu.test.tsx index bec8b1b18a3..18869971c6f 100644 --- a/packages/react/src/ActionMenu/ActionMenu.test.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.test.tsx @@ -1,6 +1,7 @@ import {describe, expect, it, vi, beforeEach} from 'vitest' import {render as HTMLRender, waitFor, act, within} from '@testing-library/react' import userEvent from '@testing-library/user-event' +import {page, userEvent as browserUserEvent} from 'vitest/browser' import type React from 'react' import {useRef, useState} from 'react' import BaseStyles from '../BaseStyles' @@ -18,6 +19,7 @@ import type {AnchorPosition} from '@primer/behaviors' import type {JSX} from 'react' import {implementsClassName} from '../utils/testing' +import {createRenderCounter} from '../utils/testing/profiler' import {FeatureFlags} from '../FeatureFlags' // Mock getAnchoredPosition for feature flag tests @@ -82,6 +84,30 @@ function ExampleWithTooltip(): JSX.Element { ) } +function IconButtonActionMenu({ + onSelect, + onFocus, + ...props +}: {onSelect?: () => void; onFocus?: React.FocusEventHandler} & Pick< + React.ComponentProps, + 'open' | 'onOpenChange' +> = {}): JSX.Element { + return ( + + + + + + + + Copy link + + + + + ) +} + // eslint-disable-next-line @typescript-eslint/no-explicit-any function ExampleWithTooltipV2(actionMenuTrigger: React.ReactElement): JSX.Element { return ( @@ -663,30 +689,113 @@ describe('ActionMenu', () => { expect(button.id).toBe(buttonId) }) - it('should use the tooltip id to name the menu when the anchor is icon button', async () => { - const component = HTMLRender( - - - - - + describe('anchor labeling and focus behavior', () => { + it('does not publish anchor label state after mounting closed', async () => { + const [Wrap, counter] = createRenderCounter() + HTMLRender( + + + + + , + ) - - - alert('Copy link clicked')}> - Copy link - ⌘C - - - - - , - ) + await act(async () => {}) + + expect(counter.mountCount).toBe(1) + expect(counter.updateCount).toBe(0) + }) - const toggleButton = component.getByRole('button', {name: 'More actions'}) - await userEvent.click(toggleButton) - expect(toggleButton).toHaveAttribute('aria-labelledby') - expect(component.getByRole('menu')).toHaveAttribute('aria-labelledby', toggleButton.getAttribute('aria-labelledby')) + it('labels an initially open controlled menu before item focus', async () => { + const onFocus = vi.fn(() => within(document.body).queryByRole('menu', {name: 'More actions'}) !== null) + const component = HTMLRender( + + + + + , + ) + + await expect.element(page.getByRole('menuitem', {name: 'Copy link', exact: true})).toHaveFocus() + expect(onFocus.mock.results[0].value).toBe(true) + const menu = component.getByRole('menu', {name: 'More actions'}) + const anchor = component.getByRole('button', {name: 'More actions'}) + expect(menu).toHaveAttribute('aria-labelledby', anchor.getAttribute('aria-labelledby')) + }) + + it('uses the tooltip id to name the menu before first item focus', async () => { + const onFocus = vi.fn(() => within(document.body).queryByRole('menu', {name: 'More actions'}) !== null) + const component = HTMLRender( + + + + + , + ) + const toggleButton = component.getByRole('button', {name: 'More actions'}) + + await browserUserEvent.click(page.getByRole('button', {name: 'More actions', exact: true})) + + await expect.element(page.getByRole('menuitem', {name: 'Copy link', exact: true})).toHaveFocus() + expect(onFocus.mock.results[0].value).toBe(true) + expect(toggleButton).toHaveAttribute('aria-labelledby') + expect(component.getByRole('menu', {name: 'More actions'})).toHaveAttribute( + 'aria-labelledby', + toggleButton.getAttribute('aria-labelledby'), + ) + }) + + it('closes with Escape and returns focus to the anchor', async () => { + const component = HTMLRender( + + + , + ) + const user = userEvent.setup() + const toggleButton = component.getByRole('button', {name: 'More actions'}) + await user.click(toggleButton) + component.getByRole('menuitem', {name: 'Copy link'}).focus() + + await user.keyboard('{Escape}') + + expect(component.queryByRole('menu')).not.toBeInTheDocument() + expect(toggleButton).toHaveFocus() + }) + + it('closes with Tab without returning focus to the anchor', async () => { + const component = HTMLRender( + + + , + ) + const user = userEvent.setup() + const toggleButton = component.getByRole('button', {name: 'More actions'}) + await user.click(toggleButton) + component.getByRole('menuitem', {name: 'Copy link'}).focus() + + await user.tab() + + expect(component.queryByRole('menu')).not.toBeInTheDocument() + expect(toggleButton).not.toHaveFocus() + }) + + it('closes after selection and returns focus to the anchor', async () => { + const onSelect = vi.fn() + const component = HTMLRender( + + + , + ) + const user = userEvent.setup() + const toggleButton = component.getByRole('button', {name: 'More actions'}) + await user.click(toggleButton) + + await user.click(component.getByRole('menuitem', {name: 'Copy link'})) + + expect(onSelect).toHaveBeenCalledOnce() + expect(component.queryByRole('menu')).not.toBeInTheDocument() + expect(toggleButton).toHaveFocus() + }) }) describe('submenus', () => { diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index 63c5c1f848a..c33997690f5 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -1,4 +1,4 @@ -import React, {useCallback, useContext, useMemo, useEffect, useState} from 'react' +import React, {useCallback, useContext, useMemo, useState} from 'react' import {clsx} from 'clsx' import {TriangleDownIcon, ChevronRightIcon} from '@primer/octicons-react' import type {AnchoredOverlayProps} from '../AnchoredOverlay' @@ -19,6 +19,7 @@ import {useResponsiveValue, type ResponsiveValue} from '../hooks/useResponsiveVa import {isSlot} from '../utils/is-slot' import type {FCWithSlotMarker, WithSlotMarker} from '../utils/types/Slots' import {DialogContext} from '../Dialog/DialogContext' +import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' export type MenuCloseHandler = ( gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab' | 'item-select' | 'arrow-left' | 'close', @@ -312,22 +313,23 @@ const Overlay: FCWithSlotMarker> = ({ [isNarrowFullscreen, onClose], ) - useMenuKeyboardNavigation(open, handleClose, containerRef, anchorRef, isSubmenu) + // Wait for the anchor label before mounting or focusing the menu. + const [anchorLabelReady, setAnchorLabelReady] = useState(false) + if (!open && anchorLabelReady) { + setAnchorLabelReady(false) + } + useMenuKeyboardNavigation(open && anchorLabelReady, handleClose, containerRef, anchorRef, isSubmenu) const responsiveVariant = useResponsiveValue(variant, {regular: 'anchored', narrow: 'anchored'}) // If the menu anchor is an icon button, we need to label the menu by tooltip that also labelled the anchor. const [anchorAriaLabelledby, setAnchorAriaLabelledby] = useState(null) - useEffect(() => { - // Necessary for HMR reloads - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - if (anchorRef?.current) { - const ariaLabelledby = anchorRef.current.getAttribute('aria-labelledby') - if (ariaLabelledby) { - setAnchorAriaLabelledby(ariaLabelledby) - } + useLayoutEffect(() => { + if (open) { + setAnchorAriaLabelledby(anchorRef.current?.getAttribute('aria-labelledby') ?? null) + setAnchorLabelReady(true) } - }, [anchorRef]) + }, [anchorRef, open]) const afterSelect = useCallback(() => onClose?.('item-select'), [onClose]) @@ -350,7 +352,7 @@ const Overlay: FCWithSlotMarker> = ({ anchorRef={anchorRef} renderAnchor={renderAnchor} anchorId={anchorId} - open={open} + open={open && anchorLabelReady} onOpen={onOpen} onClose={handleClose} align={align}