Fix issue 1450

[mattgperry]

CSS Motion Path properties (offsetDistance, offsetPath, offsetRotate, offsetAnchor) should remain as CSS styles and not be converted to SVG attributes. This fixes the issue where animating offsetDistance on SVG elements would incorrectly render as an HTML attribute rather than a CSS style property.

Fixes #1450

[claude]

Code Review

Summary

This PR fixes issue #1450 by ensuring CSS Motion Path properties (offsetDistance, offsetPath, offsetRotate, offsetAnchor) remain as CSS styles on SVG elements rather than being incorrectly converted to SVG attributes.

✅ Strengths

1. Correct Fix: The implementation correctly identifies that CSS Motion Path properties must remain in the style object for SVG elements, following the same pattern already established for transform and transformOrigin.

2. Comprehensive Test Coverage: The tests cover:

   • Basic case with offsetDistance and offsetPath
   • All four CSS Motion Path properties together
   • Different SVG element types (circle, rect)

3. Consistent Pattern: The fix follows the existing code pattern in build-attrs.ts (lines 47-63) where transform-related CSS properties are moved from attrs to style.

4. Clear Documentation: The comment "CSS Motion Path properties should remain as CSS styles" clearly explains the intent.

🔍 Observations

1. Array Iteration: Using a for-of loop over an array of property names is clean and maintainable.

2. Type Safety: The code uses attrs[key] which relies on TypeScript's index signature. This is fine but the type system won't catch typos in the property names array.

💡 Suggestions

Minor: Consider using const assertion for better type safety

While the current implementation works, you could add a const assertion for slightly better type safety:

const CSS_MOTION_PATH_PROPERTIES = [
    "offsetDistance",
    "offsetPath", 
    "offsetRotate",
    "offsetAnchor"
] as const

However, this is purely a style preference and the current implementation is perfectly fine.

Consider: E2E Test Coverage

The unit tests look great, but you might want to consider adding a Playwright E2E test similar to the ones in tests/effects/svg.spec.ts. This would verify the fix works end-to-end in a real browser environment. For example:

test("applies offsetDistance as CSS style, not attribute", async ({ page }) => {
    await page.goto("effects/offset-distance.html")
    const circle = page.locator("#circle")
    const offsetDistance = await circle.evaluate((el) =>
        getComputedStyle(el).getPropertyValue("offset-distance")
    )
    expect(offsetDistance).toBeTruthy()
    const offsetDistanceAttr = await circle.getAttribute("offset-distance")
    expect(offsetDistanceAttr).toBeNull()
})

This would require creating a corresponding HTML test file, but would provide more confidence that the fix works in real-world scenarios.

✅ No Issues Found

• Performance: No performance concerns - the loop only runs when these properties are present
• Security: No security concerns
• Breaking Changes: This is a bug fix that corrects incorrect behavior, not a breaking change
• Code Style: Follows the repository conventions (named exports, arrow functions, strict equality)

Recommendation

Approve with optional enhancements. The core fix is solid and the tests are good. The E2E test suggestion is optional but would add extra confidence.

[claude]

Pull Request Review

Summary

This PR fixes issue #1450 where CSS Motion Path properties (offsetDistance, offsetPath, offsetRotate, offsetAnchor) were incorrectly being converted to SVG attributes instead of remaining as CSS styles when animating SVG elements.

---

Code Quality and Best Practices

Strengths:

• Follows established patterns: The implementation mirrors the existing approach for transform, transformOrigin, and transformBox properties (lines 47-62)
• Clear documentation: The comment on lines 65-67 clearly explains why these properties need special handling
• Adheres to code style: Uses interface pattern, strict equality, and proper formatting per CLAUDE.md guidelines
• Comprehensive test coverage: Two well-designed tests cover both individual and combined motion path properties

Suggestions:

1. Consider refactoring for DRY principle: Lines 69-84 are repetitive. A loop or helper function would reduce code duplication and make it easier to add/remove properties in the future.

