Skip to content

Fix useScroll inner motion components bound to wrong timeline#3684

Merged
mattgperry merged 5 commits into
mainfrom
fix/scroll-view-timeline-transformed-parent
May 5, 2026
Merged

Fix useScroll inner motion components bound to wrong timeline#3684
mattgperry merged 5 commits into
mainfrom
fix/scroll-view-timeline-transformed-parent

Conversation

@mattgperry
Copy link
Copy Markdown
Collaborator

Summary

  • useScroll's acceleration factory reads target.current synchronously, but refs attach child-first during commit — so motion components inside the target run their bindToMotionValue first, when the target ref is still null.
  • That null fallback caused getTimeline() to return a generic ScrollTimeline (default cover range) instead of the intended ViewTimeline with contain range.
  • Visible symptom from useScroll with "start"/"end" string offsets broken with position: sticky inside transformed parent since 12.37.0 #3658: nested-motion opacity already advanced when the section scrolled into view, and never reached its end value.

Closes #3658. Supersedes #3659 — that PR reverted only the string-offset path; the same bug also reproduced with the array-form [[0,0],[1,1]] offset that was acceleration-eligible since the original ViewTimeline support landed.

Fix

Defer the scroll() call inside makeAccelerateConfig's factory by one microtask. By the time the microtask runs, all ref callbacks queued in the same commit have fired, so target.current is populated before getTimeline() inspects it.

The deferral is one microtask, well before the next paint, so no visual flash.

Test plan

  • New Cypress regression test in scroll-view-timeline-transformed-parent.ts that mirrors the reporter's repro structure (literal port).
    • Asserts getAnimations()[0].timeline is a ViewTimeline and rangeStart/rangeEnd are contain ....
    • Compares compositor-driven getComputedStyle(opacity) against JS scrollInfo progress at six scroll positions; max drift must be < 0.01.
    • Verified to fail without the fix (expected 'ScrollTimeline' to equal 'ViewTimeline').
  • Existing Cypress scroll specs still pass on React 18 + Chrome:
    • scroll-view-timeline.ts (3/3)
    • scroll-target-transform.ts (2/2)
    • scroll-accelerate.ts (3/3)
  • Same set passes on React 19 + Chrome.
  • use-scroll.test.tsx jest unit tests pass (6/6).

Why real Chrome

Electron's WAAPI/ViewTimeline implementation differs from production Chrome — the regression surface is compositor behavior, so the cypress run must use --browser chrome.

🤖 Generated with Claude Code

…omponents

When useScroll is given a target ref and a preset offset, motion components
nested inside that target were ending up bound to a generic ScrollTimeline
(default cover-range) instead of the intended ViewTimeline (contain range).
The visible symptom: opacity already advanced when the section scrolls into
view, and never reaching its end value.

Cause: makeAccelerateConfig's factory reads `target.current` synchronously.
Refs attach child-first during commit, so when an inner motion component's
bindToMotionValue runs, the outer target ref has not been populated yet —
getTimeline() falls through to ScrollTimeline + default range.

Fix: defer the scroll() call inside the factory by one microtask, so all
ref callbacks in the same commit have fired before getTimeline reads
target.current.

