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
18 changes: 18 additions & 0 deletions packages/motion-dom/src/animation/__tests__/JSAnimation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1422,4 +1422,22 @@ describe("JSAnimation", () => {
expect(animation.sample(1000).value).toBe("90%")
expect(animation.sample(1999).value).toBe("90%")
})

// https://github.com/motiondivision/motion/issues/2791
test("Spring over SVG polygon points never produces NaN", () => {
// An invalid spring physics value (here an explicit `undefined`
// stiffness, as forwarded from an optional prop) previously emitted
// NaN progress, which the complex-value mixer turned into an invalid
// "NaN,NaN NaN,NaN" point list written to the <polygon> element.
const animation = animateValue({
keyframes: ["150,5 75,200 225,200", "150,5 50,180 250,180"],
type: "spring",
stiffness: undefined,
autoplay: false,
})

for (let t = 0; t <= 2000; t += 50) {
expect(animation.sample(t).value).not.toContain("NaN")
}
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -285,3 +285,36 @@ describe("toString", () => {
)
})
})

// https://github.com/motiondivision/motion/issues/2791
describe("spring NaN guards", () => {
const sample = (options: ValueAnimationOptions<number>) => {
const generator = spring(options)
return [0, 100, 300, 600, 1000].map((t) => generator.next(t).value)
}

test("stiffness of 0 does not produce NaN", () => {
const values = sample({ keyframes: [0, 100], stiffness: 0 })
values.forEach((v) => expect(v).not.toBeNaN())
})

test("mass of 0 does not produce NaN", () => {
const values = sample({ keyframes: [0, 100], mass: 0 })
values.forEach((v) => expect(v).not.toBeNaN())
})

/**
* An explicit `stiffness: undefined` (e.g. a forwarded prop that resolves
* to undefined) clobbers the default via the options spread in
* getSpringOptions, which previously produced NaN spring values.
*/
test("explicit undefined stiffness does not produce NaN", () => {
const values = sample({ keyframes: [0, 100], stiffness: undefined })
values.forEach((v) => expect(v).not.toBeNaN())
})

test("explicit undefined mass does not produce NaN", () => {
const values = sample({ keyframes: [0, 100], mass: undefined })
values.forEach((v) => expect(v).not.toBeNaN())
})
})
14 changes: 14 additions & 0 deletions packages/motion-dom/src/animation/generators/spring.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,20 @@ function getSpringOptions(options: SpringOptions) {
}
}

/**
* Guard against non-positive or non-finite stiffness/mass. These divide
* and feed Math.sqrt() during resolution, so a 0 (or an explicit
* `undefined` that clobbers the default via the spread above) produces NaN
* spring values — which corrupt any animated value, e.g. an SVG polygon's
* points list. See https://github.com/motiondivision/motion/issues/2791
*/
if (!(springOptions.stiffness > 0)) {
springOptions.stiffness = springDefaults.stiffness
}
if (!(springOptions.mass > 0)) {
springOptions.mass = springDefaults.mass
}
Comment on lines +225 to +230

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 damping: undefined not guarded — same NaN path as the fixed bug

stiffness and mass are guarded, but damping goes through the exact same spread-clobber mechanism and isn't protected. If a caller passes transition={{ damping: optionalProp }} where optionalProp is undefined, the ...options spread clobbers springDefaults.damping (10), leaving springOptions.damping = undefined. Then dampingRatio = undefined / (2 * Math.sqrt(stiffness * mass)) evaluates to NaN, which makes every branch comparison (NaN < 1, NaN === 1) false, so the overdamped path runs with a NaN dampedAngularFreq — producing NaN outputs for every frame.

A guard like if (!(springOptions.damping >= 0)) springOptions.damping = springDefaults.damping would cover undefined, NaN, and negative damping (note: >= 0 rather than > 0 because zero damping is a valid underdamped spring). The new test suite would benefit from a damping: undefined case as well.

Comment on lines +225 to +230

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 The comment says "non-positive or non-finite" but Infinity satisfies Infinity > 0, so an Infinity stiffness or mass slips through the guard. With stiffness = Infinity, undampedAngularFreq = Math.sqrt(Infinity / mass) = Infinity, and A = (velocity + 0 * Infinity * delta) / Infinity = NaN, still producing NaN. Using isFinite alongside the positivity check closes this gap.

Suggested change
if (!(springOptions.stiffness > 0)) {
springOptions.stiffness = springDefaults.stiffness
}
if (!(springOptions.mass > 0)) {
springOptions.mass = springDefaults.mass
}
if (!(springOptions.stiffness > 0) || !isFinite(springOptions.stiffness)) {
springOptions.stiffness = springDefaults.stiffness
}
if (!(springOptions.mass > 0) || !isFinite(springOptions.mass)) {
springOptions.mass = springDefaults.mass
}


return springOptions
}

Expand Down