Fixing conversion between css vars

[mattgperry]

Closes #3439
Fixes #3410

[claude]

Code Review - PR #3442

Thank you for this fix! This PR effectively addresses issue #3410 by ensuring animations from calc() expressions containing CSS variables to simple values resolve correctly through proper measurement detection.

✅ Strengths

1. Excellent test coverage: Both E2E (Cypress) and unit test components added

   • Clear test case in dev/react/src/tests/unit-conversion-var-to-simple.tsx
   • Comprehensive E2E test in packages/framer-motion/cypress/integration/unit-conversion.ts:7-23

2. Minimal, targeted fix: The changes are focused and don't over-engineer the solution

   • New containsCSSVariable() helper has a clear, single purpose
   • Surgical check added at DOMKeyframesResolver.ts:90-100

3. Good documentation: Clear comments explaining the fix references issue [BUG] translateY animation incorrectly preserves CSS variables from calc() expressions #3410

4. Follows repository conventions:

   • Named exports
   • Clear function documentation
   • Consistent code style

---

🔍 Issues & Suggestions

1. Potential Logic Redundancy (Minor - DOMKeyframesResolver.ts:97)

if (originHasVar \!== targetHasVar && positionalValues[name]) {
    this.needsMeasurement = true
    return
}

Issue: The check positionalValues[name] is redundant here because:

• Line 82 already verifies positionalKeys.has(name)
• Line 97-100 is only reached if that check passed
• The logic is correct but could be cleaner

Suggestion: Remove the redundant check for clarity:

if (originHasVar \!== targetHasVar) {
    this.needsMeasurement = true
    return
}

2. Comment Handling in containsCSSVariable (Enhancement)

The current implementation:

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(--")
}

Good: You correctly strip comments using split("/*")[0] to avoid false positives like "calc(100% /* var(--x) */ + 10px)". This matches the pattern from isCSSVariableToken on line 25.

Suggestion: Consider adding a test case for this edge case to ensure the comment handling works correctly.

3. Test Timing (Minor - unit-conversion.ts:15)

// Component transition duration: 0.05s (50ms)
transition={{ duration: 0.05 }}

// Test wait time: 200ms
.wait(200)

The wait time is 4x the animation duration. While this ensures completion:

• Consider documenting why 200ms is chosen (accounting for render delays, requestAnimationFrame, etc.)
• Or use a more precise wait: .wait(100) might be sufficient

4. Missing Edge Case Tests (Enhancement)

Consider adding tests for:

• Reverse direction: Simple value → calc with var (e.g., 0 → calc(100px + var(--offset)))
• Multiple variables: calc(var(--a) + var(--b)) → 0
• Both have variables: calc(var(--a)) → calc(var(--b)) (should they still measure?)
• Nested calc: calc(calc(100% - var(--x)) / 2) → 50px

These would strengthen confidence that the fix handles all variations correctly.

5. Type Safety (Very Minor - is-css-variable.ts:36-42)

The function signature is good, but consider the AnyResolvedKeyframe type - does it include all value types you expect? The current implementation handles non-strings correctly but documenting the expected input types would help future maintainers.

---

🔒 Security & Performance

Security: ✅ No concerns

• No XSS or injection vulnerabilities introduced
• Comment stripping prevents malicious comment-based exploits

Performance: ✅ Excellent

• containsCSSVariable() uses efficient string operations (includes())
• Only triggers expensive measurement when necessary
• Early return pattern optimized

---

📋 Code Style Compliance

✅ Follows CLAUDE.md conventions:

• Uses interface for types (not shown here but consistent with codebase)
• Named exports only
• Arrow functions where appropriate
• Clear documentation comments

---

🎯 Summary

This is a solid, well-tested fix that properly addresses the reported bug. The implementation is minimal, performant, and follows best practices. The suggestions above are minor improvements rather than blockers.

Recommendation: ✅ Approve - This PR is ready to merge with optional follow-up enhancements.

Nice work on the fix! The test-first approach and targeted solution demonstrate good engineering practices.

[claude]

Code Review

Great fix for issue #3410! The solution correctly identifies when measurement is needed for animations involving CSS variables. Here's my detailed feedback:

✅ Strengths