2. TypeScript type safety: The current implementation does not validate that these properties exist on the types. Consider whether type guards or explicit type definitions would prevent future type errors.

---

Correctness and Bug Fixes

Analysis:

• Root cause correctly identified: SVG rendering converts all properties to attributes by default (line 39), but CSS Motion Path properties must remain as CSS styles
• Solution is sound: Moving these properties back to the style object (like transform properties) ensures browsers apply them correctly
• Matches W3C spec: CSS Motion Path Module Level 1 defines these as CSS properties, not SVG attributes

No bugs identified in the implementation.

---

Test Coverage

Comprehensive testing:

• Test 1 (lines 81-99): Validates offsetDistance and offsetPath remain in style object
• Test 2 (lines 102-125): Validates all four motion path properties work together
• Uses existing test patterns: Follows the structure of other useSVGProps tests
• Tests different SVG elements: Uses circle and rect to ensure element type does not matter

Potential enhancement:
Consider adding a test with MotionValue inputs (similar to test on line 65-79) to ensure motion values for offset properties work correctly.

---

Performance Considerations

No performance concerns:

• Adds 4 conditional checks (O(1) operations)
• Minimal overhead compared to existing transform property handling
• No loops or expensive operations
• Properties are only processed when present

---

Security Concerns

No security issues identified:

• Properties are moved between objects, not evaluated or executed
• No user input is directly inserted into DOM
• Follows same pattern as existing secure code for transform properties

---

Additional Notes

1. Browser compatibility: CSS Motion Path is supported in modern browsers (Chrome 46+, Firefox 72+, Safari 15.4+). Users on older browsers may not see animations, but this will not cause errors.

