fix: useAnimate respects MotionConfig.skipAnimations#3680
fix: useAnimate respects MotionConfig.skipAnimations#3680jthrilly wants to merge 1 commit intomotiondivision:mainfrom
Conversation
The imperative useAnimate hook only checked reducedMotion via useReducedMotionConfig(), ignoring MotionConfig's skipAnimations prop. This meant WAAPI animations were still created even when skipAnimations was true, just with reduced timing. This is problematic for e2e testing tools like Playwright, which call element.getAnimations() for stability checks. WebKit reports these zero-duration WAAPI animations as running, causing timeouts. When skipAnimations is true, the returned animate function now resolves immediately via an empty GroupAnimationWithThen without creating any WAAPI animations, consistent with how declarative animations behave when skipAnimations is set on MotionConfig. Fixes motiondivision#3679
There was a problem hiding this comment.
Pull request overview
Updates useAnimate to respect <MotionConfig skipAnimations> so imperative WAAPI animations aren’t created when animations are globally skipped (e.g., for E2E stability checks).
Changes:
- Read
skipAnimationsfromMotionConfigContextinsideuseAnimate. - When
skipAnimationsis true, return an “immediate” animate implementation instead of creating WAAPI animations. - Add a hook test covering the
skipAnimationsbehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/framer-motion/src/animation/hooks/use-animate.ts | Adds skipAnimations check and a no-op animate path intended to avoid WAAPI creation. |
| packages/framer-motion/src/animation/hooks/tests/use-animate.test.tsx | Adds a test asserting animations are skipped under <MotionConfig skipAnimations>. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function createNoopAnimate<T extends Element>(scope: AnimationScope<T>) { | ||
| return ((..._args: any[]) => { | ||
| return new GroupAnimationWithThen([]) | ||
| }) as ReturnType<typeof createScopedAnimate> |
There was a problem hiding this comment.
createNoopAnimate returns new GroupAnimationWithThen([]). In motion-dom, GroupAnimation assumes at least one child animation for several getters (e.g. time, speed, state, startTime) and will throw when animations is empty (GroupAnimation.ts uses this.animations[0][propName]). This makes the returned controls unsafe to interact with when skipAnimations is true. Consider returning a dedicated no-op AnimationPlaybackControlsWithThen implementation (with safe default fields and finished resolved) or a GroupAnimationWithThen containing a single no-op controls object. Also note scope is currently unused here and will be flagged by the repo's noUnusedParameters TS setting unless renamed/removed.
| // Element style should not be changed | ||
| expect(scope.current).not.toHaveStyle( | ||
| "opacity: 0.5;" | ||
| ) |
There was a problem hiding this comment.
This test asserts that when skipAnimations is true the element style is not updated. However, skipAnimations is documented/covered elsewhere as "values will be set instantly" (see MotionConfigContext and components/MotionConfig/__tests__/index.test.tsx). For useAnimate, it would be more consistent to apply the final target state synchronously (without creating WAAPI animations) and still resolve immediately, and update this expectation accordingly.
Greptile SummaryThis PR adds
Confidence Score: 4/5Not safe to merge as-is — the noop silently discards target values, causing imperative animations inside a skipAnimations subtree to have no visual effect. Two P1 findings: the noop implementation contradicts how skipAnimations behaves everywhere else in the library (final values should be applied instantly, not discarded), and the accompanying test cements the wrong behavior. The fix direction is clear, so the overall change is a small targeted improvement that just needs the implementation corrected. Both changed files need attention: Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["useAnimate() called"] --> B{skipAnimations\nfrom MotionConfigContext?}
B -- "true (PR path)" --> C["createNoopAnimate(scope)"]
B -- "false" --> D["createScopedAnimate({ scope, reduceMotion })"]
C --> E["animate(...) returns\nGroupAnimationWithThen([])"]
E --> F["Promise resolves immediately\n⚠️ No values applied to element"]
D --> G["animateSubject → animateTarget\n→ animateMotionValue"]
G --> H{MotionGlobalConfig\n.skipAnimations?}
H -- "true" --> I["makeAnimationInstant()\nApply final keyframe instantly\n✅ Values applied"]
H -- "false" --> J["Full animation runs\n✅ Values applied at end"]
style F fill:#ffcccc,stroke:#cc0000
style I fill:#ccffcc,stroke:#009900
style J fill:#ccffcc,stroke:#009900
Reviews (1): Last reviewed commit: "fix: useAnimate respects MotionConfig.sk..." | Re-trigger Greptile |
| function createNoopAnimate<T extends Element>(scope: AnimationScope<T>) { | ||
| return ((..._args: any[]) => { | ||
| return new GroupAnimationWithThen([]) | ||
| }) as ReturnType<typeof createScopedAnimate> | ||
| } |
There was a problem hiding this comment.
Noop animate doesn't apply target values — inconsistent with declarative path
createNoopAnimate resolves immediately without writing any values to the element. When skipAnimations is true, the declarative path (animateMotionValue → makeAnimationInstant → frame.update(() => onUpdate(finalKeyframe))) and MotionGlobalConfig.skipAnimations both apply the final value instantly. The imperative noop skips it entirely, leaving the element frozen in its current state.
Concrete failure: calling animate(el, { opacity: 0 }) inside a <MotionConfig skipAnimations> subtree will not make the element invisible. An exit animation that fades to 0 leaves the element fully visible; a slide animation that moves the element leaves it in place. The test even asserts this broken state: expect(scope.current).not.toHaveStyle("opacity: 0.5;") — the declarative equivalent would set opacity to 0.5 instantly.
A correct implementation should call createScopedAnimate with options that force duration:0 (or temporarily set MotionGlobalConfig.skipAnimations = true), so the underlying animateMotionValue machinery applies the final keyframe via its fast-exit path.
| expect(scope.current).not.toHaveStyle( | ||
| "opacity: 0.5;" | ||
| ) |
There was a problem hiding this comment.
Test asserts the wrong behavior
This assertion encodes the inconsistency flagged in the implementation: the test expects the element NOT to have opacity: 0.5 after the animation completes. But skipAnimations in the declarative path means "apply the final value instantly, skip the transition" — not "discard the animation entirely". If the implementation is corrected to apply values instantly (consistent with the rest of the library), this assertion should become toHaveStyle("opacity: 0.5;") and the scope.animations.length check on line 133 should similarly reflect however many instant-animations the corrected path creates.
| function createNoopAnimate<T extends Element>(scope: AnimationScope<T>) { | ||
| return ((..._args: any[]) => { | ||
| return new GroupAnimationWithThen([]) | ||
| }) as ReturnType<typeof createScopedAnimate> | ||
| } |
There was a problem hiding this comment.
The scope argument accepted by createNoopAnimate is unused inside the returned function. Since the noop discards all args via ..._args, there is no need for the typed parameter — it only clutters the signature and may mislead future readers into thinking the scope is being modified.
| function createNoopAnimate<T extends Element>(scope: AnimationScope<T>) { | |
| return ((..._args: any[]) => { | |
| return new GroupAnimationWithThen([]) | |
| }) as ReturnType<typeof createScopedAnimate> | |
| } | |
| function createNoopAnimate<T extends Element>(_scope: AnimationScope<T>) { | |
| return ((..._args: any[]) => { | |
| return new GroupAnimationWithThen([]) | |
| }) as ReturnType<typeof createScopedAnimate> | |
| } |
|
Yeah, I see that this should probably transition immediately to any end state rather than discarding animation entirely. |
Summary
useAnimateonly checkedreducedMotionviauseReducedMotionConfig(), ignoringMotionConfig'sskipAnimationsprop. This meant WAAPI animations were still created even whenskipAnimationswastrue.When
skipAnimationsistrue, the returnedanimatefunction now resolves immediately via an emptyGroupAnimationWithThenwithout creating any WAAPI animations — consistent with how declarative animations behave.Motivation
This is problematic for e2e testing tools like Playwright, which call
element.getAnimations()to check element stability before taking screenshots. WebKit reports zero-duration WAAPI animations as running, causing stability checks to time out indefinitely — even though the app has explicitly opted out of animations via<MotionConfig skipAnimations>.The declarative animation path (
initial/animate/exitprops) correctly respectsskipAnimationsviaVisualElement.shouldSkipAnimations, but the imperativeuseAnimatepath was missing this check.Changes
use-animate.ts: ReadskipAnimationsfromMotionConfigContext. Whentrue, return a no-op animate function that creates no WAAPI animations.useAnimateskips animations when wrapped in<MotionConfig skipAnimations>.Fixes #3679