Fix animating between calc() values and numerical values

[mattgperry]

Fixes #3410

[claude]

Code Review

Thank you for this well-structured fix! This PR properly addresses issue #3410 by ensuring animations from calc() expressions with CSS variables to simple values resolve correctly. The implementation follows TDD principles and is clean overall.

✅ Strengths

1. Excellent test-first approach: The PR follows the TDD workflow documented in CLAUDE.md:

   • Added failing test component (unit-conversion-var-to-simple.tsx)
   • Added Cypress E2E test to verify the bug
   • Implemented minimal fix
   • Tests now pass

2. Clear documentation: Great comments explaining the issue (GitHub [BUG] translateY animation incorrectly preserves CSS variables from calc() expressions #3410 references) in both test and implementation files.

3. Minimal, targeted fix: The changes are focused on the specific problem without over-engineering:

   • New containsCSSVariable() helper with clear purpose
   • Surgical check in DOMKeyframesResolver.ts:99-104

4. Good test coverage: Added both E2E test and unit test for mixComplex edge case.

---

🔍 Issues & Suggestions

1. Logic Issue in DOMKeyframesResolver.ts:99-104 (Minor)

The condition checks for type equality but then triggers measurement when CSS variable presence differs. However, the logic should verify that measurement is actually needed for the specific animation:

// packages/motion-dom/src/animation/keyframes/DOMKeyframesResolver.ts:99-104
if (
    positionalValues[name] &&
    containsCSSVariable(origin) \!== containsCSSVariable(target)
) {
    this.needsMeasurement = true
}

Issue: The check positionalValues[name] verifies if a conversion function exists, but we already know name is a positional key (from line 82). This check is redundant in this context.

Suggestion: The logic is correct but could be clearer:

// When types match but CSS variable presence differs, we need to measure
// to resolve the calc() expression to a concrete value.
if (containsCSSVariable(origin) \!== containsCSSVariable(target)) {
    this.needsMeasurement = true
}

2. containsCSSVariable Implementation (Minor - Edge Case)

The current implementation:

export function containsCSSVariable(
    value?: AnyResolvedKeyframe | null
): boolean {
    return typeof value === string && value.includes("var(--")
}

Potential edge case: This would match comments in CSS strings like "calc(100% /* var(--x) */ + 10px)". While unlikely in practice, consider using the same approach as isCSSVariableToken:

export function containsCSSVariable(
    value?: AnyResolvedKeyframe | null
): boolean {
    if (typeof value \!== "string") return false
    // Strip comments to avoid false positives
    return value.split("/*")[0].includes("var(--")
}

3. Test: Immediate Fallback Behavior (Question)

The unit test in mix-complex.test.ts:65-81 verifies that mismatched var counts trigger mixImmediate (instant transition):

const mixer = mixComplex("calc(100% + var(--offset))", "0")
expect(mixer(0.5)).toBe("0") // Instant transition at 0.5

Question: Is this the intended UX? With the measurement system triggered by the fix in DOMKeyframesResolver.ts, shouldn't the animation smoothly interpolate between the measured values rather than instantly switching?

The flow should be:

1. needsMeasurement = true triggers measurement
2. measureInitialState() measures calc(100% + var(--offset)) → concrete value (e.g., 150px)
3. Animation interpolates 150px → 0px smoothly

If this is expected: Consider adding a comment explaining why measurement doesn't apply in this code path (perhaps this is for cases where measurement wasn't triggered).

4. Test Duration Mismatch (Minor)

// dev/react/src/tests/unit-conversion-var-to-simple.tsx:20
transition={{ duration: 0.05 }}

// packages/framer-motion/cypress/integration/unit-conversion.ts:16
.wait(200)

Duration is 0.05s (50ms) but test waits 200ms. While this ensures the animation completes, consider using a more precise wait or documenting why 200ms is chosen (e.g., accounting for render delays).

5. Missing Edge Case Tests

Consider adding tests for:

• Animating from simple value → calc with var (reverse direction)
• Multiple CSS variables in calc: calc(var(--a) + var(--b)) → 0
• Nested calc: calc(calc(100% - var(--x)) / 2) → 50px

---

🔒 Security & Performance

Security: ✅ No concerns. The changes don't introduce XSS, injection, or other vulnerabilities.

Performance: ✅ Good. The containsCSSVariable() check uses efficient string operations. Setting needsMeasurement = true appropriately limits expensive measurement operations to when necessary.

---

📋 Code Style

✅ Follows repository conventions:

• Uses interface (per ESLint rules)
• Named exports
• Clear comments
• Arrow functions where appropriate

---

🎯 Summary

This is a solid fix with good test coverage. The main suggestion is to clarify the test behavior around mixImmediate fallback vs. measured interpolation. Overall, this PR is in good shape! 🚀

Recommendation: Approve with minor clarifications on the test expectations.