useAnchoredPosition: recalculate position on scroll#7652
useAnchoredPosition: recalculate position on scroll#7652owenniblock wants to merge 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: fbacefb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
integration-tests: recommended
|
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
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>
owenniblock
force-pushed
the
useAnchoredPosition-scroll-recalculation
branch
from
95cca6b to
8d19b43
Compare
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
- 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>
There was a problem hiding this comment.
Pull request overview
This PR updates useAnchoredPosition (used by components like AnchoredOverlay, SelectPanel, etc.) to keep floating overlays visually anchored while the page or a scrollable ancestor container scrolls, addressing the “overlay detaches on scroll” issue.
Changes:
- Add scroll listeners for all scrollable ancestors (plus
window) to trigger anchored-position recalculation on scroll (rAF-throttled). - Add unit tests asserting recalculation occurs on both
windowscroll and scrollable-ancestor scroll. - Add dev Storybook scenarios demonstrating scroll-aware repositioning and include a patch changeset entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/hooks/useAnchoredPosition.ts | Adds scroll-ancestor detection and scroll-triggered repositioning logic. |
| packages/react/src/hooks/tests/useAnchoredPosition.test.tsx | Adds tests for scroll-driven reposition recalculation. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx | Adds dev stories to manually verify scroll recalculation behavior. |
| .changeset/anchored-position-scroll-recalculation.md | Patch changeset documenting the behavior change. |
Comments suppressed due to low confidence (1)
packages/react/src/hooks/tests/useAnchoredPosition.test.tsx:105
- Same timing issue here: using
setTimeout(50)to wait for the rAF callback can make this test flaky and unnecessarily slow. Consider fake timers / rAF flushing so the test can synchronously advance to the next animation frame and assert immediately.
// 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)
| React.useEffect(() => { | ||
| const anchorEl = anchorElementRef.current | ||
| if (!anchorEl) return | ||
|
|
There was a problem hiding this comment.
The scroll-listener effect bails out when anchorElementRef.current is null, but the effect only depends on the ref object and updatePosition. If the anchor element is conditionally rendered later (so current becomes non-null without changing the ref object), this effect will not re-run and scroll listeners will never be attached. Consider keying the effect off the resolved anchorEl value (e.g., compute const anchorEl = anchorElementRef.current during render and include that in the deps) so listeners are attached/updated when the anchor element appears or changes.
| 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) | ||
| } |
There was a problem hiding this comment.
PR description mentions scroll listeners are passive, but the implementation adds them without an options object. Either update the PR description or pass a consistent options object (and use the same options for removal) so the code matches the stated behavior.
There was a problem hiding this comment.
Do we need to utilize passive here, or are we okay with our usage of requestAnimationFrame?
| // 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) |
There was a problem hiding this comment.
These tests rely on real timers (setTimeout(50)) to wait for the rAF-throttled scroll handler, which makes the suite slower and can be flaky under load. Prefer using fake timers (e.g., vi.useFakeTimers() + advancing timers) or waiting on requestAnimationFrame deterministically (and/or waitFor) so the assertion doesn't depend on an arbitrary delay.
This issue also appears on line 92 of the same file.
TylerJDev
left a comment
TylerJDev
left a comment
There was a problem hiding this comment.
Looks good, thanks for the contribution too! Left some comments. Let me know what you think! 😁
| function getScrollableAncestors(element: Element): Array<Element | Window> { | ||
| const scrollables: Array<Element | Window> = [] | ||
| 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 | ||
| } |
There was a problem hiding this comment.
Does this need to run when the AnchoredOverlay is closed? I'm wondering if there are any performance implications, but I doubt it, as nothing here stands out as being slow.
| // Uses requestAnimationFrame to avoid layout thrashing during scroll. | ||
| React.useEffect(() => { | ||
| const anchorEl = anchorElementRef.current | ||
| if (!anchorEl) return |
There was a problem hiding this comment.
Random question - could we only run this when the overlay is active? I'm assuming no, but wanted to ask 😁
Same goes for the scroll event; if we couldn't return early here if the overlay active, would we only want to add the scroll event if the overlay is open?
| 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) | ||
| } |
There was a problem hiding this comment.
Do we need to utilize passive here, or are we okay with our usage of requestAnimationFrame?
Closes #2184
useAnchoredPositioncurrently watches for resize events (viauseResizeObserver) but not scroll. When a user scrolls a page or a scrollable container, the floating overlay stays at its initial absolute position while the anchor element moves — causing the overlay to visually detach from its trigger.This adds scroll event listeners to all scrollable ancestors of the anchor element, so the position is recalculated on scroll.
Note: The ideal long-term fix here would be to move to the Popover API with CSS Anchor Positioning, which handles scroll-aware positioning natively in the browser without any JavaScript listeners. This PR is a quick fix to unblock the issue until we can make that more fundamental change.
Changelog
New
useAnchoredPositionnow recalculates the overlay position when any scrollable ancestor of the anchor element (or the window) is scrolledgetScrollableAncestors()utility that finds all ancestors withoverflow: auto|scroll|overlayChanged
Removed
Rollout strategy
Testing & Reviewing
useAnchoredPosition.test.tsx:'should recalculate position when window scrolls'— dispatches scroll on window, asserts recalculation'should recalculate position when a scrollable ancestor scrolls'— creates a scrollableoverflow: autocontainer, dispatches scroll, asserts recalculationrequestAnimationFrame(at most one per frame), and listeners are only attached to actual scrollable ancestorsMerge checklist
useEffectthat only runs client-side;getScrollableAncestorsusesgetComputedStylewhich is DOM-only)