Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions dev/react/src/tests/animate-cancel-removes-styles.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { useEffect, useRef, useState } from "react"
import { animate } from "framer-motion"

export const App = () => {
const ref = useRef<HTMLDivElement>(null)
const [result, setResult] = useState("running")

useEffect(() => {
if (!ref.current) return

const animation = animate(
ref.current,
{ opacity: 1 },
{ duration: 0.1 }
)

// Wait for animation to finish, then cancel
const timeout = setTimeout(() => {
const before = ref.current!.style.opacity

animation.cancel()

const after = ref.current!.style.opacity

if (before === "1" && after === "") {
setResult("success")
} else {
setResult(`fail:before=${before},after=${after}`)
}
}, 500)

return () => clearTimeout(timeout)
}, [])

return (
<div ref={ref} className="box" id="box" style={{ opacity: 0 }}>
{result}
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
describe("animation.cancel() removes persisted styles", () => {
it("Removes persisted inline style when cancel is called after completion", () => {
cy.visit("?test=animate-cancel-removes-styles")
.wait(3000)
.get("#box")
.then(($el) => {
const text = $el.text()
expect(text).to.equal("success")
})
})
})
1 change: 1 addition & 0 deletions packages/motion-dom/src/animation/JSAnimation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,7 @@ export class JSAnimation<T extends number | string>
cancel() {
this.holdTime = null
this.startTime = 0
this.state = "running"
this.tick(0)
this.teardown()
this.options.onCancel?.()
Comment on lines 519 to 523
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().

Expand Down
13 changes: 11 additions & 2 deletions packages/motion-dom/src/animation/NativeAnimation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
noop,
secondsToMilliseconds,
} from "motion-utils"
import { setStyle } from "../render/dom/style-set"
import { removeStyle, setStyle } from "../render/dom/style-set"
import { supportsScrollTimeline } from "../utils/supports/scroll-timeline"
import { getFinalKeyframe } from "./keyframes/get-final"
import {
Expand Down Expand Up @@ -148,6 +148,11 @@ export class NativeAnimation<T extends AnyResolvedKeyframe>
try {
this.animation.cancel()
} catch (e) {}

const { element, name } = this.options || {}
if (element && name && !this.isPseudoElement) {
removeStyle(element, name)
}
Comment on lines +152 to +155
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
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.

}

stop() {
Expand All @@ -165,7 +170,11 @@ export class NativeAnimation<T extends AnyResolvedKeyframe>
this.commitStyles()
}

if (!this.isPseudoElement) this.cancel()
if (!this.isPseudoElement) {
try {
this.animation.cancel()
} catch (e) {}
}
}

/**
Expand Down
102 changes: 102 additions & 0 deletions packages/motion-dom/src/animation/__tests__/NativeAnimation.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { motionValue } from "../../value"
import { NativeAnimation } from "../NativeAnimation"
import { NativeAnimationExtended } from "../NativeAnimationExtended"

/**
Expand All @@ -9,6 +10,107 @@ import { NativeAnimationExtended } from "../NativeAnimationExtended"
* the scheduled render could apply the correct value, causing a visual flash
* back to the initial value.
*/
describe("NativeAnimation - cancel removes persisted styles", () => {
let mockAnimation: any

beforeEach(() => {
mockAnimation = {
cancel: jest.fn(),
onfinish: null,
playbackRate: 1,
currentTime: 300,
playState: "running",
effect: {
getComputedTiming: () => ({ duration: 300 }),
updateTiming: jest.fn(),
},
}

Element.prototype.animate = jest
.fn()
.mockImplementation(() => mockAnimation)
})

afterEach(() => {
;(Element.prototype as any).animate = undefined
jest.restoreAllMocks()
})

test("cancel() removes persisted inline style after animation finishes", () => {
const element = document.createElement("div")
const mv = motionValue(0)

const anim = new NativeAnimationExtended({
element,
name: "opacity",
keyframes: [0, 1],
motionValue: mv,
finalKeyframe: 1,
onComplete: jest.fn(),
duration: 300,
ease: "easeOut",
} as any)

// Simulate the WAAPI onfinish event firing
mockAnimation.onfinish?.()

// After finish, inline style should be persisted
expect(element.style.opacity).toBe("1")

// Now cancel - should remove the persisted style
anim.cancel()
expect(element.style.opacity).toBe("")
})

test("cancel() removes persisted inline style for CSS custom properties", () => {
const element = document.createElement("div")
const mv = motionValue(0)

const anim = new NativeAnimationExtended({
element,
name: "--my-color",
keyframes: ["red", "blue"],
motionValue: mv,
finalKeyframe: "blue",
onComplete: jest.fn(),
duration: 300,
ease: "easeOut",
} as any)

// Simulate finish
mockAnimation.onfinish?.()
expect(element.style.getPropertyValue("--my-color")).toBe("blue")

// Cancel should remove
anim.cancel()
expect(element.style.getPropertyValue("--my-color")).toBe("")
})

test("stop() preserves committed inline styles", () => {
const element = document.createElement("div")
document.body.appendChild(element)

const anim = new NativeAnimation({
element,
name: "opacity",
keyframes: [0, 1],
finalKeyframe: 1,
onComplete: jest.fn(),
duration: 300,
ease: "easeOut",
} as any)

// Mock commitStyles to set inline style (simulating WAAPI behavior)
mockAnimation.commitStyles = jest.fn(() => {
element.style.opacity = "0.5"
})

// stop() should preserve the committed style
anim.stop()
expect(element.style.opacity).toBe("0.5")
})
})

describe("NativeAnimation - onfinish style commit", () => {
let mockAnimation: any

Expand Down
9 changes: 9 additions & 0 deletions packages/motion-dom/src/render/dom/style-set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,12 @@ export function setStyle(
? element.style.setProperty(name, value as string)
: (element.style[name as any] = value as string)
}

export function removeStyle(
element: HTMLElement | SVGElement,
name: string
) {
isCSSVar(name)
? element.style.removeProperty(name)
: (element.style[name as any] = "")
}
8 changes: 4 additions & 4 deletions tests/animate/animate.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,19 +112,19 @@ test.describe("animate() methods", () => {
})
})

test("cancel() after finish is a no-op", async ({ page }) => {
test("cancel() after finish removes persisted styles", async ({ page }) => {
await waitForAnimation(
"animate/animate-cancel-after-finish.html",
page
)
await eachBox(page, async (box) => {
const id = await box.getAttribute("id")
// cancel() after finish should not revert — the final value is committed
// cancel() after finish should revert — removing persisted inline styles
const boundingBox = await box.boundingBox()
expect(
boundingBox?.x,
`${id} should remain at final position after cancel`
).toBeCloseTo(100)
`${id} should revert to original position after cancel`
).toBeCloseTo(0)
})
})

Expand Down