From 834e54a294a6b6452e040423cb23010f19013c05 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 19 Jan 2026 14:38:26 +0000 Subject: [PATCH 1/5] mitigate hydration mismatches --- package-lock.json | 8 ++-- .../src/Autocomplete/AutocompleteOverlay.tsx | 5 +-- .../FilteredActionListLoaders.tsx | 11 +++++- packages/react/src/Portal/Portal.tsx | 37 ++++++++++++------- packages/react/src/ThemeProvider.tsx | 4 ++ .../src/hooks/__tests__/useMedia.test.tsx | 10 ++--- packages/react/src/hooks/useMedia.tsx | 20 +++++----- 7 files changed, 57 insertions(+), 38 deletions(-) diff --git a/package-lock.json b/package-lock.json index c08231f808a..0978b7100e9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -81,7 +81,7 @@ "react-dom": "^18.3.1" }, "devDependencies": { - "@primer/react": "38.7.0", + "@primer/react": "38.7.1", "@primer/styled-react": "1.0.2", "@types/react": "^18.3.11", "@types/react-dom": "^18.3.0", @@ -95,7 +95,7 @@ "name": "example-nextjs", "version": "0.0.0", "dependencies": { - "@primer/react": "38.7.0", + "@primer/react": "38.7.1", "@primer/styled-react": "1.0.2", "next": "^16.0.10", "react": "^19.2.0", @@ -138,7 +138,7 @@ "version": "0.0.0", "dependencies": { "@primer/octicons-react": "^19.21.0", - "@primer/react": "38.7.0", + "@primer/react": "38.7.1", "@primer/styled-react": "1.0.2", "clsx": "^2.1.1", "next": "^16.0.10", @@ -26428,7 +26428,7 @@ }, "packages/react": { "name": "@primer/react", - "version": "38.7.0", + "version": "38.7.1", "license": "MIT", "dependencies": { "@github/mini-throttle": "^2.1.1", diff --git a/packages/react/src/Autocomplete/AutocompleteOverlay.tsx b/packages/react/src/Autocomplete/AutocompleteOverlay.tsx index 0755434225e..6a8cda6fb60 100644 --- a/packages/react/src/Autocomplete/AutocompleteOverlay.tsx +++ b/packages/react/src/Autocomplete/AutocompleteOverlay.tsx @@ -63,9 +63,8 @@ function AutocompleteOverlay({ setShowMenu(false) }, [setShowMenu]) - if (typeof window === 'undefined') { - return null - } + // Note: Overlay uses Portal which requires DOM, but it's only rendered when showMenu is true. + // When showMenu is false, we safely render VisuallyHidden which doesn't require DOM. return showMenu ? ( ( - {/* eslint-disable-next-line react-hooks/purity */} - + ))} diff --git a/packages/react/src/Portal/Portal.tsx b/packages/react/src/Portal/Portal.tsx index 68797db5a15..cc246385cc6 100644 --- a/packages/react/src/Portal/Portal.tsx +++ b/packages/react/src/Portal/Portal.tsx @@ -1,6 +1,5 @@ -import React, {useContext} from 'react' +import React, {useContext, useEffect, useState} from 'react' import {createPortal} from 'react-dom' -import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' const PRIMER_PORTAL_ROOT_ID = '__primerPortalRoot__' const DEFAULT_PORTAL_CONTAINER_NAME = '__default__' @@ -75,20 +74,24 @@ export const Portal: React.FC> = ({ containerName: _containerName, }) => { const {portalContainerName} = useContext(PortalContext) - const elementRef = React.useRef(null) - if (!elementRef.current) { + const [element, setElement] = useState(null) + + const onMountRef = React.useRef(onMount) + useEffect(() => { + onMountRef.current = onMount + }) + + useEffect(() => { + // Create portal element on mount const div = document.createElement('div') // Portaled content should get their own stacking context so they don't interfere // with each other in unexpected ways. One should never find themselves tempted // to change the zIndex to a value other than "1". div.style.position = 'relative' div.style.zIndex = '1' - elementRef.current = div - } + // eslint-disable-next-line react-hooks/set-state-in-effect + setElement(div) - const element = elementRef.current - - useLayoutEffect(() => { let containerName = _containerName ?? portalContainerName if (containerName === undefined) { containerName = DEFAULT_PORTAL_CONTAINER_NAME @@ -101,14 +104,20 @@ export const Portal: React.FC> = ({ `Portal container '${containerName}' is not yet registered. Container must be registered with registerPortalRoot before use.`, ) } - parentElement.appendChild(element) - onMount?.() + parentElement.appendChild(div) + onMountRef.current?.() return () => { - parentElement.removeChild(element) + parentElement.removeChild(div) + setElement(null) } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [element, _containerName, portalContainerName]) + }, [_containerName, portalContainerName]) + + // During SSR and initial client render, return null to match server output. + // After mount, the portal element is created and we can render children into it. + if (!element) { + return null + } return createPortal(children, element) } diff --git a/packages/react/src/ThemeProvider.tsx b/packages/react/src/ThemeProvider.tsx index 75a10b07b2b..09bc3ff55f4 100644 --- a/packages/react/src/ThemeProvider.tsx +++ b/packages/react/src/ThemeProvider.tsx @@ -147,6 +147,10 @@ export function useColorSchemeVar(values: Partial>, fallb } function useSystemColorMode() { + // NOTE: Hydration consideration - useState initializer reads matchMedia which + // can differ between server and client. This is intentional; ThemeProvider has + // a `preventSSRMismatch` prop that passes resolved server state to client via + // a script tag to handle this case. Without it, color mode changes post-hydration. const [systemColorMode, setSystemColorMode] = React.useState(getSystemColorMode) React.useEffect(() => { diff --git a/packages/react/src/hooks/__tests__/useMedia.test.tsx b/packages/react/src/hooks/__tests__/useMedia.test.tsx index 2884c18f2da..13379dcc1c3 100644 --- a/packages/react/src/hooks/__tests__/useMedia.test.tsx +++ b/packages/react/src/hooks/__tests__/useMedia.test.tsx @@ -43,8 +43,8 @@ describe('useMedia', () => { mockMatchMedia() }) - it('should default to the matchMedia value', () => { - mockMatchMedia() + it('should default to false and sync to matchMedia value after mount', () => { + mockMatchMedia({defaultMatch: true}) const match: boolean[] = [] @@ -55,11 +55,9 @@ describe('useMedia', () => { } render() + // First render defaults to false for SSR hydration safety expect(match[0]).toBe(false) - - mockMatchMedia({defaultMatch: true}) - - render() + // After useEffect runs, syncs to actual matchMedia value expect(match[1]).toBe(true) }) diff --git a/packages/react/src/hooks/useMedia.tsx b/packages/react/src/hooks/useMedia.tsx index 2e5f999ee40..73a0f7229bb 100644 --- a/packages/react/src/hooks/useMedia.tsx +++ b/packages/react/src/hooks/useMedia.tsx @@ -27,16 +27,16 @@ export function useMedia(mediaQueryString: string, defaultState?: boolean) { return defaultState } - if (canUseDOM) { - return window.matchMedia(mediaQueryString).matches + // No defaultState provided - default to false to avoid hydration mismatch. + // The useEffect will sync to the real matchMedia value after hydration. + if (!canUseDOM) { + // On server, warn that defaultState should be provided + warning( + true, + '`useMedia` When server side rendering, defaultState should be defined to prevent a hydration mismatches.', + ) } - // A default value has not been provided, and you are rendering on the server, warn of a possible hydration mismatch when defaulting to false. - warning( - true, - '`useMedia` When server side rendering, defaultState should be defined to prevent a hydration mismatches.', - ) - return false }) @@ -66,7 +66,9 @@ export function useMedia(mediaQueryString: string, defaultState?: boolean) { mediaQueryList.addListener(listener) } - // Make sure the media query list is in sync with the matches state + // Sync state with actual media query value after hydration. + // This may cause a re-render if defaultState differs from the real value, + // but that's expected behavior - not a hydration error. // eslint-disable-next-line react-hooks/set-state-in-effect setMatches(mediaQueryList.matches) From 195960f074feb701e4e0b51b9cb79f31d9746dbe Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 20 Jan 2026 03:27:26 +0000 Subject: [PATCH 2/5] updates --- e2e/components/ConfirmationDialog.test.ts | 3 + .../react/src/ActionMenu/ActionMenu.test.tsx | 124 ++++++++++++------ .../src/AnchoredOverlay/AnchoredOverlay.tsx | 2 + packages/react/src/Dialog/Dialog.test.tsx | 46 ++++--- packages/react/src/Dialog/Dialog.tsx | 50 +++++-- packages/react/src/Overlay/Overlay.test.tsx | 10 +- packages/react/src/Overlay/Overlay.tsx | 42 +++++- packages/react/src/Portal/Portal.tsx | 21 +-- .../src/SelectPanel/SelectPanel.test.tsx | 10 +- 9 files changed, 217 insertions(+), 91 deletions(-) diff --git a/e2e/components/ConfirmationDialog.test.ts b/e2e/components/ConfirmationDialog.test.ts index d9d07b18c6d..4beb90da30d 100644 --- a/e2e/components/ConfirmationDialog.test.ts +++ b/e2e/components/ConfirmationDialog.test.ts @@ -35,6 +35,9 @@ test.describe('ConfirmationDialog', () => { args: {open: true}, }) + // Wait for dialog to be visible + await page.locator('role=dialog').waitFor({state: 'visible'}) + // Default state expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot( `ConfirmationDialog.${story.title}.${theme}.png`, diff --git a/packages/react/src/ActionMenu/ActionMenu.test.tsx b/packages/react/src/ActionMenu/ActionMenu.test.tsx index 0be609c68e8..a8cc2252f93 100644 --- a/packages/react/src/ActionMenu/ActionMenu.test.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.test.tsx @@ -1,5 +1,5 @@ import {describe, expect, it, vi} from 'vitest' -import {render as HTMLRender, waitFor, act, within} from '@testing-library/react' +import {render as HTMLRender, waitFor, act, within, type RenderResult} from '@testing-library/react' import userEvent from '@testing-library/user-event' import type React from 'react' import BaseStyles from '../BaseStyles' @@ -13,6 +13,23 @@ import {SearchIcon, KebabHorizontalIcon} from '@primer/octicons-react' import type {JSX} from 'react' import {implementsClassName} from '../utils/testing' +/** + * Helper to wait for a menu to become visible and focused. + * With async Portal, the overlay renders but starts hidden until position is calculated. + * FocusZone activates after position is set, moving focus into the menu. + */ +async function waitForMenu(component: RenderResult, options?: {name?: string | RegExp}) { + const menu = await component.findByRole('menu', {...options, hidden: true}) + await waitFor(() => expect(menu).toBeVisible(), {timeout: 3000}) + // Wait for focus to be inside the menu (FocusZone has activated) + await waitFor(() => expect(menu.contains(document.activeElement)).toBe(true), {timeout: 3000}) + // Delay to ensure FocusZone/FocusTrap event listeners are fully attached + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 50)) + }) + return menu +} + function Example(): JSX.Element { return ( @@ -130,7 +147,8 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) - expect(component.getByRole('menu')).toBeInTheDocument() + const menu = await waitForMenu(component) + expect(menu).toBeInTheDocument() }) it('should open Menu on MenuButton keypress', async () => { @@ -141,7 +159,8 @@ describe('ActionMenu', () => { button.focus() await user.keyboard('{Enter}') - expect(component.getByRole('menu')).toBeInTheDocument() + const menu = await waitForMenu(component) + expect(menu).toBeInTheDocument() }) it('should close Menu on selecting an action with click', async () => { @@ -151,6 +170,7 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) + await waitForMenu(component) const menuItems = component.getAllByRole('menuitem') await user.click(menuItems[0]) @@ -164,6 +184,7 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) + await waitForMenu(component) const menuItems = component.getAllByRole('menuitem') menuItems[0].focus() await user.keyboard('{Enter}') @@ -178,6 +199,7 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) + await waitForMenu(component) const menuItems = component.getAllByRole('menuitem') await user.click(menuItems[3]) @@ -193,12 +215,16 @@ describe('ActionMenu', () => { await user.click(button) // select first item by role, that would close the menu - await user.click(component.getAllByRole('menuitemradio')[0]) + await waitForMenu(component) + const menuItems = component.getAllByRole('menuitemradio') + await user.click(menuItems[0]) expect(component.queryByRole('menu')).not.toBeInTheDocument() // open menu again and check if the first option is checked await user.click(button) - expect(component.getAllByRole('menuitemradio')[0]).toHaveAttribute('aria-checked', 'true') + await waitForMenu(component) + const menuItemsAfter = component.getAllByRole('menuitemradio') + expect(menuItemsAfter[0]).toHaveAttribute('aria-checked', 'true') }) it('should assign the right roles with groups & mixed selectionVariant', async () => { @@ -211,6 +237,7 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) + await waitForMenu(component) expect(component.getByLabelText('Status')).toHaveAttribute('role', 'menuitemradio') expect(component.getByLabelText('Clear Group by')).toHaveAttribute('role', 'menuitem') }) @@ -222,18 +249,19 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) - expect(component.queryByRole('menu')).toBeInTheDocument() + await waitForMenu(component) + const menuItems = component.getAllByRole('menuitem') // linkItem button is the active element at this point await user.keyboard('{ArrowDown}{s}') - expect(component.getAllByRole('menuitem')[4]).toEqual(document.activeElement) + await waitFor(() => expect(menuItems[4]).toEqual(document.activeElement)) await user.keyboard('{ArrowUp}') - expect(component.getAllByRole('menuitem')[3]).toEqual(document.activeElement) + await waitFor(() => expect(menuItems[3]).toEqual(document.activeElement)) // assumes mnemonics aria-keyshortcuts are ignored await user.keyboard('{g}') - expect(component.getAllByRole('menuitem')[3]).toEqual(document.activeElement) + await waitFor(() => expect(menuItems[3]).toEqual(document.activeElement)) }) it('should select last element when ArrowUp is pressed after opening Menu with click', async () => { @@ -244,13 +272,14 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) - expect(component.queryByRole('menu')).toBeInTheDocument() + await waitForMenu(component) + const menuItems = component.getAllByRole('menuitem') // 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) + expect(menuItems.pop()).toEqual(document.activeElement) }) it('should close the menu if Tab is pressed and move to next element', async () => { @@ -267,8 +296,9 @@ describe('ActionMenu', () => { 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) + await waitForMenu(component) + const menuItems = component.getAllByRole('menuitem') + expect(menuItems[0]).toEqual(document.activeElement) // TODO: revisit why we need this waitFor await waitFor(async () => { @@ -287,7 +317,7 @@ describe('ActionMenu', () => { await user.click(button) }) - expect(component.queryByRole('menu')).toBeInTheDocument() + await waitForMenu(component) const menuItems = component.getAllByRole('menuitem') // TODO: Fix the focus trap from taking over focus control @@ -318,7 +348,8 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) - expect(component.getByRole('menu')).toBeInTheDocument() + const menu = await waitForMenu(component) + expect(menu).toBeInTheDocument() }) it('should open menu on menu button click and it is wrapped with tooltip v2', async () => { @@ -334,7 +365,8 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) - expect(component.getByRole('menu')).toBeInTheDocument() + const menu = await waitForMenu(component) + expect(menu).toBeInTheDocument() }) it('should display tooltip when menu button is focused', async () => { @@ -375,7 +407,8 @@ describe('ActionMenu', () => { const user = userEvent.setup() await user.click(button) - expect(component.getByRole('menu')).toBeInTheDocument() + const menu = await waitForMenu(component) + expect(menu).toBeInTheDocument() }) it('should display tooltip v2 and menu anchor is focused', async () => { @@ -475,8 +508,9 @@ describe('ActionMenu', () => { const toggleButton = component.getByRole('button', {name: 'More actions'}) await userEvent.click(toggleButton) + const menu = await waitForMenu(component) expect(toggleButton).toHaveAttribute('aria-labelledby') - expect(component.getByRole('menu')).toHaveAttribute('aria-labelledby', toggleButton.getAttribute('aria-labelledby')) + expect(menu).toHaveAttribute('aria-labelledby', toggleButton.getAttribute('aria-labelledby')) }) describe('submenus', () => { @@ -487,12 +521,13 @@ describe('ActionMenu', () => { const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) await user.click(baseAnchor) + await waitForMenu(component) const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) expect(submenuAnchor).toHaveAttribute('aria-haspopup') await user.click(submenuAnchor) expect(submenuAnchor).toHaveAttribute('aria-expanded') - const subSubmenuAnchor = component.getByRole('menuitem', {name: 'Paste from'}) + const subSubmenuAnchor = await component.findByRole('menuitem', {name: 'Paste from'}) expect(subSubmenuAnchor).toHaveAttribute('aria-haspopup') await user.click(subSubmenuAnchor) expect(subSubmenuAnchor).toHaveAttribute('aria-expanded') @@ -505,10 +540,11 @@ describe('ActionMenu', () => { const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) await user.click(baseAnchor) + await waitForMenu(component) const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) await user.click(submenuAnchor) - const submenu = component.getByRole('menu', {name: 'Paste special'}) + const submenu = await component.findByRole('menu', {name: 'Paste special'}) await waitFor(() => { expect(submenu).toBeVisible() }) @@ -516,7 +552,7 @@ describe('ActionMenu', () => { const subSubmenuAnchor = within(submenu).getByRole('menuitem', {name: 'Paste from'}) await user.click(subSubmenuAnchor) - const subSubmenu = component.getByRole('menu', {name: 'Paste from'}) + const subSubmenu = await component.findByRole('menu', {name: 'Paste from'}) await waitFor(() => { expect(subSubmenu).toBeVisible() @@ -542,13 +578,14 @@ describe('ActionMenu', () => { const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) await user.click(baseAnchor) + await waitForMenu(component) const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) expect(submenuAnchor).toHaveAttribute('aria-haspopup', 'true') submenuAnchor.focus() await user.keyboard('{Enter}') expect(submenuAnchor).toHaveAttribute('aria-expanded', 'true') - const subSubmenuAnchor = component.getByRole('menuitem', {name: 'Paste from'}) + const subSubmenuAnchor = await component.findByRole('menuitem', {name: 'Paste from'}) subSubmenuAnchor.focus() await user.keyboard('{ArrowRight}') expect(subSubmenuAnchor).toHaveAttribute('aria-expanded', 'true') @@ -561,22 +598,34 @@ describe('ActionMenu', () => { const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) await user.click(baseAnchor) + await waitForMenu(component) const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) await user.click(submenuAnchor) + // Wait for submenu to be visible and focused + const submenu = await component.findByRole('menu', {name: 'Paste special', hidden: true}) + await waitFor(() => expect(submenu).toBeVisible()) + await waitFor(() => expect(submenu.contains(document.activeElement)).toBe(true)) + const subSubmenuAnchor = component.getByRole('menuitem', {name: 'Paste from'}) await user.click(subSubmenuAnchor) - expect(subSubmenuAnchor).toHaveAttribute('aria-expanded', 'true') + // Wait for sub-submenu to be visible and focused + const subSubmenu = await component.findByRole('menu', {name: 'Paste from', hidden: true}) + await waitFor(() => expect(subSubmenu).toBeVisible()) + await waitFor(() => expect(subSubmenu.contains(document.activeElement)).toBe(true)) + await waitFor(() => expect(subSubmenuAnchor).toHaveAttribute('aria-expanded', 'true')) await user.keyboard('{Escape}') - expect(subSubmenuAnchor).not.toHaveAttribute('aria-expanded', 'true') - expect(submenuAnchor).toHaveAttribute('aria-expanded', 'true') + await waitFor(() => expect(subSubmenuAnchor).not.toHaveAttribute('aria-expanded', 'true')) + await waitFor(() => expect(submenuAnchor).toHaveAttribute('aria-expanded', 'true')) + // Focus the submenuAnchor before ArrowLeft (like passing tests do) + submenuAnchor.focus() await user.keyboard('{ArrowLeft}') - expect(submenuAnchor).not.toHaveAttribute('aria-expanded', 'true') + await waitFor(() => expect(submenuAnchor).not.toHaveAttribute('aria-expanded', 'true')) - expect(baseAnchor).toHaveAttribute('aria-expanded', 'true') + await waitFor(() => expect(baseAnchor).toHaveAttribute('aria-expanded', 'true')) }) it('closes all menus when an item is selected', async () => { @@ -586,13 +635,14 @@ describe('ActionMenu', () => { const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) await user.click(baseAnchor) + await waitForMenu(component) const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) await user.click(submenuAnchor) - const subSubmenuAnchor = component.getByRole('menuitem', {name: 'Paste from'}) + const subSubmenuAnchor = await component.findByRole('menuitem', {name: 'Paste from'}) await user.click(subSubmenuAnchor) - const subSubmenuItem = component.getByRole('menuitem', {name: 'Current clipboard'}) + const subSubmenuItem = await component.findByRole('menuitem', {name: 'Current clipboard'}) await user.click(subSubmenuItem) expect(baseAnchor).not.toHaveAttribute('aria-expanded', 'true') @@ -676,7 +726,7 @@ describe('ActionMenu', () => { const button = component.getByRole('button') await user.click(button) - expect(component.getByRole('menu')).toBeInTheDocument() + await waitForMenu(component) expect(mockOnClick).toHaveBeenCalledTimes(1) // select and close menu @@ -686,7 +736,7 @@ describe('ActionMenu', () => { expect(button).toEqual(document.activeElement) await user.keyboard('{Enter}') - expect(component.queryByRole('menu')).toBeInTheDocument() + await waitForMenu(component) expect(mockOnKeyDown).toHaveBeenCalledTimes(1) }) @@ -717,7 +767,7 @@ describe('ActionMenu', () => { const button = component.getByRole('button') await user.click(button) - expect(component.getByRole('menu')).toBeInTheDocument() + await waitForMenu(component) expect(mockOnClick).toHaveBeenCalledTimes(1) // select and close menu @@ -727,7 +777,7 @@ describe('ActionMenu', () => { expect(button).toEqual(document.activeElement) await user.keyboard('{Enter}') - expect(component.queryByRole('menu')).toBeInTheDocument() + await waitForMenu(component) expect(mockOnKeyDown).toHaveBeenCalledTimes(1) }) @@ -755,7 +805,7 @@ describe('ActionMenu', () => { const button = component.getByRole('button') await user.click(button) - expect(component.getByRole('menu')).toBeInTheDocument() + await waitForMenu(component) expect(mockOnClick).toHaveBeenCalledTimes(1) // select and close menu @@ -765,7 +815,7 @@ describe('ActionMenu', () => { expect(button).toEqual(document.activeElement) await user.keyboard('{Enter}') - expect(component.queryByRole('menu')).toBeInTheDocument() + await waitForMenu(component) expect(mockOnKeyDown).toHaveBeenCalledTimes(1) }) @@ -798,7 +848,7 @@ describe('ActionMenu', () => { const button = component.getByRole('button') await user.click(button) - expect(component.getByRole('menu')).toBeInTheDocument() + await waitForMenu(component) expect(mockOnClick).toHaveBeenCalledTimes(1) // select and close menu @@ -808,7 +858,7 @@ describe('ActionMenu', () => { expect(button).toEqual(document.activeElement) await user.keyboard('{Enter}') - expect(component.queryByRole('menu')).toBeInTheDocument() + await waitForMenu(component) expect(mockOnKeyDown).toHaveBeenCalledTimes(1) }) }) diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx index c0cd18f64ef..c23ac6f144f 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx @@ -257,6 +257,8 @@ export const AnchoredOverlay: React.FC { diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index 3ce178375ab..e2508a30184 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -1,5 +1,5 @@ import React from 'react' -import {render, fireEvent, waitFor} from '@testing-library/react' +import {render, fireEvent, waitFor, screen} from '@testing-library/react' import {describe, expect, it, vi, beforeEach} from 'vitest' import userEvent from '@testing-library/user-event' import {Dialog} from './Dialog' @@ -99,12 +99,13 @@ describe('Dialog', () => { expect(onClose).toHaveBeenCalledWith('escape') }) - it('changes the style for `overflow` if it is not set to "hidden"', () => { + it('changes the style for `overflow` if it is not set to "hidden"', async () => { document.body.style.overflow = 'scroll' - const {container} = render( {}}>Pay attention to me) + const {container, getByRole} = render( {}}>Pay attention to me) - expect(container.ownerDocument.body).toHaveStyle('overflow: hidden') + await waitFor(() => getByRole('dialog')) + await waitFor(() => expect(container.ownerDocument.body).toHaveStyle('overflow: hidden')) }) it('does not attempt to change the style for `overflow` if it is already set to "hidden"', () => { @@ -164,8 +165,8 @@ describe('Dialog', () => { await user.click(getByLabelText('Close')) - expect(queryByRole('dialog')).toBeNull() - expect(triggerButton).toHaveFocus() + await waitFor(() => expect(queryByRole('dialog')).toBeNull()) + await waitFor(() => expect(triggerButton).toHaveFocus()) }) it('returns focus to the element passed in returnFocusRef when the dialog closes', async () => { @@ -201,7 +202,7 @@ describe('Dialog', () => { await user.click(triggerButton) await user.click(getByLabelText('Close')) - expect(getByRole('button', {name: 'return focus to (button 2)'})).toHaveFocus() + await waitFor(() => expect(getByRole('button', {name: 'return focus to (button 2)'})).toHaveFocus()) }) it('should support `className` on the Dialog element', async () => { @@ -233,7 +234,7 @@ describe('Dialog', () => { }) }) -it('automatically focuses the element that is specified as initialFocusRef', () => { +it('automatically focuses the element that is specified as initialFocusRef', async () => { const initialFocusRef = React.createRef() const {getByRole} = render( , ) - expect(getByRole('link')).toHaveFocus() + await waitFor(() => expect(getByRole('link')).toHaveFocus()) }) describe('Footer button loading states', () => { @@ -396,15 +397,18 @@ describe('Footer button loading states', () => { expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) }) - it('handles multiple dialogs with ref counting when flag is ON', () => { + it('handles multiple dialogs with ref counting when flag is ON', async () => { const {unmount: unmount1} = render( {}}>Dialog 1 , ) - expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) - expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) + // Wait for first dialog to fully mount + await waitFor(() => expect(screen.getByRole('dialog')).toBeInTheDocument()) + + await waitFor(() => expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true)) + await waitFor(() => expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true)) // Render second dialog const {unmount: unmount2} = render( @@ -413,23 +417,29 @@ describe('Footer button loading states', () => { , ) + // Wait for second dialog to fully mount + await waitFor(() => expect(screen.getAllByRole('dialog')).toHaveLength(2)) + // Attributes should still be present - expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) - expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) + await waitFor(() => expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true)) + await waitFor(() => expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true)) // Unmount first dialog unmount1() + // Wait for DOM to settle - verify one dialog still exists + await waitFor(() => expect(screen.getAllByRole('dialog')).toHaveLength(1)) + // Attributes should still be present (second dialog and provider are still mounted) - expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) - expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) + await waitFor(() => expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true)) + await waitFor(() => expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true)) // Unmount second dialog unmount2() // Now both should be removed - expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) - expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) + await waitFor(() => expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false)) + await waitFor(() => expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false)) }) it('handles multiple dialogs correctly when flag is OFF', () => { diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 41bfa481dd0..e30b0c75dfc 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -2,10 +2,9 @@ import React, {useCallback, useEffect, useRef, useState, type SyntheticEvent} fr import type {ButtonProps} from '../Button' import {Button, IconButton} from '../Button' import {useOnEscapePress, useProvidedRefOrCreate} from '../hooks' -import {useFocusTrap} from '../hooks/useFocusTrap' import {XIcon} from '@primer/octicons-react' import {useFocusZone} from '../hooks/useFocusZone' -import {FocusKeys} from '@primer/behaviors' +import {FocusKeys, focusTrap} from '@primer/behaviors' import Portal from '../Portal' import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' import {useId} from '../hooks/useId' @@ -270,16 +269,41 @@ const _Dialog = React.forwardRef(null) + const dialogRef = useRef(null) useRefObjectAsForwardedRef(forwardedRef, dialogRef) const backdropRef = useRef(null) - - useFocusTrap({ - containerRef: dialogRef, - initialFocusRef: initialFocusRef ?? autoFocusedFooterButtonRef, - restoreFocusOnCleanUp: returnFocusRef?.current ? false : true, - returnFocusRef, - }) + const hasInitialFocusRef = useRef(false) + const focusTrapAbortRef = useRef(null) + const previousFocusRef = useRef(null) as React.MutableRefObject + const [dialogMounted, setDialogMounted] = useState(false) + + // Callback ref to handle focus when element mounts (for async Portal timing) + const setDialogRef = useCallback( + (node: HTMLDivElement | null) => { + dialogRef.current = node + setDialogMounted(!!node) + if (node && !hasInitialFocusRef.current) { + hasInitialFocusRef.current = true + // Save previously focused element for return focus + previousFocusRef.current = document.activeElement as HTMLElement + const finalInitialFocusRef = initialFocusRef ?? autoFocusedFooterButtonRef + // Initialize focus trap - handles both initial focus and keyboard trapping + focusTrapAbortRef.current = focusTrap(node, finalInitialFocusRef?.current ?? undefined) ?? null + } + if (!node) { + hasInitialFocusRef.current = false + // Cleanup focus trap and handle return focus + focusTrapAbortRef.current?.abort() + if (returnFocusRef?.current instanceof HTMLElement) { + returnFocusRef.current.focus() + } else if (previousFocusRef.current instanceof HTMLElement) { + previousFocusRef.current.focus() + } + previousFocusRef.current = null + } + }, + [initialFocusRef, autoFocusedFooterButtonRef, returnFocusRef], + ) useOnEscapePress( (event: KeyboardEvent) => { @@ -290,6 +314,8 @@ const _Dialog = React.forwardRef { + if (!dialogMounted) return + const scrollbarWidth = window.innerWidth - document.body.clientWidth const dialog = dialogRef.current const usePerfOptimization = document.body.hasAttribute('data-dialog-scroll-optimized') @@ -316,7 +342,7 @@ const _Dialog = React.forwardRef
{ const user = userEvent.setup() const {getByRole} = render() await user.click(getByRole('button', {name: 'open overlay'})) - await waitFor(() => getByRole('button', {name: 'Confirm'})) - const confirmButton = getByRole('button', {name: 'Confirm'}) - expect(document.activeElement).toEqual(confirmButton) + const confirmButton = await waitFor(() => getByRole('button', {name: 'Confirm'})) + await waitFor(() => expect(document.activeElement).toEqual(confirmButton)) }) it('should focus first element on open when no initialFocusRef is passed', async () => { const user = userEvent.setup() const {getByRole} = render() await user.click(getByRole('button', {name: 'open overlay'})) - await waitFor(() => getByRole('button', {name: 'Cancel'})) - const cancelButton = getByRole('button', {name: 'Cancel'}) - expect(document.activeElement).toEqual(cancelButton) + const cancelButton = await waitFor(() => getByRole('button', {name: 'Cancel'})) + await waitFor(() => expect(document.activeElement).toEqual(cancelButton)) }) it('should not focus any element within the overlay on open when preventFocusOnOpen prop is true', async () => { diff --git a/packages/react/src/Overlay/Overlay.tsx b/packages/react/src/Overlay/Overlay.tsx index 9f588146323..a3b0cd970e0 100644 --- a/packages/react/src/Overlay/Overlay.tsx +++ b/packages/react/src/Overlay/Overlay.tsx @@ -1,11 +1,11 @@ import type {ComponentPropsWithRef, ReactElement} from 'react' import React, {useEffect, useRef} from 'react' import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' +import {iterateFocusableElements} from '@primer/behaviors/utils' import type {AriaRole, Merge} from '../utils/types' import type {TouchOrMouseEvent} from '../hooks' import {useOverlay} from '../hooks' import Portal from '../Portal' -import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' import type {AnchorSide} from '@primer/behaviors' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {useFeatureFlag} from '../FeatureFlags' @@ -190,8 +190,42 @@ const Overlay = React.forwardRef( forwardedRef, // eslint-disable-next-line @typescript-eslint/no-explicit-any ): ReactElement => { - const overlayRef = useRef(null) - useRefObjectAsForwardedRef(forwardedRef, overlayRef) + const overlayRef = useRef(null) + // Track if we've already handled initial focus to avoid double-focusing + const hasInitialFocusRef = useRef(false) + // Use a callback ref to sync both overlayRef and forwardedRef immediately when element mounts. + // This is needed because Portal may delay mounting the element. + // We also handle initial focus here since useOpenAndCloseFocus's effect runs before + // the Portal creates the element, so containerRef.current is null when it first runs. + const setRef = React.useCallback( + (node: HTMLDivElement | null) => { + overlayRef.current = node + if (typeof forwardedRef === 'function') { + forwardedRef(node) + } else if (forwardedRef) { + forwardedRef.current = node + } + + // Handle initial focus when element mounts (for async Portal) + // Schedule for next tick so inner content elements (like inputs) are rendered + if (node && !preventFocusOnOpen && !hasInitialFocusRef.current) { + hasInitialFocusRef.current = true + setTimeout(() => { + if (initialFocusRef?.current) { + initialFocusRef.current.focus() + } else { + const firstItem = iterateFocusableElements(node).next().value + firstItem?.focus() + } + }, 0) + } + // Reset when unmounting + if (!node) { + hasInitialFocusRef.current = false + } + }, + [forwardedRef, preventFocusOnOpen, initialFocusRef], + ) const slideAnimationDistance = 8 // var(--base-size-8), hardcoded to do some math const slideAnimationEasing = 'cubic-bezier(0.33, 1, 0.68, 1)' @@ -237,7 +271,7 @@ const Overlay = React.forwardRef( role={role} width={width} data-reflow-container={overflowEnabled || !preventOverflow ? true : undefined} - ref={overlayRef} + ref={setRef} left={leftPosition} right={right} height={height} diff --git a/packages/react/src/Portal/Portal.tsx b/packages/react/src/Portal/Portal.tsx index cc246385cc6..63ad3390867 100644 --- a/packages/react/src/Portal/Portal.tsx +++ b/packages/react/src/Portal/Portal.tsx @@ -1,5 +1,6 @@ -import React, {useContext, useEffect, useState} from 'react' +import React, {useContext, useState} from 'react' import {createPortal} from 'react-dom' +import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' const PRIMER_PORTAL_ROOT_ID = '__primerPortalRoot__' const DEFAULT_PORTAL_CONTAINER_NAME = '__default__' @@ -74,23 +75,24 @@ export const Portal: React.FC> = ({ containerName: _containerName, }) => { const {portalContainerName} = useContext(PortalContext) - const [element, setElement] = useState(null) + const [element, setElement] = useState(null) + // Store onMount in ref to avoid re-running effect when callback changes const onMountRef = React.useRef(onMount) - useEffect(() => { + useLayoutEffect(() => { onMountRef.current = onMount }) - useEffect(() => { - // Create portal element on mount + // Create and attach portal element in useLayoutEffect for SSR safety. + // useLayoutEffect runs synchronously after DOM mutations but before paint, + // minimizing visual flicker while avoiding hydration mismatches. + useLayoutEffect(() => { const div = document.createElement('div') // Portaled content should get their own stacking context so they don't interfere // with each other in unexpected ways. One should never find themselves tempted // to change the zIndex to a value other than "1". div.style.position = 'relative' div.style.zIndex = '1' - // eslint-disable-next-line react-hooks/set-state-in-effect - setElement(div) let containerName = _containerName ?? portalContainerName if (containerName === undefined) { @@ -105,16 +107,15 @@ export const Portal: React.FC> = ({ ) } parentElement.appendChild(div) + setElement(div) onMountRef.current?.() return () => { parentElement.removeChild(div) - setElement(null) } }, [_containerName, portalContainerName]) - // During SSR and initial client render, return null to match server output. - // After mount, the portal element is created and we can render children into it. + // Return null on server or before effect runs if (!element) { return null } diff --git a/packages/react/src/SelectPanel/SelectPanel.test.tsx b/packages/react/src/SelectPanel/SelectPanel.test.tsx index fae621080c7..031d19373c8 100644 --- a/packages/react/src/SelectPanel/SelectPanel.test.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.test.tsx @@ -270,7 +270,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { ) await user.click(screen.getByText('Select items')) - expect(screen.getByLabelText('Filter items')).toHaveFocus() + await waitFor(() => expect(screen.getByLabelText('Filter items')).toHaveFocus(), {timeout: 3000}) }) describe('selection', () => { @@ -753,9 +753,11 @@ for (const usingRemoveActiveDescendant of [false, true]) { renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) - expect(screen.getByLabelText('Filter items')).toHaveFocus() + await waitFor(() => expect(screen.getByLabelText('Filter items')).toHaveFocus(), {timeout: 3000}) - expect(getLiveRegion().getMessage('polite')?.trim()).toContain('This is a notice') + await waitFor(() => expect(getLiveRegion().getMessage('polite')?.trim()).toContain('This is a notice'), { + timeout: 3000, + }) }) it('should announce filtered results', async () => { @@ -763,7 +765,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) - expect(screen.getByLabelText('Filter items')).toHaveFocus() + await waitFor(() => expect(screen.getByLabelText('Filter items')).toHaveFocus(), {timeout: 3000}) await waitFor( async () => { From 810d5e07e77254c84368f067dc08ab13d1ff44f5 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Wed, 21 Jan 2026 19:21:53 +0000 Subject: [PATCH 3/5] mitigate hydration mismatches --- packages/react/src/ActionMenu/ActionMenu.tsx | 16 +++++++++--- .../ConfirmationDialog.module.css | 26 +++++++++++++++++++ packages/react/src/Dialog/Dialog.tsx | 2 +- .../src/SelectPanel/SelectPanel.test.tsx | 23 ++++++++++++---- .../react/src/SelectPanel/SelectPanel.tsx | 25 +++++++++++------- .../react/src/hooks/useMenuInitialFocus.ts | 3 ++- .../src/hooks/useMenuKeyboardNavigation.ts | 23 ++++++++++------ packages/react/src/hooks/useMnemonics.ts | 12 ++++++--- 8 files changed, 98 insertions(+), 32 deletions(-) create mode 100644 packages/react/src/ConfirmationDialog/ConfirmationDialog.module.css diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index 7e97ae4d673..ebb4cfc6dc1 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -3,7 +3,12 @@ import {TriangleDownIcon, ChevronRightIcon} from '@primer/octicons-react' import type {AnchoredOverlayProps} from '../AnchoredOverlay' import {AnchoredOverlay} from '../AnchoredOverlay' import type {OverlayProps} from '../Overlay' -import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuKeyboardNavigation} from '../hooks' +import { + useProvidedRefOrCreate, + useProvidedStateOrCreate, + useMenuKeyboardNavigation, + useRenderForcingRef, +} from '../hooks' import {Divider} from '../ActionList/Divider' import {ActionListContainerContext} from '../ActionList/ActionListContainerContext' import type {ButtonProps} from '../Button' @@ -284,7 +289,9 @@ const Overlay: FCWithSlotMarker> = ({ isSubmenu = false, } = React.useContext(MenuContext) as MandateProps - const containerRef = React.useRef(null) + // Use useRenderForcingRef so the component re-renders when containerRef is set. + // This ensures keyboard navigation hooks re-run after Portal creates its element. + const [containerRef, updateContainerRef] = useRenderForcingRef() const isNarrow = useResponsiveValue({narrow: true}, false) const isNarrowFullscreen = !!isNarrow && variant.narrow === 'fullscreen' @@ -300,7 +307,8 @@ const Overlay: FCWithSlotMarker> = ({ [isNarrowFullscreen, onClose], ) - useMenuKeyboardNavigation(open, handleClose, containerRef, anchorRef, isSubmenu) + // eslint-disable-next-line react-hooks/refs -- accessing ref.current in dependencies is intentional to re-run when ref is set + useMenuKeyboardNavigation(open, handleClose, containerRef, anchorRef, isSubmenu, [containerRef.current]) const responsiveVariant = useResponsiveValue(variant, {regular: 'anchored', narrow: 'anchored'}) @@ -333,7 +341,7 @@ const Overlay: FCWithSlotMarker> = ({ variant={variant} >
h1 { + padding: var(--base-size-6) var(--base-size-8); + flex-grow: 1; + } +} + +.ConfirmationBody { + padding: 0 var(--base-size-16) var(--base-size-16) var(--base-size-16); + font-size: var(--text-body-size-medium); + flex-grow: 1; +} + +.ConfirmationFooter { + display: grid; + grid-auto-flow: column; + grid-auto-columns: max-content; + grid-gap: var(--base-size-8); + align-items: end; + justify-content: end; + padding: var(--base-size-4) var(--base-size-16) var(--base-size-16); +} diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index e30b0c75dfc..df956655a35 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -288,7 +288,7 @@ const _Dialog = React.forwardRef { + expect(itemOne).toHaveAttribute('tabindex', '0') + }) - expect(document.activeElement!).toHaveAccessibleName('item three') + // Navigate to last item with PageDown + await user.keyboard('{PageDown}') - await user.type(document.activeElement!, '{PageUp}') + await waitFor(() => { + expect(document.activeElement!).toHaveAccessibleName('item three') + }) - expect(document.activeElement!).toHaveAccessibleName('item one') + // Navigate back to first item with PageUp + await user.keyboard('{PageUp}') + + await waitFor(() => { + expect(document.activeElement!).toHaveAccessibleName('item one') + }) } else { // First item by default should be the active element expect(document.activeElement!).toHaveAttribute( diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index 112327b846c..5beb3872ef7 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -476,20 +476,25 @@ function Panel({ }, [open, isNarrowScreenSize]) useEffect(() => { - const announceNotice = async () => { - if (!noticeRef.current) return - const liveRegion = document.querySelector('live-region') + if (!open || !notice) return - liveRegion?.clear() + // Use requestAnimationFrame to wait for Portal to render + const frameId = requestAnimationFrame(() => { + const announceNotice = async () => { + if (!noticeRef.current) return + const liveRegion = document.querySelector('live-region') - await announceFromElement(noticeRef.current, { - from: liveRegion ? liveRegion : undefined, - }) - } + liveRegion?.clear() + + await announceFromElement(noticeRef.current, { + from: liveRegion ? liveRegion : undefined, + }) + } - if (open && notice) { announceNotice() - } + }) + + return () => cancelAnimationFrame(frameId) }, [notice, open]) const anchorRef = useProvidedRefOrCreate(externalAnchorRef) diff --git a/packages/react/src/hooks/useMenuInitialFocus.ts b/packages/react/src/hooks/useMenuInitialFocus.ts index 20e0af08292..093a5c1edbe 100644 --- a/packages/react/src/hooks/useMenuInitialFocus.ts +++ b/packages/react/src/hooks/useMenuInitialFocus.ts @@ -5,6 +5,7 @@ export const useMenuInitialFocus = ( open: boolean, containerRef?: React.RefObject, anchorRef?: React.RefObject, + dependencies: React.DependencyList = [], ) => { /** * We need to pick the first element to focus based on how the menu was opened, @@ -72,6 +73,6 @@ export const useMenuInitialFocus = ( // 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], + [open, openingGesture, anchorRef, ...dependencies], ) } diff --git a/packages/react/src/hooks/useMenuKeyboardNavigation.ts b/packages/react/src/hooks/useMenuKeyboardNavigation.ts index 16192f2e228..ca5575210b5 100644 --- a/packages/react/src/hooks/useMenuKeyboardNavigation.ts +++ b/packages/react/src/hooks/useMenuKeyboardNavigation.ts @@ -17,12 +17,13 @@ export const useMenuKeyboardNavigation = ( containerRef?: React.RefObject, anchorRef?: React.RefObject, isSubmenu: boolean = false, + dependencies: React.DependencyList = [], ) => { - useMenuInitialFocus(open, containerRef, anchorRef) - useMnemonics(open, containerRef) - useCloseMenuOnTab(open, onClose, containerRef, anchorRef) - useMoveFocusToMenuItem(open, containerRef, anchorRef) - useCloseSubmenuOnArrow(open, isSubmenu, onClose, containerRef) + useMenuInitialFocus(open, containerRef, anchorRef, dependencies) + useMnemonics(open, containerRef, dependencies) + useCloseMenuOnTab(open, onClose, containerRef, anchorRef, dependencies) + useMoveFocusToMenuItem(open, containerRef, anchorRef, dependencies) + useCloseSubmenuOnArrow(open, isSubmenu, onClose, containerRef, dependencies) } /** @@ -34,6 +35,7 @@ const useCloseMenuOnTab = ( onClose: MenuCloseHandler | undefined, containerRef?: React.RefObject, anchorRef?: React.RefObject, + dependencies: React.DependencyList = [], ) => { React.useEffect(() => { const container = containerRef?.current @@ -49,7 +51,8 @@ const useCloseMenuOnTab = ( container?.removeEventListener('keydown', handler) anchor?.removeEventListener('keydown', handler) } - }, [open, onClose, containerRef, anchorRef]) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open, onClose, containerRef, anchorRef, ...dependencies]) } /** @@ -60,6 +63,7 @@ const useCloseSubmenuOnArrow = ( isSubmenu: boolean, onClose: MenuCloseHandler | undefined, containerRef?: React.RefObject, + dependencies: React.DependencyList = [], ) => { React.useEffect(() => { const container = containerRef?.current @@ -72,7 +76,8 @@ const useCloseSubmenuOnArrow = ( return () => { container?.removeEventListener('keydown', handler) } - }, [open, onClose, containerRef, isSubmenu]) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open, onClose, containerRef, isSubmenu, ...dependencies]) } /** @@ -83,6 +88,7 @@ const useMoveFocusToMenuItem = ( open: boolean, containerRef?: React.RefObject, anchorRef?: React.RefObject, + dependencies: React.DependencyList = [], ) => { React.useEffect(() => { const container = containerRef?.current @@ -110,5 +116,6 @@ const useMoveFocusToMenuItem = ( anchor?.addEventListener('keydown', handler) return () => anchor?.addEventListener('keydown', handler) - }, [open, containerRef, anchorRef]) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open, containerRef, anchorRef, ...dependencies]) } diff --git a/packages/react/src/hooks/useMnemonics.ts b/packages/react/src/hooks/useMnemonics.ts index 7f79e67001d..8fa04109923 100644 --- a/packages/react/src/hooks/useMnemonics.ts +++ b/packages/react/src/hooks/useMnemonics.ts @@ -8,7 +8,11 @@ import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' * typically appearing in a menu title, menu item, or the text of a button. */ -export const useMnemonics = (open: boolean, providedRef?: React.RefObject) => { +export const useMnemonics = ( + open: boolean, + providedRef?: React.RefObject, + dependencies: React.DependencyList = [], +) => { const containerRef = useProvidedRefOrCreate(providedRef) React.useEffect( @@ -26,7 +30,8 @@ export const useMnemonics = (open: boolean, providedRef?: React.RefObject container.removeEventListener('keydown', handler) }, - [open, containerRef], + // eslint-disable-next-line react-hooks/exhaustive-deps + [open, containerRef, ...dependencies], ) return {containerRef} From 2ba449d5900bed77bbe6696a39b0356924d17f3c Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Wed, 21 Jan 2026 21:30:36 +0000 Subject: [PATCH 4/5] renderforceref --- .../src/Autocomplete/AutocompleteOverlay.tsx | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/react/src/Autocomplete/AutocompleteOverlay.tsx b/packages/react/src/Autocomplete/AutocompleteOverlay.tsx index 6a8cda6fb60..eab184c4d8a 100644 --- a/packages/react/src/Autocomplete/AutocompleteOverlay.tsx +++ b/packages/react/src/Autocomplete/AutocompleteOverlay.tsx @@ -1,11 +1,10 @@ import type React from 'react' import {useCallback, useContext, useEffect, useRef} from 'react' -import {useAnchoredPosition} from '../hooks' +import {useAnchoredPosition, useRenderForcingRef} from '../hooks' import type {OverlayProps} from '../Overlay' import Overlay from '../Overlay' import type {ComponentProps} from '../utils/types' import {AutocompleteContext} from './AutocompleteContext' -import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' import VisuallyHidden from '../_VisuallyHidden' import classes from './AutocompleteOverlay.module.css' @@ -48,16 +47,31 @@ function AutocompleteOverlay({ computedAnchorRef.current = explicit ?? tokensRoot ?? inputRef.current }, [menuAnchorRef, inputRef]) - const {floatingElementRef, position} = useAnchoredPosition( + // Use useRenderForcingRef to trigger position recalculation when overlay mounts + // (Portal creates element asynchronously via useLayoutEffect) + const [floatingElementRef, updateFloatingRef] = useRenderForcingRef() + + const {position} = useAnchoredPosition( { side: 'outside-bottom', align: 'start', anchorElementRef: computedAnchorRef as React.RefObject, + floatingElementRef, }, - [showMenu, selectedItemLength], + [showMenu, selectedItemLength, floatingElementRef.current], ) - useRefObjectAsForwardedRef(scrollContainerRef, floatingElementRef) + // Sync floatingElementRef to scrollContainerRef for scroll handling + useEffect(() => { + scrollContainerRef.current = floatingElementRef.current + }, [scrollContainerRef, floatingElementRef.current]) + + // Clear overlay ref when closed so position resets between open/close cycles + useEffect(() => { + if (!showMenu && floatingElementRef.current) { + updateFloatingRef(null) + } + }, [showMenu, floatingElementRef, updateFloatingRef]) const closeOptionList = useCallback(() => { setShowMenu(false) @@ -72,7 +86,7 @@ function AutocompleteOverlay({ preventFocusOnOpen={true} onClickOutside={closeOptionList} onEscape={closeOptionList} - ref={floatingElementRef as React.RefObject} + ref={updateFloatingRef} top={position?.top} left={position?.left} className={clsx(classes.Overlay, className)} From cf9d66a82872ef03fc3c95e115977cfb9e4d62c1 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Wed, 21 Jan 2026 22:55:23 +0000 Subject: [PATCH 5/5] mitigate hydration mismatches --- packages/react/src/Autocomplete/AutocompleteOverlay.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/Autocomplete/AutocompleteOverlay.tsx b/packages/react/src/Autocomplete/AutocompleteOverlay.tsx index eab184c4d8a..09146279e7a 100644 --- a/packages/react/src/Autocomplete/AutocompleteOverlay.tsx +++ b/packages/react/src/Autocomplete/AutocompleteOverlay.tsx @@ -58,13 +58,13 @@ function AutocompleteOverlay({ anchorElementRef: computedAnchorRef as React.RefObject, floatingElementRef, }, - [showMenu, selectedItemLength, floatingElementRef.current], + [showMenu, selectedItemLength], ) // Sync floatingElementRef to scrollContainerRef for scroll handling useEffect(() => { scrollContainerRef.current = floatingElementRef.current - }, [scrollContainerRef, floatingElementRef.current]) + }, [scrollContainerRef, floatingElementRef]) // Clear overlay ref when closed so position resets between open/close cycles useEffect(() => {