fix transform motion svg-attribute conflicts with style-transform#3628
fix transform motion svg-attribute conflicts with style-transform#3628malo0n wants to merge 2 commits intomotiondivision:mainfrom
Conversation
Greptile SummaryThis PR fixes a real bug (issue #3082) where SVG However, there are two meaningful concerns introduced by the supporting changes:
Minor: the import added at the top of Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["SVG MotionValue prop\ne.g. transform=mv"] --> B{Pipeline}
B --> C["Effects pipeline\nsvgEffect / addSVGValue"]
B --> D["Render pipeline\nbuildSVGAttrs"]
C --> E{"isSVGAnimatedProperty\nelement has baseVal?"}
E -- yes --> F["addAttrValue\nsetAttribute"]
E -- no --> G{"key in element.style?"}
G -- yes --> H["addStyleValue\nelement.style key = v"]
G -- no --> F
D --> I["buildHTMLStyles\nsets state.style.transform"]
I --> J["state.attrs = state.style\nstate.style = {}"]
J --> K{"attrs.transform\ntruthy?"}
K -- no --> N["No transform set"]
K -- yes --> L{"latest.transform\n=== undefined?"}
L -- "yes: motion-synthesized\nx/y/scale/rotate" --> M["style.transform = attrs.transform\nCSS transform applied"]
L -- "no: user-supplied\ntransform value" --> O["attrs.transform kept\nSVG attribute applied"]
Last reviewed commit: 7ef68ce |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function isSVGAnimatedProperty(element: Element, name: string): boolean { | ||
| const value = (element as any)[name] | ||
|
|
||
| return ( | ||
| value !== null && | ||
| typeof value === "object" && | ||
| "baseVal" in value | ||
| ) | ||
| } |
There was a problem hiding this comment.
isSVGAnimatedProperty is too broad — affects width, height, cx, cy, r, etc.
The check "baseVal" in value is true for any SVGAnimated* interface, not just SVGAnimatedTransformList. This means width, height, cx, cy, r, rx, ry, x, y (all of which return SVGAnimatedLength), viewBox (SVGAnimatedRect), etc., will all satisfy this predicate.
The effect is that in addSVGValue (in effects/svg/index.ts), the new early-return for isSVGAnimatedProperty now intercepts all of these geometry properties and routes them to addAttrValue (setAttribute), bypassing the existing key in element.style check that would previously route them through addStyleValue. For example, animating width on a <rect> as a MotionValue would now be set as an SVG attribute instead of a CSS property, which is a silent behavior change from the original code.
If the intent is only to fix transform, consider narrowing the check. For example:
export function isSVGTransformProperty(element: Element, name: string): boolean {
if (name !== "transform") return false
const value = (element as any)[name]
return (
value !== null &&
typeof value === "object" &&
"baseVal" in value
)
}Or, accept that all SVGAnimated* geometry attributes should go through setAttribute (and verify this is intentional).
| if (attrs.transform) { | ||
| style.transform = attrs.transform | ||
| delete attrs.transform | ||
| if (latest.transform === undefined) { | ||
| style.transform = attrs.transform | ||
| delete attrs.transform | ||
| } | ||
| } |
There was a problem hiding this comment.
latest.transform === undefined breaks CSS-transform path when using animate={{ transform: ... }}
The outer condition if (attrs.transform) will be entered any time attrs.transform is set (i.e., whenever a transform value exists in the render state). The inner guard latest.transform === undefined is meant to distinguish between a synthesized transform (from x/y/scale/rotate props, where latest.transform is undefined) and an explicit user-supplied transform value.
However, consider the scenario where someone uses animate={{ transform: "translate(50px, 0)" }}. In this case:
latestValues.transform = "translate(50px, 0)"(truthy), sobuildHTMLStylesskips synthesizing a transform, and instead setsstyle.transform = "translate(50px, 0)"- After
state.attrs = state.style,attrs.transform = "translate(50px, 0)" latest.transformis"translate(50px, 0)"(notundefined)- The new code skips the
style.transform = attrs.transformassignment, leavingattrs.transformas an SVG attribute
For SVG child elements (non-<svg> tags), a CSS transform applied via the animate prop would previously have been moved to style.transform (enabling CSS transitions and transform-box: fill-box). With this change, it instead stays as an SVG attribute. This may or may not be desirable, but it is an undocumented behavioral change for animate={{ transform: ... }} on SVG elements.
It would be good to add a test covering animate={{ transform: "..." }} to confirm the intended behavior.
…issing pw test for transform svg-attribute
|
Thanks for the PR! Though handled this through #3629 |
2 participants
This pull request addresses a bug where SVG attributes like
transformcould incorrectly render as[object Object]when using MotionValues, and ensures that SVG-animated properties are handled correctly in the DOM. It introduces detection for SVGAnimated* properties, updates attribute handling logic, and adds tests to prevent regressions. The changes improve compatibility with SVG and prevent incorrect rendering of MotionValue objects.fixes #3082
SVG attribute handling improvements:
isSVGAnimatedPropertyto detect SVGAnimated* properties (e.g.,transform), ensuring they are set viasetAttributeinstead of as properties, which prevents rendering issues.addSVGValueandcanSetAsPropertylogic to useisSVGAnimatedProperty, ensuring SVG-animated properties are handled as attributes, not as DOM properties. [1] [2] [3]transforminbuildSVGAttrsto ensure user-provided SVG transform attributes override motion-synthesized transforms and prevent leaking MotionValue objects into the DOM.Testing and reliability:
[object Object], and that animated MotionValues update SVG attributes as expected.Minor fixes:
convertAttrKeyfor better Unicode handling.It's my first contribution in open-source project ever, so please let me know if smth wrong! I've tried to follow all recommendations