Adds a regression test in real Chrome (Electron's WAAPI semantics differ),
asserting both that the inner motion component receives a ViewTimeline with
"contain" range, and that the WAAPI-driven computed opacity matches the
JS scrollInfo path across the scroll range.

Fixes #3658
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 5, 2026

Greptile Summary

This PR fixes a race condition in useScroll's WAAPI acceleration path where inner motion components bound their timelines before the outer target ref was populated, causing them to fall back to a generic ScrollTimeline instead of the intended ViewTimeline.

  • use-scroll.ts: makeAccelerateConfig's factory now wraps the scroll() call in queueMicrotask, so target.current is read after React has finished attaching all refs in the current commit (child-first ordering). A cancelled flag prevents the scroll from starting if the component unmounts before the microtask fires.
  • Dev test page + Cypress spec: A new reproduction page mirrors the reporter's exact structure, and the accompanying Cypress test asserts ViewTimeline attachment and validates WAAPI/JS progress agreement across six scroll positions.

Confidence Score: 4/5

The fix is safe to merge — it correctly defers timeline binding until after refs are populated, with a clean cancel path for early unmounts.

The core change in use-scroll.ts is correct and well-reasoned: queueMicrotask fires after the entire commit (all ref attachments), so target.current is reliably populated before getTimeline inspects it. The cancelled guard correctly handles the unmount-before-microtask edge case. The one thing holding back a higher score is the Rules of Hooks violation in the new dev test component — calling useTransform inside .map() is a latent breakage risk if the static text prop ever varies in word count.

dev/react/src/tests/scroll-view-timeline-transformed-parent.tsx — the useTransform call inside .map() violates the Rules of Hooks and would break if word count ever changes.

Important Files Changed

Filename Overview
packages/framer-motion/src/value/use-scroll.ts Core fix: wraps the scroll() call inside makeAccelerateConfig's factory in a queueMicrotask to ensure target.current is populated before getTimeline() is called. The cancelled flag correctly handles early cleanup before the microtask fires.
dev/react/src/tests/scroll-view-timeline-transformed-parent.tsx New dev-page reproduction of #3658. Contains a Rules of Hooks violation: useTransform is called inside a .map() loop in TextReveal, which works only because the word count is static.
packages/framer-motion/cypress/integration/scroll-view-timeline-transformed-parent.ts New Cypress regression test. Verifies the animation attaches a ViewTimeline (not ScrollTimeline) and that WAAPI and JS progress values stay within 0.01 across six scroll positions. Coverage looks thorough.

Sequence Diagram

sequenceDiagram
    participant React as React Commit Phase
    participant Child as Child motion component
    participant Parent as Target ref (parent div)
    participant MQ as Microtask Queue
    participant Scroll as scroll() / getTimeline()

    Note over React: Before fix — synchronous path
    React->>Child: Attach child ref & call bindToMotionValue
    Child->>Scroll: factory() → scroll() called immediately
    Note over Scroll: target.current is null → ScrollTimeline ❌
    React->>Parent: Attach parent/target ref (too late)

    Note over React: After fix — deferred path
    React->>Child: Attach child ref & call bindToMotionValue
    Child->>MQ: factory() → queueMicrotask(scroll call)
    React->>Parent: Attach parent/target ref ✓
    Note over React: Commit complete
    MQ->>Scroll: Microtask fires → scroll() with target.current set
    Note over Scroll: target.current is Element → ViewTimeline ✓
Loading

Reviews (1): Last reviewed commit: "Fix useScroll acceleration binding wrong..." | Re-trigger Greptile

Comment on lines +98 to +114
{words.map((word, i) => {
const start = i / words.length
const end = start + 1 / words.length
const opacity = useTransform(
scrollYProgress,
[start, end],
[0.2, 1]
)
return (
<motion.span
key={i}
style={{ opacity, color: "cyan" }}
>
{word}
</motion.span>
)
})}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Rules of Hooks violation in test component

useTransform is called inside words.map(...), which is a loop. React's Rules of Hooks prohibit calling hooks inside loops because the call count must be stable across renders. In practice this holds here since text is a static prop with a fixed word count, but if text ever changes to a different word count (e.g. in a future variant of this test), React will throw "Rendered more/fewer hooks than previous render." Consider extracting the per-word animation into a small child component so each hook call site is at the top level of a function component.

mattgperry and others added 4 commits May 5, 2026 14:53
Reverts the useMotionRef ordering from 1449483 ("Fix ViewTimeline ref
timing") and the register() split from 8b8aacf (#3682). The microtask
deferral in makeAccelerateConfig (previous commit) handles the original
ViewTimeline ref-timing motivation in a way that also covers nested
motion components inside the target — which 1449483's reordering never
did.

Restores the long-standing order: onMount → mount(instance) → external
ref. mount() once again sets current + visualElementStore inline. The
visualElementStore.get-from-external-ref guarantee asserted by the
test added in #3682 is preserved by this order: mount populates the
store before the external ref callback runs.

Closes the regression #3682's reorder caused for Framer.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- use-scroll.ts: switch the factory's deferral from raw queueMicrotask + a
  cancelled flag to motion-dom's microtask.read / cancelMicrotask. Same
  scheduling semantics, real cancellation, matches the existing pattern
  in projection/node/create-projection-node.ts.
- dev test page: drop multi-paragraph file header, type FullRangeProbe's
  prop as MotionValue<number>, drop unused as-any casts, hoist duplicated
  hero style.
- cypress spec: drop stale "side-by-side plain vs scaled" header (the
  page has only one target now), drop dead readProgress helper, drop the
  TimelineRangeOffset stringify dance in favour of asserting rangeName
  directly, collapse stops/labels/lines bookkeeping to a number[] and
  assert max drift inline.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sync lockfile with package.json updates landed via recent dependabot
merges on main (next 15.5.10 → 15.5.15 etc).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI runs Cypress in its bundled Electron, whose Chromium predates
ViewTimeline. Without ViewTimeline support, useScroll falls back to
the JS scroll path so there's no WAAPI animation on the probe to
inspect — the assertion has nothing to test.

Skip the timeline-attachment test when window.ViewTimeline is absent
(matching the conditional pattern in scroll-view-timeline.ts). The
WAAPI-vs-JS parity test still runs unconditionally; in Electron it
trivially passes because both paths fall back to JS.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@mattgperry mattgperry merged commit 42cb5bb into main May 5, 2026
4 of 5 checks passed
@mattgperry mattgperry deleted the fix/scroll-view-timeline-transformed-parent branch May 5, 2026 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

useScroll with "start"/"end" string offsets broken with position: sticky inside transformed parent since 12.37.0

1 participant