1. Targeted Solution: The fix correctly addresses the root cause by detecting when one keyframe contains CSS variables and the other doesn't, triggering measurement to resolve them to pixels.

2. Good Test Coverage: Added both a Cypress E2E test and a comprehensive unit test suite for containsCSSVariable. The E2E test validates the full user interaction flow.

3. Clear Documentation: Comments at DOMKeyframesResolver.ts:90-92 and is-css-variable.ts:32-35 clearly explain the purpose and behavior.

4. Follows Conventions: Code adheres to repository style (uses interface, arrow functions, named exports).

🔍 Observations & Suggestions

1. Edge Case: Simple Value to Simple Value

Location: DOMKeyframesResolver.ts:97-100

The current logic only triggers measurement when originHasVar !== targetHasVar. This correctly handles:

• ✅ calc(100px + var(--x)) → 0 (measurement triggered)
• ✅ 0 → calc(100px + var(--x)) (measurement triggered)

But what about:

• ❓ calc(100px + var(--x)) → calc(50px + var(--y)) (both have vars, no measurement)

This might be fine if the animation system can handle it, but worth considering if different CSS variables could cause issues.

2. Comment Stripping Logic

Location: is-css-variable.ts:41

The comment stripping uses split("/*")[0], which works but has a subtle limitation:

// Current implementation
return value.split("/*")[0].includes("var(--")

Potential issues:

• Doesn't handle */ closing comments - a string like "100px */ var(--x) /*" would incorrectly return true
• This is extremely unlikely in real-world usage, but the test at line 30 suggests awareness of comment edge cases

Suggestion: Consider if the existing approach is sufficient or if a more robust regex would be better:

