From 64f8eeb2e0a34a6539d0ad7c332559e30fe49818 Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Tue, 10 Mar 2026 19:25:17 +0000 Subject: [PATCH 1/6] useAnchoredPosition: recalculate position on scroll --- .../__tests__/useAnchoredPosition.test.tsx | 75 ++++++++++++++++++- .../react/src/hooks/useAnchoredPosition.ts | 51 +++++++++++++ 2 files changed, 124 insertions(+), 2 deletions(-) diff --git a/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx b/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx index d894305aa18..258db5ec4bb 100644 --- a/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx +++ b/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx @@ -1,5 +1,5 @@ -import {render, waitFor} from '@testing-library/react' -import {it, expect, vi} from 'vitest' +import {render, waitFor, act} from '@testing-library/react' +import {it, expect, vi, describe} from 'vitest' import React from 'react' import {useAnchoredPosition} from '../../hooks/useAnchoredPosition' @@ -34,3 +34,74 @@ it('should should return a position', async () => { `) }) }) + +describe('scroll recalculation', () => { + it('should recalculate position when window scrolls', async () => { + const cb = vi.fn() + render() + + // Wait for initial position calculation to stabilise + await waitFor(() => { + expect(cb).toHaveBeenCalledTimes(2) + }) + + // Wait a tick to let any pending effects settle + await new Promise(resolve => setTimeout(resolve, 50)) + const callCountBefore = cb.mock.calls.length + + // Simulate a window scroll event + act(() => { + window.dispatchEvent(new Event('scroll')) + }) + + // Wait for rAF-throttled handler to fire and trigger re-render + await new Promise(resolve => setTimeout(resolve, 50)) + + expect(cb.mock.calls.length).toBeGreaterThan(callCountBefore) + }) + + it('should recalculate position when a scrollable ancestor scrolls', async () => { + const ScrollableComponent = ({ + callback, + }: { + callback: (hookReturnValue: ReturnType) => void + }) => { + const floatingElementRef = React.useRef(null) + const anchorElementRef = React.useRef(null) + callback(useAnchoredPosition({floatingElementRef, anchorElementRef})) + return ( +
+
+
+
+
+
+ ) + } + + const cb = vi.fn() + const {container} = render() + + await waitFor(() => { + expect(cb).toHaveBeenCalledTimes(2) + }) + + // Wait a tick to let any pending effects settle + await new Promise(resolve => setTimeout(resolve, 50)) + const callCountBefore = cb.mock.calls.length + const scrollContainer = container.firstElementChild! + + // Simulate scroll on the scrollable ancestor + act(() => { + scrollContainer.dispatchEvent(new Event('scroll')) + }) + + // Wait for rAF-throttled handler to fire and trigger re-render + await new Promise(resolve => setTimeout(resolve, 50)) + + expect(cb.mock.calls.length).toBeGreaterThan(callCountBefore) + }) +}) diff --git a/packages/react/src/hooks/useAnchoredPosition.ts b/packages/react/src/hooks/useAnchoredPosition.ts index 32777aad1d7..ed23c2e0f67 100644 --- a/packages/react/src/hooks/useAnchoredPosition.ts +++ b/packages/react/src/hooks/useAnchoredPosition.ts @@ -5,6 +5,27 @@ import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' import {useResizeObserver} from './useResizeObserver' import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' +/** + * Returns all scrollable ancestor elements of the given element, plus the window. + * An element is scrollable if its computed overflow/overflow-x/overflow-y is + * 'auto', 'scroll', or 'overlay'. + */ +function getScrollableAncestors(element: Element): Array { + const scrollables: Array = [] + let current = element.parentElement + while (current) { + const style = getComputedStyle(current) + const overflowY = style.overflowY + const overflowX = style.overflowX + if (/auto|scroll|overlay/.test(overflowY) || /auto|scroll|overlay/.test(overflowX)) { + scrollables.push(current) + } + current = current.parentElement + } + scrollables.push(window) + return scrollables +} + export interface AnchoredPositionHookSettings extends Partial { floatingElementRef?: React.RefObject anchorElementRef?: React.RefObject @@ -100,6 +121,36 @@ export function useAnchoredPosition( useResizeObserver(updatePosition) // watches for changes in window size useResizeObserver(updatePosition, floatingElementRef as React.RefObject) // watches for changes in floating element size + // Recalculate position when any scrollable ancestor of the anchor scrolls. + // Uses requestAnimationFrame to avoid layout thrashing during scroll. + React.useEffect(() => { + const anchorEl = anchorElementRef.current + if (!anchorEl) return + + let rafId: number | null = null + const handleScroll = () => { + if (rafId !== null) return + rafId = requestAnimationFrame(() => { + rafId = null + updatePosition() + }) + } + + const scrollables = getScrollableAncestors(anchorEl) + for (const scrollable of scrollables) { + scrollable.addEventListener('scroll', handleScroll, {passive: true}) + } + + return () => { + for (const scrollable of scrollables) { + scrollable.removeEventListener('scroll', handleScroll) + } + if (rafId !== null) { + cancelAnimationFrame(rafId) + } + } + }, [anchorElementRef, updatePosition]) + return { floatingElementRef, anchorElementRef, From 702edd05b16fca647cc265f25910657b1771c83b Mon Sep 17 00:00:00 2001 From: owenniblock <4696224+owenniblock@users.noreply.github.com> Date: Wed, 11 Mar 2026 14:09:21 +0000 Subject: [PATCH 2/6] chore: auto-fix lint and formatting issues --- packages/react/src/hooks/useAnchoredPosition.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/hooks/useAnchoredPosition.ts b/packages/react/src/hooks/useAnchoredPosition.ts index ed23c2e0f67..6809146c933 100644 --- a/packages/react/src/hooks/useAnchoredPosition.ts +++ b/packages/react/src/hooks/useAnchoredPosition.ts @@ -138,7 +138,7 @@ export function useAnchoredPosition( const scrollables = getScrollableAncestors(anchorEl) for (const scrollable of scrollables) { - scrollable.addEventListener('scroll', handleScroll, {passive: true}) + scrollable.addEventListener('scroll', handleScroll) } return () => { From 8d19b43382174e81d3174a253ac87c5928169add Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Wed, 11 Mar 2026 17:30:28 +0000 Subject: [PATCH 3/6] Add scroll recalculation dev stories for useAnchoredPosition Add two new Storybook stories under Components/AnchoredOverlay/Dev: - ScrollRecalculation: demonstrates the scroll detachment bug inside a scrollable overflow:auto container - WindowScrollRecalculation: demonstrates the same bug with window-level scrolling Both stories include step-by-step instructions explaining how to reproduce the issue and what to expect before and after the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AnchoredOverlay.dev.stories.tsx | 124 +++++++++++++++++- 1 file changed, 123 insertions(+), 1 deletion(-) diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx index 1d79a4569c6..89353125ff7 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx @@ -4,7 +4,7 @@ import React, {useState} from 'react' import {Button} from '../Button' import {AnchoredOverlay} from '.' import {Stack} from '../Stack' -import {Dialog, Spinner} from '..' +import {Dialog, Spinner, Text} from '..' const meta = { title: 'Components/AnchoredOverlay/Dev', @@ -59,6 +59,128 @@ export const RepositionAfterContentGrows = () => { ) } +export const ScrollRecalculation = () => { + const [open, setOpen] = useState(false) + + return ( +
+ + + How to test (scrollable container): + + + 1. Scroll down inside the bordered box until you see the "Open overlay" button +
+ 2. Click the button to open the overlay +
+ 3. Scroll inside the box again +
+ 4. Expected: the overlay stays visually attached to the button +
+ 5. Bug (without fix): the overlay stays at its initial absolute position and detaches from + the button + + + +
+ {/* Spacer to push button below the fold */} +
+ + ↓ Scroll down to find the trigger button + +
+ +
+ setOpen(true)} + onClose={() => setOpen(false)} + renderAnchor={props => } + overlayProps={{ + role: 'dialog', + 'aria-modal': true, + 'aria-label': 'Scroll recalculation demo', + }} + focusZoneSettings={{disabled: true}} + preventOverflow={false} + > +
+ This overlay should stay attached to the button as you scroll. +
+ +
+ + {/* Spacer below the button so there's room to scroll further */} +
+
+
+ ) +} + +export const WindowScrollRecalculation = () => { + const [open, setOpen] = useState(false) + + return ( +
+ + + How to test (window scroll): + + + 1. Scroll down the page until you see the "Open overlay" button +
+ 2. Click the button to open the overlay +
+ 3. Scroll the page again +
+ 4. Expected: the overlay stays visually attached to the button +
+ 5. Bug (without fix): the overlay stays at its initial absolute position and detaches from + the button + + + + {/* Spacer to push button below the fold */} +
+ + ↓ Scroll down to find the trigger button + +
+ +
+ setOpen(true)} + onClose={() => setOpen(false)} + renderAnchor={props => } + overlayProps={{ + role: 'dialog', + 'aria-modal': true, + 'aria-label': 'Window scroll recalculation demo', + }} + focusZoneSettings={{disabled: true}} + preventOverflow={false} + > +
+ This overlay should stay attached to the button as you scroll the page. +
+ +
+ + {/* Spacer below the button so there's room to scroll further */} +
+
+ ) +} + export const RepositionAfterContentGrowsWithinDialog = () => { const [open, setOpen] = useState(false) const [loading, setLoading] = useState(true) From fbacefb3aef577adb01500e30c1c6957a61491f1 Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Thu, 12 Mar 2026 09:48:24 +0000 Subject: [PATCH 4/6] Fix lint and type errors, add changeset - Add eslint-disable for scroll listener (IntersectionObserver cannot detect continuous scroll position changes needed for repositioning) - Replace sx prop with style prop on Text components in dev stories - Add patch changeset for useAnchoredPosition scroll recalculation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .changeset/anchored-position-scroll-recalculation.md | 5 +++++ .../src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx | 8 ++++---- packages/react/src/hooks/useAnchoredPosition.ts | 1 + 3 files changed, 10 insertions(+), 4 deletions(-) create mode 100644 .changeset/anchored-position-scroll-recalculation.md diff --git a/.changeset/anchored-position-scroll-recalculation.md b/.changeset/anchored-position-scroll-recalculation.md new file mode 100644 index 00000000000..e29a6819a1b --- /dev/null +++ b/.changeset/anchored-position-scroll-recalculation.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +useAnchoredPosition: recalculate overlay position when any scrollable ancestor (or the window) is scrolled. diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx index 89353125ff7..3a9a149b282 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx @@ -65,7 +65,7 @@ export const ScrollRecalculation = () => { return (
- + How to test (scrollable container): @@ -93,7 +93,7 @@ export const ScrollRecalculation = () => { > {/* Spacer to push button below the fold */}
- + ↓ Scroll down to find the trigger button
@@ -131,7 +131,7 @@ export const WindowScrollRecalculation = () => { return (
- + How to test (window scroll): @@ -150,7 +150,7 @@ export const WindowScrollRecalculation = () => { {/* Spacer to push button below the fold */}
- + ↓ Scroll down to find the trigger button
diff --git a/packages/react/src/hooks/useAnchoredPosition.ts b/packages/react/src/hooks/useAnchoredPosition.ts index 6809146c933..7d89bdfd595 100644 --- a/packages/react/src/hooks/useAnchoredPosition.ts +++ b/packages/react/src/hooks/useAnchoredPosition.ts @@ -138,6 +138,7 @@ export function useAnchoredPosition( const scrollables = getScrollableAncestors(anchorEl) for (const scrollable of scrollables) { + // eslint-disable-next-line github/prefer-observers -- IntersectionObserver cannot detect continuous scroll position changes needed for repositioning scrollable.addEventListener('scroll', handleScroll) } From d5f2bb752cba50290a13d94499e528097d9e4a50 Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Tue, 24 Mar 2026 10:26:39 +0000 Subject: [PATCH 5/6] Address review feedback: enabled option, waitFor tests, passive clarification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `enabled` option to useAnchoredPosition to skip scroll listeners when the overlay is closed (addresses TylerJDev's feedback) - Replace flaky setTimeout(50) waits with waitFor() in scroll tests (addresses Copilot bot feedback about test reliability) - Add test for enabled=false behaviour - Drop passive: true — scroll events are not cancellable so it has no effect (confirmed by github/no-useless-passive lint rule) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../__tests__/useAnchoredPosition.test.tsx | 49 +++++++++++++++---- .../react/src/hooks/useAnchoredPosition.ts | 11 ++++- 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx b/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx index 258db5ec4bb..e567c4fd550 100644 --- a/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx +++ b/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx @@ -45,8 +45,6 @@ describe('scroll recalculation', () => { expect(cb).toHaveBeenCalledTimes(2) }) - // Wait a tick to let any pending effects settle - await new Promise(resolve => setTimeout(resolve, 50)) const callCountBefore = cb.mock.calls.length // Simulate a window scroll event @@ -55,9 +53,9 @@ describe('scroll recalculation', () => { }) // Wait for rAF-throttled handler to fire and trigger re-render - await new Promise(resolve => setTimeout(resolve, 50)) - - expect(cb.mock.calls.length).toBeGreaterThan(callCountBefore) + await waitFor(() => { + expect(cb.mock.calls.length).toBeGreaterThan(callCountBefore) + }) }) it('should recalculate position when a scrollable ancestor scrolls', async () => { @@ -89,8 +87,6 @@ describe('scroll recalculation', () => { expect(cb).toHaveBeenCalledTimes(2) }) - // Wait a tick to let any pending effects settle - await new Promise(resolve => setTimeout(resolve, 50)) const callCountBefore = cb.mock.calls.length const scrollContainer = container.firstElementChild! @@ -100,8 +96,43 @@ describe('scroll recalculation', () => { }) // Wait for rAF-throttled handler to fire and trigger re-render - await new Promise(resolve => setTimeout(resolve, 50)) + await waitFor(() => { + expect(cb.mock.calls.length).toBeGreaterThan(callCountBefore) + }) + }) + + it('should not attach scroll listeners when enabled is false', async () => { + const addEventListenerSpy = vi.spyOn(window, 'addEventListener') + + const DisabledComponent = ({ + callback, + }: { + callback: (hookReturnValue: ReturnType) => void + }) => { + const floatingElementRef = React.useRef(null) + const anchorElementRef = React.useRef(null) + callback(useAnchoredPosition({floatingElementRef, anchorElementRef, enabled: false})) + return ( +
+
+
+
+ ) + } + + const cb = vi.fn() + render() + + await waitFor(() => { + expect(cb).toHaveBeenCalled() + }) + + const scrollListeners = addEventListenerSpy.mock.calls.filter(([event]) => event === 'scroll') + expect(scrollListeners).toHaveLength(0) - expect(cb.mock.calls.length).toBeGreaterThan(callCountBefore) + addEventListenerSpy.mockRestore() }) }) diff --git a/packages/react/src/hooks/useAnchoredPosition.ts b/packages/react/src/hooks/useAnchoredPosition.ts index 7d89bdfd595..516ed04d8c1 100644 --- a/packages/react/src/hooks/useAnchoredPosition.ts +++ b/packages/react/src/hooks/useAnchoredPosition.ts @@ -31,6 +31,12 @@ export interface AnchoredPositionHookSettings extends Partial anchorElementRef?: React.RefObject pinPosition?: boolean onPositionChange?: (position: AnchorPosition | undefined) => void + /** + * When false, scroll listeners are not attached. Useful for skipping + * scroll-based repositioning when the overlay is closed. + * @default true + */ + enabled?: boolean } /** @@ -123,7 +129,10 @@ export function useAnchoredPosition( // Recalculate position when any scrollable ancestor of the anchor scrolls. // Uses requestAnimationFrame to avoid layout thrashing during scroll. + const enabled = settings?.enabled ?? true React.useEffect(() => { + if (!enabled) return + const anchorEl = anchorElementRef.current if (!anchorEl) return @@ -150,7 +159,7 @@ export function useAnchoredPosition( cancelAnimationFrame(rafId) } } - }, [anchorElementRef, updatePosition]) + }, [anchorElementRef, updatePosition, enabled]) return { floatingElementRef, From d988de10735c8a755b1201a00b0a1d1d89a6ba79 Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Tue, 24 Mar 2026 10:36:24 +0000 Subject: [PATCH 6/6] Remove enabled option from useAnchoredPosition The enabled option would require updating every caller to pass it, which isn't worth the complexity for this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../__tests__/useAnchoredPosition.test.tsx | 35 ------------------- .../react/src/hooks/useAnchoredPosition.ts | 11 +----- 2 files changed, 1 insertion(+), 45 deletions(-) diff --git a/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx b/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx index e567c4fd550..f822966f5a8 100644 --- a/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx +++ b/packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx @@ -100,39 +100,4 @@ describe('scroll recalculation', () => { expect(cb.mock.calls.length).toBeGreaterThan(callCountBefore) }) }) - - it('should not attach scroll listeners when enabled is false', async () => { - const addEventListenerSpy = vi.spyOn(window, 'addEventListener') - - const DisabledComponent = ({ - callback, - }: { - callback: (hookReturnValue: ReturnType) => void - }) => { - const floatingElementRef = React.useRef(null) - const anchorElementRef = React.useRef(null) - callback(useAnchoredPosition({floatingElementRef, anchorElementRef, enabled: false})) - return ( -
-
-
-
- ) - } - - const cb = vi.fn() - render() - - await waitFor(() => { - expect(cb).toHaveBeenCalled() - }) - - const scrollListeners = addEventListenerSpy.mock.calls.filter(([event]) => event === 'scroll') - expect(scrollListeners).toHaveLength(0) - - addEventListenerSpy.mockRestore() - }) }) diff --git a/packages/react/src/hooks/useAnchoredPosition.ts b/packages/react/src/hooks/useAnchoredPosition.ts index 516ed04d8c1..7d89bdfd595 100644 --- a/packages/react/src/hooks/useAnchoredPosition.ts +++ b/packages/react/src/hooks/useAnchoredPosition.ts @@ -31,12 +31,6 @@ export interface AnchoredPositionHookSettings extends Partial anchorElementRef?: React.RefObject pinPosition?: boolean onPositionChange?: (position: AnchorPosition | undefined) => void - /** - * When false, scroll listeners are not attached. Useful for skipping - * scroll-based repositioning when the overlay is closed. - * @default true - */ - enabled?: boolean } /** @@ -129,10 +123,7 @@ export function useAnchoredPosition( // Recalculate position when any scrollable ancestor of the anchor scrolls. // Uses requestAnimationFrame to avoid layout thrashing during scroll. - const enabled = settings?.enabled ?? true React.useEffect(() => { - if (!enabled) return - const anchorEl = anchorElementRef.current if (!anchorEl) return @@ -159,7 +150,7 @@ export function useAnchoredPosition( cancelAnimationFrame(rafId) } } - }, [anchorElementRef, updatePosition, enabled]) + }, [anchorElementRef, updatePosition]) return { floatingElementRef,