diff --git a/.changeset/poor-carrots-guess.md b/.changeset/poor-carrots-guess.md new file mode 100644 index 00000000000..9522e985590 --- /dev/null +++ b/.changeset/poor-carrots-guess.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +AnchoredOverlay: Ensure styles persist on anchors even when re-mounted (behind feature flag) diff --git a/packages/react/src/ActionMenu/ActionMenu.dev.stories.tsx b/packages/react/src/ActionMenu/ActionMenu.dev.stories.tsx index a436e4499ff..a0df92ec445 100644 --- a/packages/react/src/ActionMenu/ActionMenu.dev.stories.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.dev.stories.tsx @@ -1,7 +1,9 @@ import type {Meta} from '@storybook/react-vite' +import {useRef, useState} from 'react' import type {ComponentProps} from '../utils/types' import {ActionMenu} from './ActionMenu' import {ActionList} from '../ActionList' +import {Button} from '../Button' export default { title: 'Components/ActionMenu/Dev', @@ -34,3 +36,47 @@ export const WithCss = () => ( ) + +/** + * Reproduces a bug where switching the anchor DOM element (via unmount/remount) + * causes the CSS anchor positioning to break because `anchor-name` is never + * re-applied to the new element. + */ +export const AnchorElementReplacement = () => { + const anchorRef = useRef(null) + const [open, setOpen] = useState(false) + const [anchorKey, setAnchorKey] = useState(0) + + return ( +
+

+ 1. Open the menu below. 2. Click "Switch anchor (remount)" inside the menu. 3. The overlay should + remain anchored to the button — not jump to the top-left corner. +

+ + + + + + + { + // Prevent the menu from closing when clicking this item + event.preventDefault() + setAnchorKey(k => k + 1) + }} + > + Switch anchor (remount) + + + Item one + Item two + Item three + + + +
+ ) +} diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx index 345f379317e..d93f2672041 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx @@ -391,3 +391,64 @@ describe('AnchoredOverlay feature flag specific behavior', () => { }) }) }) + +describe('AnchoredOverlay anchor element replacement', () => { + it('should re-apply anchor-name to a new anchor DOM element when the overlay reopens', () => { + function TestComponent() { + const anchorRef = useRef(null) + const [open, setOpen] = useState(true) + const [anchorKey, setAnchorKey] = useState(0) + + return ( + + + + + setOpen(true)} + onClose={() => setOpen(false)} + renderAnchor={null} + anchorRef={anchorRef} + > +
content
+
+
+ ) + } + + const {baseElement} = render() + + // Verify anchor-name is set on the initial anchor element + const initialAnchor = baseElement.querySelector('[data-testid="anchor"]') as HTMLElement + expect(initialAnchor.style.getPropertyValue('anchor-name')).not.toBe('') + const anchorName = initialAnchor.style.getPropertyValue('anchor-name') + + // Close the overlay + const toggleButton = baseElement.querySelector('[data-testid="toggle"]') as HTMLElement + act(() => { + toggleButton.click() + }) + + // Replace the anchor DOM element by changing its key while overlay is closed + const switchButton = baseElement.querySelector('[data-testid="switch"]') as HTMLElement + act(() => { + switchButton.click() + }) + + // Reopen the overlay — the new anchor should get anchor-name before paint + act(() => { + toggleButton.click() + }) + + const newAnchor = baseElement.querySelector('[data-testid="anchor"]') as HTMLElement + expect(newAnchor).not.toBe(initialAnchor) + expect(newAnchor.style.getPropertyValue('anchor-name')).toBe(anchorName) + }) +}) diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx index 31b4b755b8f..51a532bcccb 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx @@ -1,6 +1,5 @@ import type React from 'react' -import {useCallback, useEffect, useRef, type JSX} from 'react' -import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' +import {useCallback, useEffect, useState, type JSX} from 'react' import type {OverlayProps} from '../Overlay' import Overlay from '../Overlay' import type {FocusTrapHookSettings} from '../hooks/useFocusTrap' @@ -170,13 +169,32 @@ export const AnchoredOverlay: React.FC { const cssAnchorPositioningFlag = useFeatureFlag('primer_react_css_anchor_positioning') - const supportsNativeCSSAnchorPositioning = useRef(false) - // eslint-disable-next-line react-hooks/refs - const cssAnchorPositioning = cssAnchorPositioningFlag && supportsNativeCSSAnchorPositioning.current + // Lazy initial state so feature detection runs once per mount on the client. + // Guarded for SSR where `document` is undefined. + const [supportsNativeCSSAnchorPositioning] = useState( + () => typeof document !== 'undefined' && 'anchorName' in document.documentElement.style, + ) + + const cssAnchorPositioning = cssAnchorPositioningFlag && supportsNativeCSSAnchorPositioning // Only use Popover API when both CSS anchor positioning is enabled AND renderAs is true const shouldRenderAsPopover = cssAnchorPositioning && renderAs === 'popover' const anchorRef = useProvidedRefOrCreate(externalAnchorRef) + // Track the current anchor DOM element in state so that effects depending on + // its identity (e.g. applying `anchor-name` for CSS anchor positioning) re-run + // when a consumer remounts/replaces the anchor element while keeping the same + // `anchorRef` object. Refs alone don't notify React when `.current` changes. + const [anchorElement, setAnchorElement] = useState(null) + // Detect when `anchorRef.current` has been mutated by a consumer (e.g. via + // their own `ref={anchorRef}`) without React's ref system notifying us. + // Reading the ref during render and conditionally calling setState is the + // documented React pattern for syncing external mutable state, but the + // react-hooks/refs lint rule is conservative and disallows it. + // eslint-disable-next-line react-hooks/refs + if (anchorRef.current !== anchorElement) { + setAnchorElement(anchorRef.current) + } const [overlayRef, updateOverlayRef] = useRenderForcingRef() + const [overlayElement, setOverlayElement] = useState(null) const anchorId = useId(externalAnchorId) const onClickOutside = useCallback(() => onClose?.('click-outside'), [onClose]) @@ -230,13 +248,11 @@ export const AnchoredOverlay: React.FC { - supportsNativeCSSAnchorPositioning.current = 'anchorName' in document.documentElement.style - // ensure overlay ref gets cleared when closed, so position can reset between closing/re-opening if (!open && overlayRef.current) { updateOverlayRef(null) @@ -254,36 +270,16 @@ export const AnchoredOverlay: React.FC { - if (!cssAnchorPositioning || !anchorRef.current) return - - const anchor = anchorRef.current - const overlay = overlayRef.current - anchor.style.setProperty('anchor-name', `--anchored-overlay-anchor-${id}`) - - return () => { - anchor.style.removeProperty('anchor-name') - if (overlay) { - overlay.style.removeProperty('position-anchor') - } - } - // eslint-disable-next-line react-hooks/refs - }, [cssAnchorPositioning, anchorRef, overlayRef, id, open]) + if (!cssAnchorPositioning || !anchorElement) return - // Track the overlay element so we can re-run the effect when it changes. - // The overlay unmounts when closed, so each open creates a new DOM node - - // that needs showPopover() called. - // eslint-disable-next-line react-hooks/refs - const overlayElement = overlayRef.current - - useLayoutEffect(() => { - // Read ref inside effect to get the value after child refs are attached const currentOverlay = overlayRef.current - if (!cssAnchorPositioning || !open || !currentOverlay) return - currentOverlay.style.setProperty('position-anchor', `--anchored-overlay-anchor-${id}`) + // Link the anchor and the overlay (when present) via CSS anchor positioning. + anchorElement.style.setProperty('anchor-name', `--anchored-overlay-anchor-${id}`) + + if (open && currentOverlay) { + currentOverlay.style.setProperty('position-anchor', `--anchored-overlay-anchor-${id}`) - const anchorElement = anchorRef.current - if (anchorElement) { const overlayWidth = width ? parseInt(widthMap[width]) : null const result = getDefaultPosition(anchorElement, overlayWidth) @@ -292,20 +288,29 @@ export const AnchoredOverlay: React.FC { + anchorElement.style.removeProperty('anchor-name') + // The overlay may no longer be in the DOM at this point, so we need to check for its presence before trying to update it. + if (currentOverlay) { + currentOverlay.style.removeProperty('position-anchor') + } + } + // overlayRef is a stable ref object; including it in deps is unnecessary. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [cssAnchorPositioning, shouldRenderAsPopover, open, anchorElement, overlayElement, id, width]) const showXIcon = onClose && variant.narrow === 'fullscreen' && displayCloseButton const XButtonAriaLabelledBy = closeButtonProps['aria-labelledby'] @@ -316,6 +321,8 @@ export const AnchoredOverlay: React.FC {renderAnchor && + // anchorRef is a ref object passed as a JSX `ref` prop on the rendered + // anchor; React writes to it at commit time, it is not read during render. // eslint-disable-next-line react-hooks/refs renderAnchor({ ref: anchorRef, @@ -353,6 +360,7 @@ export const AnchoredOverlay: React.FC