Fix wrong starting position for layout transitions in sticky containers#3657
Fix wrong starting position for layout transitions in sticky containers#3657mattgperry wants to merge 1 commit intomainfrom
Conversation
…ainers When elements with layoutId are inside a position: sticky container, measurePageBox() incorrectly adds root scroll offset to viewport coordinates. Since sticky elements maintain viewport-relative positions when stuck, adding scroll offset produces incorrect page-relative coordinates, causing layout animation starting positions to be offset by the scroll amount. Add hasStickyAncestor check to measurePageBox() that walks up the DOM ancestor chain to detect sticky positioning. When a sticky ancestor is found, skip adding root scroll offset, keeping measurements in viewport coordinates where both snapshot and current layout are consistent. Fixes #2941 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR fixes a long-standing bug (#2941) where The fix introduces a Key concerns:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[measurePageBox called] --> B[measureViewportBox - get viewport coords]
B --> C{wasInScrollRoot?}
C -- "scroll?.wasRoot OR path has scroll root" --> D[Return viewport coords as-is]
C -- "new: hasStickyAncestor check" --> E{hasStickyAncestor?}
E -- "true (any sticky ancestor)" --> D
E -- "false" --> F[translateAxis: add root scroll offset\nviewport → page coords]
F --> G[Return page-relative box]
D --> G
style E fill:#ffe0b2,stroke:#f57c00
style D fill:#c8e6c9,stroke:#388e3c
style F fill:#e3f2fd,stroke:#1976d2
Last reviewed commit: 21301dd |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| hasStickyAncestor: (instance) => { | ||
| let el = instance.parentElement | ||
| while (el) { | ||
| if (window.getComputedStyle(el).position === "sticky") return true | ||
| el = el.parentElement | ||
| } | ||
| return false | ||
| }, |
There was a problem hiding this comment.
hasStickyAncestor returns true purely based on the CSS position property — it does not check whether the sticky ancestor is currently in its "stuck" (engaged) state. This means that when a page has been scrolled but not enough for the sticky container to engage, the scroll offset compensation is still skipped.
In this scenario the element's getBoundingClientRect().y equals naturalPageY - scrollY (normal-flow behaviour). Skipping translateAxis(box.y, scrollY) makes measurePageBox() return naturalPageY - scrollY instead of the correct naturalPageY, so the animation starts scrollY pixels too high.
To detect whether a sticky ancestor is actually engaged, you could compare the ancestor's getBoundingClientRect().top against its top CSS value:
hasStickyAncestor: (instance) => {
let el = instance.parentElement
while (el) {
const style = window.getComputedStyle(el)
if (style.position === "sticky") {
const stickyTop = parseFloat(style.top) || 0
// If the element is at (or near) its `top` value, it is engaged
if (el.getBoundingClientRect().top <= stickyTop + 1) return true
}
el = el.parentElement
}
return false
},The existing test only exercises the fully-engaged state (scroll to 2 000 px), so this case is not currently covered.
| hasStickyAncestor: (instance) => { | ||
| let el = instance.parentElement | ||
| while (el) { | ||
| if (window.getComputedStyle(el).position === "sticky") return true | ||
| el = el.parentElement | ||
| } | ||
| return false | ||
| }, |
There was a problem hiding this comment.
position: sticky is resolved relative to the nearest scrollable ancestor, not necessarily the root viewport. If the sticky container is inside an overflow: scroll/auto element, its "stuck" position is relative to that inner container, not the window. In that case the code skips this.root.scroll.offset (the window-level scroll) even though the correction should still be applied for the inner-container case.
Consider stopping the ancestor walk when an element with overflow: scroll or overflow: auto is encountered — that scroll container is the sticky's actual reference frame and the root scroll offset would still be relevant.
hasStickyAncestor: (instance) => {
let el = instance.parentElement
while (el) {
const style = window.getComputedStyle(el)
const overflow = style.overflowY
// Stop at a scroll container — sticky is relative to it, not the root
if (overflow === "scroll" || overflow === "auto") break
if (style.position === "sticky") return true
el = el.parentElement
}
return false
},| hasStickyAncestor: (instance) => { | ||
| let el = instance.parentElement | ||
| while (el) { | ||
| if (window.getComputedStyle(el).position === "sticky") return true | ||
| el = el.parentElement | ||
| } | ||
| return false | ||
| }, |
There was a problem hiding this comment.
getComputedStyle called for every ancestor on each measurement
window.getComputedStyle() forces a style recalculation and is relatively expensive. The function is called for every DOM ancestor in the tree on every measurePageBox() invocation — which also occurs inside removeTransform() for each ancestor node that has a transform (line 1124 of create-projection-node.ts).
For layouts with deep DOM hierarchies or many transformed ancestors this could accumulate meaningful cost at animation-start time. Consider caching the result on the projection node (invalidated on mount/unmount) so the walk is done at most once per animation batch rather than per-measurement.
| describe("Shared layout: sticky container", () => { | ||
| it("Layout animation works inside a sticky container after scrolling", () => { | ||
| cy.visit("?test=layout-shared-sticky") | ||
| .wait(50) | ||
| // Click btn-2 so indicator moves there | ||
| .get("#btn-2") | ||
| .trigger("click") | ||
| .wait(300) | ||
| // Scroll down so sticky container kicks in | ||
| .window() | ||
| .then((win: any) => { | ||
| win.scrollTo(0, 2000) | ||
| }) | ||
| .wait(100) | ||
| // Click btn-3 to trigger layoutId animation while sticky | ||
| .get("#btn-3") | ||
| .trigger("click") | ||
| .wait(500) | ||
| .get("#indicator") | ||
| .then(([$indicator]: any) => { | ||
| const appDoc = $indicator.ownerDocument | ||
| const btn2 = ( | ||
| appDoc.querySelector("#btn-2") as HTMLElement | ||
| ).getBoundingClientRect() | ||
| const btn3 = ( | ||
| appDoc.querySelector("#btn-3") as HTMLElement | ||
| ).getBoundingClientRect() | ||
| const indicator = $indicator.getBoundingClientRect() | ||
| const scrollY = | ||
| appDoc.defaultView.scrollY || | ||
| appDoc.defaultView.pageYOffset | ||
|
|
||
| // The indicator is animating from btn-2 to btn-3 (10s linear). | ||
| // At 500ms (~5% progress), it should be between the two buttons. | ||
| // | ||
| // WITHOUT the fix, the starting position is offset by ~scrollY | ||
| // (2000px), so the indicator would be far outside the button range. | ||
| // | ||
| // WITH the fix, the indicator stays within the button area. | ||
| const buttonsMinTop = Math.min(btn2.top, btn3.top) | ||
| const buttonsMaxTop = Math.max(btn2.top, btn3.top) | ||
|
|
||
| // Indicator must be within the button range (with tolerance | ||
| // for animation overshoot). The key assertion: it must NOT | ||
| // be offset by the scroll amount. | ||
| expect(indicator.top).to.be.greaterThan( | ||
| buttonsMinTop - 50, | ||
| `Indicator (${indicator.top}) should be near buttons ` + | ||
| `(${buttonsMinTop}-${buttonsMaxTop}), ` + | ||
| `not offset by scroll (${scrollY})` | ||
| ) | ||
| expect(indicator.top).to.be.lessThan( | ||
| buttonsMaxTop + 50, | ||
| `Indicator (${indicator.top}) should be near buttons ` + | ||
| `(${buttonsMinTop}-${buttonsMaxTop}), ` + | ||
| `not offset by scroll (${scrollY})` | ||
| ) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The Cypress test scrolls to 2 000 px (far past the sticky threshold of top: 20) and then triggers the animation. This validates the primary bug scenario but leaves two important cases untested:
- Sticky not yet engaged with page scroll > 0 — e.g.,
top: 200, scroll to 100 px, then animate. With the current implementationhasStickyAncestorstill returnstrueand the scroll offset is skipped, producing an animation starting position offset by-100 px. - Sticky inside an inner scroll container — the root scroll offset should still be applied.
Adding a test variant at scrollY < sticky.top (before engagement) would catch the regression described in the hasStickyAncestor comments above.
1 participant
Summary
layoutIdelements are insideposition: stickycontainersmeasurePageBox()was adding root scroll offset to viewport coordinates for elements inside sticky containers. Since sticky elements maintain viewport-relative positions when stuck, this produced incorrect page-relative coordinates, offsetting the animation start by the scroll amounthasStickyAncestorcheck that walks up the DOM to detect sticky positioning and skips the scroll offset when foundFixes #2941
Test plan
layout-shared-sticky) that verifies layout animations work correctly inside sticky containers after scrollingyarn buildsucceedsNote: The full bug (animation starting hundreds of pixels off) is most visible in Chrome/Edge/Firefox with real user scrolling. The Cypress/Electron environment partially reproduces the offset. The test validates that the indicator stays within the button area during animation, not offset by the scroll amount.
🤖 Generated with Claude Code