Skip to content

Fix layout animations not triggering with MotionValue#3651

Open
mattgperry wants to merge 1 commit intomainfrom
worktree-fix-issue-2907
Open

Fix layout animations not triggering with MotionValue#3651
mattgperry wants to merge 1 commit intomainfrom
worktree-fix-issue-2907

Conversation

@mattgperry
Copy link
Collaborator

Summary

Fixes #2907

Layout animations did not work when a MotionValue drove a layout-affecting style property (e.g. width, height). This happened because MotionValue updates bypass React's render cycle, so the projection system was never notified of the layout change — willUpdate() and didUpdate() were never called.

Cause: In VisualElement.bindToMotionValue(), the onChange handler only set isTransformDirty for transform properties. Non-transform positional properties (width, height, top, left, right, bottom) triggered a DOM render but never notified the projection system.

Fix: When a layout-affecting MotionValue changes, call projection.willUpdate() (to snapshot the old layout before render) and schedule projection.root.didUpdate() via frame.postRender (to measure the new layout and start the animation after the DOM updates).

Test plan

  • Added Cypress E2E test (layout-motion-value) that verifies a motion.div with layout and a useMotionValue-driven width animates correctly when the value changes
  • Test fails without the fix (width snaps to 300 instead of animating)
  • Test passes with the fix on React 18
  • Test passes with the fix on React 19
  • All 774 existing unit tests pass (2 pre-existing flaky failures in delay.test.ts)

🤖 Generated with Claude Code

When a layout-affecting MotionValue (width, height, top, left, right,
bottom) changes, notify the projection system so it can snapshot the
old layout and animate to the new one.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link

greptile-apps bot commented Mar 17, 2026

Greptile Summary

This PR fixes layout animations not triggering when a MotionValue directly drives a layout-affecting style property such as width or height. Because MotionValue updates bypass React's render cycle, willUpdate() and didUpdate() were never called, so the projection system had no snapshot to animate from.

The fix introduces a layoutKeys set (width, height, top, left, right, bottom) in VisualElement.ts and, when a bound MotionValue for one of those keys changes, synchronously calls projection.willUpdate() to capture a layout snapshot, then queues projection.root.didUpdate() via frame.postRender — which runs after the render step in the same animation frame, ensuring the DOM has already been updated before the new layout is measured. The approach correctly mirrors the existing React-lifecycle-driven path and the timing is sound.

Key observations:

  • Timing is correct: frame.render (DOM write) runs before frame.postRender (measurement trigger) in every animation frame, preserving the snapshot-before/measure-after invariant.
  • Guards prevent duplicate work: willUpdate's isLayoutDirty check and didUpdate's updateScheduled flag ensure only one snapshot and one measurement cycle occur per change event, even if onChange fires multiple times in a frame.
  • Minor: anonymous postRender callback — the closure () => this.projection?.root?.didUpdate() is re-created on every onChange call and does not benefit from Set-based deduplication in the frame batcher. A stable class-field reference (like the existing this.notifyUpdate pattern) would be marginally more efficient.
  • Minor: layoutKeys scope — properties like minWidth, maxWidth, padding, margin, and inset also affect layout but are not included. A comment documenting the intentional scope would help future contributors.

Confidence Score: 4/5

  • Safe to merge — the fix is logically correct and well-tested; minor style improvements could be made but none are blocking.
  • The core fix is sound: snapshot timing is correct (render before postRender), existing guards in willUpdate and didUpdate prevent duplicate work, optional chaining protects against projection teardown races, and the E2E test covers the described regression with a deterministic assertion. Two minor style concerns (anonymous callback allocation and incomplete layoutKeys coverage) are not bugs and do not affect correctness.
  • packages/motion-dom/src/render/VisualElement.ts — consider the anonymous postRender callback and undocumented layoutKeys scope.

Important Files Changed

Filename Overview
packages/motion-dom/src/render/VisualElement.ts Core fix: adds a layoutKeys set and notifies the projection system via willUpdate/postRender(didUpdate) when a layout-affecting MotionValue changes. Logic is correct and timing is sound (render step fires before postRender), but the anonymous closure in postRender and the limited scope of layoutKeys are worth addressing.
packages/framer-motion/cypress/integration/layout-motion-value.ts New Cypress E2E test that deterministically verifies the layout animation by using ease: () => 0.5 to freeze visual progress at 50%, then asserting getBoundingClientRect().width === 200. Timing and assertions are correct.
dev/react/src/tests/layout-motion-value.tsx New dev-test fixture: a motion.div with layout and a useMotionValue-driven width, toggled by a button. Straightforward and correct.

Sequence Diagram

sequenceDiagram
    participant User
    participant MotionValue
    participant VisualElement
    participant ProjectionNode
    participant FrameLoop
    participant DOM

    User->>MotionValue: width.set(300)
    MotionValue->>VisualElement: onChange(300)
    VisualElement->>VisualElement: latestValues[key] = 300
    VisualElement->>ProjectionNode: willUpdate() — snapshot current layout (100px)
    VisualElement->>FrameLoop: frame.postRender(scheduleDidUpdate)
    VisualElement->>FrameLoop: frame.render(this.render)

    Note over FrameLoop: Next animation frame
    FrameLoop->>DOM: render step — apply width: 300px
    FrameLoop->>ProjectionNode: postRender step — root.didUpdate()
    ProjectionNode->>ProjectionNode: microtask.read(scheduleUpdate)

    Note over FrameLoop: Microtask
    ProjectionNode->>DOM: update() — measure new layout (300px)
    ProjectionNode->>ProjectionNode: compute delta (100→300)
    ProjectionNode->>DOM: start layout animation via CSS transform
Loading

Last reviewed commit: 59bd0d5

Comment on lines +597 to +599
frame.postRender(
() => this.projection?.root?.didUpdate()
)
Copy link

Choose a reason for hiding this comment

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

P2 New anonymous function allocated on every onChange call

The postRender callback () => this.projection?.root?.didUpdate() is an arrow function created fresh each time the onChange handler fires. Because frame.postRender (backed by a Set<Process>) deduplicates by function reference, each distinct closure is stored separately. If onChange fires multiple times before the next postRender step runs, N distinct callbacks are enqueued and all executed — only the first didUpdate() call does real work (thanks to its updateScheduled guard), but the rest run needlessly.

A stable, pre-bound reference on the class (similar to how this.notifyUpdate is used for preRender) would ensure only one entry lives in the set at any time:

// as a class field:
private scheduleDidUpdate = () => this.projection?.root?.didUpdate()

// in the onChange handler:
frame.postRender(this.scheduleDidUpdate)

This mirrors the existing pattern for this.notifyUpdate and prevents unnecessary closure allocations on rapid value changes.

Comment on lines 47 to +54

const layoutKeys = new Set([
"width",
"height",
"top",
"left",
"right",
"bottom",
Copy link

Choose a reason for hiding this comment

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

P2 layoutKeys doesn't cover all layout-affecting CSS properties

The set only includes the six box-position/size properties listed here. Several other CSS properties that a user might drive with a MotionValue can equally affect an element's bounding box and therefore fail to trigger layout animations:

  • minWidth / maxWidth / minHeight / maxHeight
  • padding / paddingTop / paddingRight / paddingBottom / paddingLeft
  • margin / marginTop / marginRight / marginBottom / marginLeft
  • inset (CSS shorthand for top/right/bottom/left)
  • borderWidth and its long-hand variants

If this is an intentional trade-off (covering only the most common cases), it would be worth documenting with a comment so future contributors understand the scope.

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.

[BUG] Layout animations don't work with MotionValue

1 participant