From 610299df803f4f58f689b99f12ef45fadf55c923 Mon Sep 17 00:00:00 2001 From: Alexander Lelidis Date: Thu, 22 Oct 2026 08:29:57 -0700 Subject: [PATCH 1/4] Fix ActionMenu anchor label updates Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .changeset/quiet-action-menu-labels.md | 5 + .../react/src/ActionMenu/ActionMenu.test.tsx | 122 ++++++++++++++---- packages/react/src/ActionMenu/ActionMenu.tsx | 21 ++- 3 files changed, 114 insertions(+), 34 deletions(-) create mode 100644 .changeset/quiet-action-menu-labels.md 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..a3210eab91c 100644 --- a/packages/react/src/ActionMenu/ActionMenu.test.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.test.tsx @@ -18,6 +18,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 +83,21 @@ function ExampleWithTooltip(): JSX.Element { ) } +function IconButtonActionMenu({onSelect}: {onSelect?: () => void} = {}): JSX.Element { + return ( + + + + + + + Copy link + + + + ) +} + // eslint-disable-next-line @typescript-eslint/no-explicit-any function ExampleWithTooltipV2(actionMenuTrigger: React.ReactElement): JSX.Element { return ( @@ -663,30 +679,92 @@ 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) + }) + + it('uses the tooltip id to name the menu on first open', async () => { + const component = HTMLRender( + + + , + ) + const user = userEvent.setup() + const toggleButton = component.getByRole('button', {name: 'More actions'}) + + await user.click(toggleButton) + + 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}') - 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')) + 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..ac6d4961b69 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' @@ -318,16 +318,13 @@ const Overlay: FCWithSlotMarker> = ({ // 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) - } - } - }, [anchorRef]) + const handleOpen = useCallback>( + (gesture, event) => { + setAnchorAriaLabelledby(anchorRef.current?.getAttribute('aria-labelledby') ?? null) + onOpen?.(gesture, event) + }, + [anchorRef, onOpen], + ) const afterSelect = useCallback(() => onClose?.('item-select'), [onClose]) @@ -351,7 +348,7 @@ const Overlay: FCWithSlotMarker> = ({ renderAnchor={renderAnchor} anchorId={anchorId} open={open} - onOpen={onOpen} + onOpen={handleOpen} onClose={handleClose} align={align} side={side ?? (isSubmenu ? 'outside-right' : 'outside-bottom')} From e6416ac07a4d521944a3e015542df668b5502e56 Mon Sep 17 00:00:00 2001 From: Alexander Lelidis Date: Fri, 23 Oct 2026 10:31:40 +0000 Subject: [PATCH 2/4] Preserve accessible names when controlled menus open Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23a42dde-c59d-4f5a-97ca-4fa6f7c41164 --- .../react/src/ActionMenu/ActionMenu.test.tsx | 28 +++++++++++++++++-- packages/react/src/ActionMenu/ActionMenu.tsx | 14 ++++------ 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/packages/react/src/ActionMenu/ActionMenu.test.tsx b/packages/react/src/ActionMenu/ActionMenu.test.tsx index a3210eab91c..5ddd7ff978d 100644 --- a/packages/react/src/ActionMenu/ActionMenu.test.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.test.tsx @@ -83,9 +83,12 @@ function ExampleWithTooltip(): JSX.Element { ) } -function IconButtonActionMenu({onSelect}: {onSelect?: () => void} = {}): JSX.Element { +function IconButtonActionMenu({ + onSelect, + ...props +}: {onSelect?: () => void} & Pick, 'open' | 'onOpenChange'> = {}): JSX.Element { return ( - + @@ -696,6 +699,27 @@ describe('ActionMenu', () => { expect(counter.updateCount).toBe(0) }) + it.each([false, true])('labels controlled menus with initial open state %s', async initialOpen => { + const onOpenChange = vi.fn() + const component = HTMLRender( + + + , + ) + + if (!initialOpen) { + component.rerender( + + + , + ) + } + + const menu = await component.findByRole('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 on first open', async () => { const component = HTMLRender( diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index ac6d4961b69..8c5f3e4a5e5 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -1,4 +1,4 @@ -import React, {useCallback, useContext, useMemo, useState} from 'react' +import React, {useCallback, useContext, useMemo, useEffect, useState} from 'react' import {clsx} from 'clsx' import {TriangleDownIcon, ChevronRightIcon} from '@primer/octicons-react' import type {AnchoredOverlayProps} from '../AnchoredOverlay' @@ -318,13 +318,11 @@ const Overlay: FCWithSlotMarker> = ({ // 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) - const handleOpen = useCallback>( - (gesture, event) => { + useEffect(() => { + if (open) { setAnchorAriaLabelledby(anchorRef.current?.getAttribute('aria-labelledby') ?? null) - onOpen?.(gesture, event) - }, - [anchorRef, onOpen], - ) + } + }, [anchorRef, open]) const afterSelect = useCallback(() => onClose?.('item-select'), [onClose]) @@ -348,7 +346,7 @@ const Overlay: FCWithSlotMarker> = ({ renderAnchor={renderAnchor} anchorId={anchorId} open={open} - onOpen={handleOpen} + onOpen={onOpen} onClose={handleClose} align={align} side={side ?? (isSubmenu ? 'outside-right' : 'outside-bottom')} From 5d325d5bdbe4975e63b512aa74f9fe4a22758eb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arma=C4=9Fan?= Date: Mon, 26 Oct 2026 10:23:32 +0000 Subject: [PATCH 3/4] Fix ActionMenu labeling before initial focus --- .../react/src/ActionMenu/ActionMenu.test.tsx | 70 ++++++++++++++++++- packages/react/src/ActionMenu/ActionMenu.tsx | 15 ++-- 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/packages/react/src/ActionMenu/ActionMenu.test.tsx b/packages/react/src/ActionMenu/ActionMenu.test.tsx index 5ddd7ff978d..c475b0139e0 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' @@ -85,8 +86,12 @@ function ExampleWithTooltip(): JSX.Element { function IconButtonActionMenu({ onSelect, + onFocus, ...props -}: {onSelect?: () => void} & Pick, 'open' | 'onOpenChange'> = {}): JSX.Element { +}: {onSelect?: () => void; onFocus?: React.FocusEventHandler} & Pick< + React.ComponentProps, + 'open' | 'onOpenChange' +> = {}): JSX.Element { return ( @@ -94,7 +99,9 @@ function IconButtonActionMenu({ - Copy link + + Copy link + @@ -683,6 +690,65 @@ describe('ActionMenu', () => { }) describe('anchor labeling and focus behavior', () => { + it.each(['uncontrolled', 'controlled', 'initially open'] as const)( + 'names the menu before initial item focus with CSS anchor positioning (%s)', + async mode => { + const onFocus = vi.fn(() => within(document.body).queryByRole('menu', {name: 'More actions'}) !== null) + const onOpenChange = vi.fn() + const example = (open?: boolean) => ( + + + + + + ) + const component = HTMLRender(example(mode === 'uncontrolled' ? undefined : mode === 'initially open')) + + if (mode === 'uncontrolled') { + await browserUserEvent.click(page.getByRole('button', {name: 'More actions', exact: true})) + } else if (mode === 'controlled') { + component.rerender(example(true)) + } + + await expect.element(page.getByRole('menuitem', {name: 'Copy link', exact: true})).toHaveFocus() + expect(onFocus).toHaveBeenCalled() + expect(onFocus.mock.results[0].value).toBe(true) + }, + ) + + it('resolves a changed external anchor label before focusing a reopened menu', async () => { + const anchorRef: React.RefObject = {current: null} + const onFocus = vi.fn(() => within(document.body).queryByRole('menu')?.getAttribute('aria-labelledby')) + const example = (open: boolean, labelId: string) => ( + + + More actions + Updated actions + + {}}> + + + Copy link + + + + + + ) + const component = HTMLRender(example(true, 'first-label')) + const item = page.getByRole('menuitem', {name: 'Copy link', exact: true}) + await expect.element(item).toHaveFocus() + expect(onFocus.mock.results.map(result => result.value)).toEqual(['first-label']) + + component.rerender(example(false, 'second-label')) + component.rerender(example(true, 'second-label')) + + await expect.element(item).toHaveFocus() + expect(onFocus.mock.results.map(result => result.value)).toEqual(['first-label', 'second-label']) + }) + it('does not publish anchor label state after mounting closed', async () => { const [Wrap, counter] = createRenderCounter() HTMLRender( diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index 8c5f3e4a5e5..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,15 +313,21 @@ 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(() => { + useLayoutEffect(() => { if (open) { setAnchorAriaLabelledby(anchorRef.current?.getAttribute('aria-labelledby') ?? null) + setAnchorLabelReady(true) } }, [anchorRef, open]) @@ -345,7 +352,7 @@ const Overlay: FCWithSlotMarker> = ({ anchorRef={anchorRef} renderAnchor={renderAnchor} anchorId={anchorId} - open={open} + open={open && anchorLabelReady} onOpen={onOpen} onClose={handleClose} align={align} From 21ee8c6c363fff45b22155d32a612ff6a134e4ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arma=C4=9Fan?= Date: Mon, 26 Oct 2026 10:44:51 +0000 Subject: [PATCH 4/4] Simplfy the tests --- .../react/src/ActionMenu/ActionMenu.test.tsx | 99 ++++--------------- 1 file changed, 20 insertions(+), 79 deletions(-) diff --git a/packages/react/src/ActionMenu/ActionMenu.test.tsx b/packages/react/src/ActionMenu/ActionMenu.test.tsx index c475b0139e0..18869971c6f 100644 --- a/packages/react/src/ActionMenu/ActionMenu.test.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.test.tsx @@ -690,65 +690,6 @@ describe('ActionMenu', () => { }) describe('anchor labeling and focus behavior', () => { - it.each(['uncontrolled', 'controlled', 'initially open'] as const)( - 'names the menu before initial item focus with CSS anchor positioning (%s)', - async mode => { - const onFocus = vi.fn(() => within(document.body).queryByRole('menu', {name: 'More actions'}) !== null) - const onOpenChange = vi.fn() - const example = (open?: boolean) => ( - - - - - - ) - const component = HTMLRender(example(mode === 'uncontrolled' ? undefined : mode === 'initially open')) - - if (mode === 'uncontrolled') { - await browserUserEvent.click(page.getByRole('button', {name: 'More actions', exact: true})) - } else if (mode === 'controlled') { - component.rerender(example(true)) - } - - await expect.element(page.getByRole('menuitem', {name: 'Copy link', exact: true})).toHaveFocus() - expect(onFocus).toHaveBeenCalled() - expect(onFocus.mock.results[0].value).toBe(true) - }, - ) - - it('resolves a changed external anchor label before focusing a reopened menu', async () => { - const anchorRef: React.RefObject = {current: null} - const onFocus = vi.fn(() => within(document.body).queryByRole('menu')?.getAttribute('aria-labelledby')) - const example = (open: boolean, labelId: string) => ( - - - More actions - Updated actions - - {}}> - - - Copy link - - - - - - ) - const component = HTMLRender(example(true, 'first-label')) - const item = page.getByRole('menuitem', {name: 'Copy link', exact: true}) - await expect.element(item).toHaveFocus() - expect(onFocus.mock.results.map(result => result.value)).toEqual(['first-label']) - - component.rerender(example(false, 'second-label')) - component.rerender(example(true, 'second-label')) - - await expect.element(item).toHaveFocus() - expect(onFocus.mock.results.map(result => result.value)).toEqual(['first-label', 'second-label']) - }) - it('does not publish anchor label state after mounting closed', async () => { const [Wrap, counter] = createRenderCounter() HTMLRender( @@ -765,38 +706,38 @@ describe('ActionMenu', () => { expect(counter.updateCount).toBe(0) }) - it.each([false, true])('labels controlled menus with initial open state %s', async initialOpen => { - const onOpenChange = vi.fn() + 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( - - - , - ) - - if (!initialOpen) { - component.rerender( + - - , - ) - } + + + , + ) - const menu = await component.findByRole('menu', {name: 'More actions'}) + 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 on first open', async () => { + 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 user = userEvent.setup() const toggleButton = component.getByRole('button', {name: 'More actions'}) - await user.click(toggleButton) + 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',