2. Documentation: Consider adding an example to the documentation showing how to use offsetDistance with SVG elements, as this was a pain point for users (see issue [BUG] Offset Distance on SVG elements are placed in the wrong place. #1450).

3. Related properties: The W3C CSS Motion Path Module also defines offsetPosition. Consider whether this should be included now or tracked for future enhancement.

---

Recommendation

Approve with minor suggestion: The fix is correct, well-tested, and follows established patterns. The only enhancement would be refactoring lines 69-84 to reduce repetition, but this is a minor point and not blocking.

Great work addressing this bug! The fix properly distinguishes CSS-only properties from SVG attributes, matching browser behavior and W3C specifications.

---

Files reviewed:

• packages/framer-motion/src/render/svg/utils/build-attrs.ts
• packages/framer-motion/src/render/svg/tests/use-props.test.ts

[claude]

Code Review

Thank you for addressing issue #1450! This is a well-crafted fix for CSS Motion Path properties on SVG elements. Here's my detailed review:

✅ Strengths

1. Correct Root Cause Fix: The solution properly identifies that CSS Motion Path properties (offsetDistance, offsetPath, offsetRotate, offsetAnchor) should remain as CSS styles rather than being converted to SVG attributes.

2. Consistent Pattern: The implementation follows the same pattern already established for transform and transformOrigin properties in build-attrs.ts (lines 57-73), ensuring consistency within the codebase.

3. Comprehensive Test Coverage: The new tests cover both individual properties and all four CSS Motion Path properties together, validating the expected behavior thoroughly.

4. Clean Implementation: The code is simple, readable, and performant - using a straightforward loop to move properties from attrs to style.

🔍 Observations & Considerations

1. Early Return for SVG Tags

The fix correctly applies only to SVG element tags (not the <svg> container itself), since the early return on line 42 (if (isSVGTag)) means this code only runs for SVG elements like <circle>, <rect>, <path>, etc.

2. Potential Type Safety Enhancement (Minor)

While not blocking, consider whether TypeScript types need updating to explicitly include CSS Motion Path properties in the relevant type definitions. The current implementation works, but explicit typing could improve IDE autocomplete and type checking.

3. Property Iteration Performance (Very Minor)

The for...of loop over the array is fine for 4 items and matches the codebase style. This is not a performance concern given it runs once per render cycle for affected elements.

4. Test Coverage Gap (Enhancement Opportunity)

The tests validate the expected output but don't explicitly verify that:

• Properties are actually deleted from attrs (though this is implied)
• Behavior when properties have undefined values
• Interaction with the isSVGTag early return path

Consider adding a test like:

test("should not process CSS motion path properties for SVG tags", () => {
    const { result } = renderHook(() =>
        useSVGProps(
            { style: {} } as any,
            { offsetDistance: "50%" },
            false,
            "svg" // SVG tag, not SVG element
        )
    )
    
    // Should not have offsetDistance in style for <svg> tags
    expect(result.current.style).toStrictEqual({})
})

🛡️ Security & Performance

• No security concerns: The fix only moves properties between objects without introducing any new attack vectors.
• Performance impact: Negligible - adds a simple loop that runs once per render.
• No breaking changes: This fix corrects incorrect behavior, making previously broken functionality work correctly.

📋 Recommendations

1. ✅ Approve and merge - The core fix is solid and solves the reported issue correctly.
2. 🔧 Consider adding a test for the <svg> tag case to ensure CSS Motion Path properties aren't incorrectly handled for the container element.
3. 📝 Optional enhancement: Add JSDoc comments explaining why these specific properties need this treatment (referencing the CSS Motion Path spec).

Verdict

LGTM with minor suggestions! 🎉

This PR successfully fixes the issue where CSS Motion Path properties were incorrectly rendered as SVG attributes. The implementation is clean, follows established patterns, and includes good test coverage. The minor suggestions above are enhancements, not blockers.

Great work addressing this bug! 🚀

[claude]

Code Review

Summary

This PR fixes a bug where CSS Motion Path properties (offsetDistance, offsetPath, offsetRotate, offsetAnchor) were incorrectly being converted to SVG attributes instead of remaining as CSS styles when animating SVG elements.

Positive Findings

1. Correct Approach
The fix correctly identifies that CSS Motion Path properties should remain as CSS styles rather than being converted to SVG attributes. The implementation follows the same pattern already established for transform and transformOrigin (lines 57-73 in build-attrs.ts).

2. Comprehensive Test Coverage
The tests cover:

• Basic use case with offsetDistance and offsetPath
• All four CSS Motion Path properties together
• Different SVG element types (circle, rect)

3. Clear Documentation
The code includes a helpful comment explaining why these properties need special handling.

4. Minimal and Focused
The PR makes only the necessary changes without over-engineering.

Code Quality

Implementation Pattern Consistency
The implementation correctly follows the established pattern in build-attrs.ts:

1. Properties initially placed in attrs by buildHTMLStyles()
2. Moved to style for non-SVG-tag elements
3. Removed from attrs

Logic Flow Analysis
The placement of CSS Motion Path handling (lines 75-80) is correct:

• Runs AFTER buildHTMLStyles() populates initial values
• Runs ONLY when NOT isSVGTag (early return at line 42-46)
• Runs BEFORE SVG path rendering logic

This ensures motion path properties work for SVG child elements (circle, rect, etc.) but not the root svg tag itself.

Performance & Security

Performance: No concerns

• Single loop over fixed 4-element array
• Only runs when properties present
• No memory leaks or unnecessary object creation

Security: No concerns

• No user input parsing
• No injection vulnerabilities
• Simple property assignment

Recommendations

1. Approve and merge - This is a clean, well-tested fix
2. Consider adding a test case for root svg element (optional)
3. After merge, consider updating documentation with CSS Motion Path examples for SVG

Final Assessment

Status: APPROVED

This PR successfully fixes issue #1450 with:

• Correct implementation following established patterns
• Comprehensive test coverage
• No performance or security concerns
• Minimal, focused changes

Great work! The code quality is high and the approach is sound.

---

Reviewed with Claude Code