return value.replace(/\/\*[^*]*\*+(?:[^/*][^*]*\*+)*\//g, '').includes("var(--")

However, for performance reasons and given the rarity of comments in animation values, the current simple approach is probably fine.

3. Test Comment Accuracy

Location: __tests__/is-css-variable.test.ts:30

The test description says "ignores CSS variables in comments" but only tests the edge case where there's no actual var(--) outside the comment. Consider adding a test like:

test("detects CSS variables outside comments", () => {
    expect(containsCSSVariable("var(--real) /* var(--commented) */")).toBe(true)
})

This would verify the function works correctly when there ARE real variables mixed with commented ones.

4. Makefile Change

Location: Makefile:98

The change to test-single target is for local development convenience. This is fine, but remember to document or communicate that this is a "last edited test" convenience rather than a permanent choice.

🐛 Potential Issues

None identified - the logic appears sound for the stated use case.

⚡ Performance Considerations

1. Measurement Performance: The fix correctly identifies when measurement is needed, avoiding unnecessary measurements when both keyframes have the same CSS variable status. This is optimal.

2. String Operations: containsCSSVariable uses simple string operations (split + includes) which are fast. No performance concerns here.

🔒 Security Concerns

None - The code doesn't introduce any security vulnerabilities. It's purely internal logic for animation resolution.

📋 Test Coverage

✅ Excellent:

• E2E test covering the full user interaction (click → animate → click → animate back)
• Unit tests covering all code paths:
  • Non-string inputs
  • Strings without variables
  • Standalone variable tokens
  • Variables in calc expressions
  • Comments edge case

🎯 Recommendations

1. Minor: Add a test case for CSS variables both inside and outside comments
2. Optional: Consider the edge case of animating between two different CSS variables
3. Nice-to-have: Document the comment-stripping behavior more explicitly if you expect it to matter

Summary

This is a solid, well-tested fix that correctly addresses the reported issue. The solution is minimal, focused, and doesn't over-engineer. The test coverage is thorough. I'd approve this PR with confidence.

Rating: ✅ Approve with minor suggestions for consideration

[claude]

Code Review

Thank you for this fix! This PR properly addresses issue #3410 by ensuring animations from calc() expressions with CSS variables to simple values resolve correctly through measurement.

---

✅ Strengths

1. Well-targeted fix: The change correctly identifies when measurement is needed - when one keyframe contains CSS variables and the other doesn't, triggering the measurement system to resolve both to concrete pixel values.

2. Good test coverage:

   • E2E test with bidirectional animation (0 → calc → 0)
   • Unit tests for the new containsCSSVariable() helper
   • Test file follows conventions from CLAUDE.md

3. Clear documentation: Comments reference GitHub [BUG] translateY animation incorrectly preserves CSS variables from calc() expressions #3410 in both test and implementation files.

4. Minimal implementation: The new containsCSSVariable() helper is simple and efficient, stripping comments to avoid false positives.

5. Follows repository conventions: Uses named exports, clear comments, and appropriate test patterns.

---

🔍 Issues & Suggestions

1. Redundant check in DOMKeyframesResolver.ts:97 (Minor)

if (originHasVar !== targetHasVar && positionalValues[name]) {
    this.needsMeasurement = true
    return
}

Issue: The positionalValues[name] check is redundant here. We already know name is a positional key from the guard at line 82 (if (!positionalKeys.has(name))), and the subsequent check at line 118 already handles the case where positionalValues[name] exists.

Suggestion: The check is defensive but unnecessary. The logic is correct as-is, but could be simplified to:

if (originHasVar !== targetHasVar) {
    this.needsMeasurement = true
    return
}

This matches the pattern at line 122 where positionalValues[name] is checked again for the general case.

2. Test timing could be more precise (Minor)

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

// packages/framer-motion/cypress/integration/unit-conversion.ts:18,23,28
.wait(200)

The animation duration is 50ms but tests wait 200ms (4× longer). While this provides a safe buffer, consider:

• Reducing wait time to 100 for faster tests
• Or documenting why 200ms is needed (e.g., accounting for frame timing, React render cycles)

3. Consider additional edge case tests (Enhancement)

The current test covers the main case well. Consider adding tests for:

• Reverse direction: Simple value → calc with var (e.g., 0 → calc(100px + var(--offset)))
• Multiple variables: calc(var(--a) + var(--b)) → 0
• Nested expressions: calc(calc(100% - var(--x)) / 2) → 50px

These would ensure the fix handles all variants of the bug.

4. containsCSSVariable could be more defensive (Very Minor)

Current implementation:

return value.split("/*")[0].includes("var(--")

Edge case: Multi-line comments with */ on different line could theoretically cause issues, though unlikely in practice. The current approach is pragmatic and matches isCSSVariableToken.

Alternative (if being extra defensive):

return value.replace(/\/\*[\s\S]*?\*\//g, '').includes("var(--")

But the current implementation is likely fine for real-world usage.

---

🔒 Security & Performance

Security: ✅ No concerns. String operations are safe, no injection vectors.

Performance: ✅ Excellent. The containsCSSVariable() check uses efficient string operations (split + includes), and properly limits expensive measurement operations to only when needed.

---

📋 Code Style Compliance

✅ Follows CLAUDE.md conventions:

• Uses interface for types
• Named exports
• Clear comments referencing issue
• Consistent code style

---

🎯 Summary

This is a solid, well-tested fix that correctly handles the edge case where CSS variables in calc() expressions need measurement for proper interpolation. The implementation is clean and follows repository patterns.

Minor suggestions:

1. Remove redundant positionalValues[name] check at line 97
2. Consider reducing test wait times or documenting why 200ms is needed
3. Consider adding edge case tests for comprehensive coverage

Recommendation: ✅ Approve - The fix is correct and well-implemented. The suggestions above are minor optimizations that could be addressed in a follow-up if desired.

Great work addressing this tricky animation edge case! 🚀

[claude]

Code Review

I've reviewed PR #3442 which fixes the issue of animating between calc() values containing CSS variables and numerical values (issue #3410).

✅ Strengths

1. Correct Root Cause Analysis: The fix correctly identifies that when one keyframe contains embedded CSS variables (e.g., in calc()) and the other doesn't, the animation system needs to measure the DOM to convert values to pixels for proper interpolation.

2. Well-Tested: The PR includes:

   • A comprehensive E2E test that validates the roundtrip animation (0 → calc → 0)
   • Extensive unit tests for the new containsCSSVariable() function covering edge cases
   • The test correctly verifies the bug is fixed by checking that the element returns to position 0 after the roundtrip

3. Minimal, Focused Solution: The fix is surgical - adding just the necessary check in DOMKeyframesResolver.ts:97-100 to trigger measurement when CSS variable presence differs between keyframes.

4. Good Code Quality:

   • Clear JSDoc comments explaining the purpose
   • The containsCSSVariable() function has appropriate type guards
   • Comment stripping prevents false positives from CSS comments

🔍 Observations & Suggestions

1. Comment Stripping Consistency

The containsCSSVariable() function strips comments using value.split("/*")[0], which is consistent with isCSSVariableToken(). However, this only handles block comments starting with /* and doesn't handle closing */ or multiple comment blocks. For example:

"100px /* comment */ var(--foo) /* another */" // Would incorrectly return true

Suggestion: While this edge case is unlikely in practice, consider documenting this limitation or using a more robust comment-stripping approach if this becomes an issue.

2. Performance Consideration

The new check at DOMKeyframesResolver.ts:97-100 triggers needsMeasurement = true, which forces DOM measurements. This is correct for fixing the bug, but it means animations between calc() and simple values will be slightly slower due to the measurement phase.

This is acceptable because:

• Correctness > performance for this edge case
• The performance impact only affects animations mixing CSS variables with non-variable values
• The alternative (incorrect animations) is worse

3. Type Safety

The containsCSSVariable() function has good type checking, but consider if it should handle all AnyResolvedKeyframe types or just strings. Currently, it returns false for numbers/null/undefined, which is correct, but the parameter type AnyResolvedKeyframe | null might be too broad.

Current behavior is fine - the type guard properly narrows to string checks.

4. Test Coverage

The E2E test is excellent, but consider adding a test case for:

• Animating from a simple value to calc() (reverse direction)
• Multiple roundtrips to ensure no accumulation errors
• Different CSS variables in origin vs target (e.g., calc(var(--a)) → calc(var(--b)))

Not blocking - current tests adequately verify the fix.

🐛 Potential Issues

Minor: Makefile Test Spec Update

The Makefile change updates the test spec to unit-conversion.ts, which appears to be for local development/debugging. This seems intentional but:

-test-single: build test-mkdir
-\tyarn start-server-and-test "yarn dev-server" http://localhost:9991 "cd packages/framer-motion && cypress run --config-file=cypress.react-19.json --headed --spec cypress/integration/layout-group.ts"
+test-single: build test-mkdir  
+\tyarn start-server-and-test "yarn dev-server" http://localhost:9991 "cd packages/framer-motion && cypress run --config-file=cypress.react-19.json --headed --spec cypress/integration/unit-conversion.ts"

Question: Should this Makefile change be committed, or is it just for development? If it's meant to stay, that's fine, but if it's accidental, it might confuse future developers about which test make test-single should run.

🔒 Security

No security concerns. The change doesn't introduce any vulnerabilities:

• No user input is processed unsafely
• Comment stripping prevents injection attacks via CSS comments
• Type guards prevent unexpected value types

📊 Overall Assessment

Recommendation: ✅ APPROVE

This is a well-crafted fix that:

• ✅ Correctly addresses the root cause
• ✅ Includes comprehensive tests
• ✅ Follows the repository's conventions (interfaces, named exports, arrow functions)
• ✅ Has clear documentation
• ✅ Makes the minimal necessary changes

The only minor question is about the Makefile change - if that's intentional, this PR is ready to merge!

---

Nice work 🎉 This fix will prevent a confusing animation bug that was preserving CSS variables incorrectly.

[claude]

Pull Request Review

Thanks for this fix! The changes effectively address issue #3410 where animations between calc() values with CSS variables and simple values were incorrectly preserving the CSS variable structure.

Positives

1. Clear Problem Identification - The fix correctly identifies the root cause when animating between values where one contains CSS variables and the other doesn't
2. Elegant Solution - The containsCSSVariable() function is a clean abstraction that checks for CSS variables embedded anywhere in a value
3. Good Test Coverage - Unit tests cover key cases and E2E test validates the roundtrip behavior
4. Follows Code Style - Uses interface types and named exports per repository conventions

Suggestions

1. Logic Consideration (DOMKeyframesResolver.ts:97) - The check triggers measurement when exactly one value has CSS variables. Consider whether the case where BOTH values contain CSS variables needs handling, or add a comment explaining the XOR logic.

2. Performance - The containsCSSVariable() function uses split on every call. Consider only splitting if the string contains comments as a micro-optimization.

3. Test Coverage - The E2E test validates roundtrip but doesn't test the original issue's exact case: calc(-100% - var(--myVar)) to 0% with translateY. Consider adding this test case.

4. Makefile Change - The test-single target now points to unit-conversion.ts. Should this be reverted to a default test file?

Security & Performance

No security concerns. The performance impact is negligible as the check only runs for positional values with exactly 2 keyframes.

Overall: Approve with minor suggestions. Great work on fixing this tricky edge case!

[claude]

Code Review

Thank you for this fix! I've reviewed the changes and they look solid. Here's my detailed feedback:

✅ Strengths

1. Root Cause Identified: The fix correctly identifies that animations between calc() expressions with CSS variables and simple values need measurement to resolve properly.

2. Well-Tested: The PR includes comprehensive test coverage:

   • New unit tests for containsCSSVariable() with good edge cases
   • New E2E test that validates the roundtrip scenario (0 → calc → 0)
   • Tests verify both computed position and style attribute preservation

3. Minimal & Focused: The fix is surgical - it adds the new check in exactly the right place in DOMKeyframesResolver.ts without over-engineering.

4. Good Abstractions: The new containsCSSVariable() function is properly separated from isCSSVariableToken() with clear documentation explaining the difference.

🔍 Detailed Analysis

Core Fix (DOMKeyframesResolver.ts:90-100)

const originHasVar = containsCSSVariable(origin)
const targetHasVar = containsCSSVariable(target)

if (originHasVar \!== targetHasVar && positionalValues[name]) {
    this.needsMeasurement = true
    return
}

Logic: When one keyframe contains a CSS var and the other doesn't, force measurement. This is correct because:

• calc(3 * var(--width)) → 0 requires measuring the computed pixel value
• Without measurement, the animation would incorrectly preserve the CSS var structure

New Function (is-css-variable.ts:36-42)

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

Good:

• Handles non-string values gracefully
• Strips comments to avoid false positives (consistent with isCSSVariableToken)
• Simple and efficient implementation

Minor Consideration: The function uses includes("var(--") which could theoretically match "var(--" in a string literal, but this is an acceptable edge case given the context.

E2E Test (unit-conversion.ts:7-44)

The test properly validates:

1. Initial state at 0
2. Animation to calc() expression (300px)
3. Roundtrip back to 0 - this is the critical regression test
4. Style attribute preservation vs clearing

Excellent: Uses DOMMatrix to parse computed transform values rather than regex parsing - robust approach.

🎯 Minor Suggestions

1. Type Safety: In DOMKeyframesResolver.ts:97, consider adding a type check:

if (originHasVar \!== targetHasVar && positionalValues[name]) {

The positionalValues[name] check is good, but this could be clearer with a comment explaining it filters for properties that need measurement (x, y, width, height, etc.).

2. Test Coverage: The unit tests for containsCSSVariable() are comprehensive, but could add one more edge case:

test("handles multiple var() instances", () => {
    expect(containsCSSVariable("calc(var(--a) + var(--b))")).toBe(true)
})

Note: This is already tested, so this is not critical.

3. Documentation: The inline comment at line 91-92 is excellent. Consider also noting that this fixes issue [BUG] translateY animation incorrectly preserves CSS variables from calc() expressions #3410 in the code comment for future reference.

🔒 Security & Performance

• No security concerns: The changes handle user input (CSS values) safely
• Performance: The containsCSSVariable() check is O(n) on string length but only runs for 2-keyframe animations, so negligible impact
• No regressions: The fix only triggers measurement when needed (mixed CSS var usage), preserving existing optimizations

📝 Code Style Compliance

Checked against CLAUDE.md:

• ✅ Uses interface where appropriate (not applicable here)
• ✅ Named exports used
• ✅ Arrow functions for callbacks
• ✅ Strict equality (===)
• ✅ No var declarations

✅ Final Verdict

Approved - This is a high-quality fix that:

• Solves the reported bug (issue [BUG] translateY animation incorrectly preserves CSS variables from calc() expressions #3410)
• Includes comprehensive tests
• Maintains code quality standards
• Has no performance or security concerns
• Follows the repository's conventions

The implementation is correct and ready to merge. Great work! 🎉