Skip to content

Remove persisted styles when calling cancel()#3656

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

Remove persisted styles when calling cancel()#3656
mattgperry wants to merge 1 commit intomainfrom
worktree-fix-issue-2948

Conversation

@mattgperry
Copy link
Collaborator

Summary

  • cancel() now removes inline styles that were persisted by Motion's style persistence mechanism, reverting the element to its pre-animation state
  • Previously, cancel() only cancelled the WAAPI animation but left the persisted inline style in place, making it impossible to undo an animation's visual effect
  • Also fixes JSAnimation.cancel() after finish, which previously left the element at the final value because tick(0) was a no-op when the animation state was "finished"

Changes

  • NativeAnimation.cancel(): Now calls removeStyle() to clear the persisted inline style after cancelling the WAAPI animation
  • NativeAnimation.stop(): Calls native WAAPI cancel() directly instead of this.cancel(), so committed styles are preserved during interruption
  • JSAnimation.cancel(): Resets state to "running" before tick(0) so the animation properly resets to its initial keyframe value when cancelled after finish
  • removeStyle(): New utility in style-set.ts that mirrors setStyle() — removes a style property from an element's inline styles

Note: This is a behavioral change. Previously cancel() after finish was a no-op (the final value remained committed). Now it properly reverts the animation effect, matching WAAPI cancel semantics.

Fixes #2948

Test plan

  • Unit tests: 3 new tests in NativeAnimation.test.ts covering cancel removes styles, cancel removes CSS custom properties, and stop preserves committed styles
  • Cypress E2E: New animate-cancel-removes-styles test passing on React 18 and React 19
  • Playwright E2E: Updated cancel() after finish test to assert revert behavior — passes on Chromium and WebKit
  • All 776 existing unit tests pass
  • All 88 Playwright animate tests pass
  • Full build succeeds

🤖 Generated with Claude Code

cancel() now removes inline styles that were persisted by Motion's
style persistence mechanism, making the element revert to its
pre-animation state. Previously, cancel() only cancelled the WAAPI
animation but left the persisted inline style in place.

Changes:
- NativeAnimation.cancel(): removes persisted inline style via removeStyle()
- NativeAnimation.stop(): calls native WAAPI cancel directly to preserve
  committed styles (stop should keep the current value)
- JSAnimation.cancel(): resets state to "running" before tick(0) so the
  animation properly resets to its initial value when cancelled after finish
- Added removeStyle() utility alongside existing setStyle()

Fixes #2948

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@greptile-apps
Copy link

greptile-apps bot commented Mar 17, 2026

Greptile Summary

This PR fixes cancel() to properly revert Motion's style-persistence side-effects, matching native WAAPI cancel semantics. It corrects two independent bugs: (1) NativeAnimation left a Motion-persisted inline style in place after cancel() so the element was visually stuck at the final value; and (2) JSAnimation.cancel() was a no-op when called after finish because tick(0) short-circuited for state === "finished". The stop() path is simultaneously hardened to avoid triggering the new removeStyle() call during interruption.

Key changes:

  • NativeAnimation.cancel() — calls new removeStyle() after the WAAPI cancel to erase the inline style that was persisted in onfinish. However, the call is unconditional: it fires even when cancel() is invoked mid-animation (before onfinish and setStyle() have run), risking erasure of pre-existing inline styles unrelated to Motion.
  • NativeAnimation.stop() — now calls this.animation.cancel() directly rather than this.cancel(), preserving the committed style during interruption.
  • JSAnimation.cancel()this.state = "running" is set before tick(0), preventing the state === "finished" guard from forcing currentTime to totalDuration and applying the final keyframe instead of the initial one. As a side-effect, teardown() (and activeAnimations.mainThread--) is now reachable twice in one animation lifecycle (once from finish(), once from cancel()), surfacing a pre-existing counter imbalance that was previously masked by cancel() being a no-op after finish.
  • removeStyle() — clean new utility mirroring setStyle(); correctly uses removeProperty() for CSS custom properties and empty-string assignment for standard properties.

Confidence Score: 3/5

  • Safe to merge for the post-finish cancel fix, but the unconditional removeStyle() call introduces a regression for mid-animation cancels on elements with pre-existing inline styles.
  • The core intent is correct and well-tested for the after-finish case. However, removeStyle() firing unconditionally in NativeAnimation.cancel() is a behavioral regression for mid-animation cancels, and the JSAnimation double-teardown/double-decrement issue is now reachable in normal usage for the first time. These two issues reduce confidence from a clean merge.
  • packages/motion-dom/src/animation/NativeAnimation.ts (unconditional removeStyle) and packages/motion-dom/src/animation/JSAnimation.ts (double teardown after finish+cancel)

Important Files Changed

Filename Overview
packages/motion-dom/src/animation/NativeAnimation.ts cancel() now calls removeStyle() unconditionally for non-pseudo-element animations — this can clear pre-existing inline styles when cancel() is called mid-animation (before onfinish fires), creating a regression for elements with inline styles that predate the animation.
packages/motion-dom/src/animation/JSAnimation.ts Adding this.state = "running" before tick(0) correctly fixes the after-finish cancel being a no-op; however teardown() (and its activeAnimations.mainThread--) will now be called twice when cancel() follows finish(), making the pre-existing double-decrement reachable in normal usage.
packages/motion-dom/src/render/dom/style-set.ts New removeStyle() utility correctly mirrors setStyle() — uses removeProperty() for CSS custom properties and empty-string assignment for standard properties.
packages/motion-dom/src/animation/tests/NativeAnimation.test.ts Three new tests cover the intended post-finish cancel and stop-preserves-style scenarios well, but no test covers the mid-animation cancel regression (cancel() called before onfinish fires on an element with a pre-existing inline style).
tests/animate/animate.spec.ts Playwright test correctly updated from asserting the final position is preserved to asserting it reverts to the origin, matching the new cancel() semantics.

Sequence Diagram

sequenceDiagram
    participant User
    participant NativeAnimation
    participant WAAPI as Browser WAAPI
    participant DOM as Element Inline Style

    Note over NativeAnimation,DOM: Normal finish flow
    WAAPI->>NativeAnimation: onfinish callback
    NativeAnimation->>DOM: setStyle(element, name, finalKeyframe)
    NativeAnimation->>WAAPI: animation.cancel() [removes keyframe effect]

    Note over User,DOM: cancel() after finish (NEW behavior)
    User->>NativeAnimation: cancel()
    NativeAnimation->>WAAPI: animation.cancel() [no-op, already cancelled]
    NativeAnimation->>DOM: removeStyle(element, name) [clears persisted style ✓]

    Note over User,DOM: stop() during animation (fixed)
    User->>NativeAnimation: stop()
    NativeAnimation->>NativeAnimation: commitStyles() / updateMotionValue()
    NativeAnimation->>DOM: setStyle(element, name, currentValue)
    NativeAnimation->>WAAPI: animation.cancel() [direct, NOT this.cancel()]
    Note over DOM: Inline style preserved ✓

    Note over User,DOM: cancel() mid-animation (potential regression)
    User->>NativeAnimation: cancel() [before onfinish]
    NativeAnimation->>WAAPI: animation.cancel() [removes keyframe effect]
    NativeAnimation->>DOM: removeStyle(element, name) [⚠ may clear pre-existing style]
Loading

Last reviewed commit: 32a18a3

Comment on lines +152 to +155
const { element, name } = this.options || {}
if (element && name && !this.isPseudoElement) {
removeStyle(element, name)
}
Copy link

Choose a reason for hiding this comment

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

P2 removeStyle() called unconditionally on mid-animation cancel

removeStyle() is called here regardless of whether the animation has already finished. setStyle() is only invoked inside the onfinish handler (which also sets this.finishedTime). For a mid-animation cancel() call, setStyle() will never have run, so removeStyle() will inadvertently erase any pre-existing inline style that Motion itself did not write.

Consider this scenario:

element.style.opacity = '0'        // pre-existing inline style
const anim = animate(element, { opacity: 1 }, { duration: 1 })
// 500ms in…
anim.cancel()
// Before this PR → opacity reverts to '0' (WAAPI removes its effect, inline '0' survives)
// After this PR  → opacity becomes ''   (removeStyle clears the pre-existing value)

Because finishedTime !== null is already a reliable proxy for "the onfinish handler ran and setStyle() persisted the final value", the guard should include that check:

Suggested change
const { element, name } = this.options || {}
if (element && name && !this.isPseudoElement) {
removeStyle(element, name)
}
const { element, name } = this.options || {}
if (element && name && !this.isPseudoElement && this.finishedTime !== null) {
removeStyle(element, name)
}

Comment on lines +152 to +155
const { element, name } = this.options || {}
if (element && name && !this.isPseudoElement) {
removeStyle(element, name)
}
Copy link

Choose a reason for hiding this comment

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

P2 NativeAnimationExtended.stop() also calls setStyle() — cancel interacts with it

NativeAnimationExtended.updateMotionValue() (called from stop() when a motionValue is present) independently calls setStyle(element, name, current) to commit the mid-animation value before the WAAPI animation is cancelled. If a caller holds on to a stopped NativeAnimationExtended and later calls cancel(), removeStyle() will silently erase that committed mid-animation value even though finishedTime is still null.

With the finishedTime !== null guard suggested above, those mid-stop committed styles would survive a subsequent cancel(), which matches the intent of stop(). However, consider whether a cancel() after a stop() should instead revert completely — and if so, whether a dedicated flag (e.g. private hasPersistedStyle) would be a cleaner, more explicit way to track whether setStyle() was called, regardless of the path.

Comment on lines 519 to 523
this.startTime = 0
this.state = "running"
this.tick(0)
this.teardown()
this.options.onCancel?.()
Copy link

Choose a reason for hiding this comment

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

P2 Double teardown() / double decrement of activeAnimations.mainThread

finish() calls teardown() (which does activeAnimations.mainThread--), then sets this.state = "finished". If cancel() is subsequently called, it now produces observable effects (the tick(0) revert), so users are more likely to call it after finish than before. teardown() will run a second time, decrementing activeAnimations.mainThread again.

This is a pre-existing issue (the teardown() call was already in cancel() before this PR), but this change makes the double-decrement reachable in normal usage for the first time.

A simple guard would prevent it:

Suggested change
this.startTime = 0
this.state = "running"
this.tick(0)
this.teardown()
this.options.onCancel?.()
this.state = "running"
this.tick(0)
if (this.state !== "idle") this.teardown()

Or alternatively check activeAnimations.mainThread > 0 before decrementing inside teardown().

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.

[FEATURE] Remove persisted styles when calling cancel.

1 participant