878346 - Make SVG transform="" a presentation attribute (map it into the style property)

Status: RESOLVED FIXED

(Core :: SVG, task)

Description

[Cameron McCormack (:heycam)]

In SVG 2, the trasnform="" attribute has become a presentation attribute for the transform property.

https://svgwg.org/svg2-draft/coords.html#TransformProperty

Comment 2

[Louis Chang [:louis]]

Attachment: svg-transform.svg

Comment 3

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09b2b66f124c65b4beaee8f82a444ac98296a02b

Comment 4

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41e66d9a0c6199359d67834f83301cfa9710ff89

Comment 5

[Louis Chang [:louis]]

Attachment: [WIP] Bug 878346 - make transform="" a presentation attribute for the transform property

Comment 6

[Louis Chang [:louis]]

Attachment: additive-1.svg

Comment 7

[Louis Chang [:louis]]

The patch in comment 5 seems to fix the test case svg-transform.svg but also breaks some animation reftests (see try results in comment 4). And I simplify one of the test cases additive-1.svg, and seems that transform translate does work but I have no idea why the animation is broken...

Comment 8

[Louis Chang [:louis]]

Hi Brian,

Comment 9

[Louis Chang [:louis]]

(In reply to Louis Chang [:louis] from comment #8)
> Hi Brian,

Seems you are working on the animation, could you help me out with the problem mentioned in comment 7?

Comment 10

[Louis Chang [:louis]]

(In reply to Louis Chang [:louis] from comment #7)
> The patch in comment 5 seems to fix the test case svg-transform.svg but also
> breaks some animation reftests (see try results in comment 4). And I
> simplify one of the test cases additive-1.svg, and seems that transform
> translate does work but I have no idea why the animation is broken...

Note that if I remove the transform="translate(50)" in path element, the animation works again.

Comment 11

[Brian Birtles (:birtles)]

(In reply to Louis Chang [:louis] from comment #7)
> The patch in comment 5 seems to fix the test case svg-transform.svg but also
> breaks some animation reftests (see try results in comment 4). And I
> simplify one of the test cases additive-1.svg, and seems that transform
> translate does work but I have no idea why the animation is broken...

I haven't looked into this in any detail, but I imagine promoting `transform` to a presentation attribute involves converting it to a mapped attribute including:

* Adding it to SVGTransformableElement::IsAttributeMapped
* Making the SVGAnimatedTransformList's animVal and baseVal attributes look/modify up the mapped-attribute style.
  animVal should technically only return the SMIL-animated style, but I suppose it would be ok to make it equivalent to `getComputedStyle(elem).transform`. baseVal should return the style specified on the attribute (as opposed to simply the un-animated style).
  We'll possibly deprecate animVal at some time but we really should do that for all interfaces at once, not piecemeal. I notice SVG2 still specifies it.
* Making <animateTransform> update the corresponding mapped-attribute style.
* Removing some of the special handling for SVG transforms (see uses of nsIFrame::IsSVGTransformed in graphics and animation code)

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles) from comment #11)
>   We'll possibly deprecate animVal at some time but we really should do that
> for all interfaces at once, not piecemeal. I notice SVG2 still specifies it.
> * Making <animateTransform> update the corresponding mapped-attribute style.

The current SVG 2 spec text says that the SVGAnimatedTransformList's baseVal and animVal should reflect the presentation attribute value.  That is part of the wider decision to get rid of animVal's functionality, but with lesser breakage risk than removing the animVal property altogether.  If we did that, I think it would simplify the work in this bug, since we wouldn't have to worry about correctly exposing and updating a DOMSVGTransformList to script that represents the current computed value of the transform property.

So what do you think about doing that first?  Do you know what the status of animVal is other browsers?

Comment 13

[Brian Birtles (:birtles)]

That sounds ok to me. I don't know what the status is of other browsers. (My impression is no-one's really touching their SVG code but, like us, they'll make these changes when they're in the neighborhood.)

Comment 14

[Louis Chang [:louis]]

I will not work on this bug for now since it's out of my knowledge...
Feel free to take over the bug if anyone want to solve it.

Comment 15

[Louis Chang [:louis]]

Attachment: ref_testcase

Comment 19

[Arnaud Brousseau]

I recently ran into the fact that CSS transforms aren't working in the context of SVG in Firefox and I'd be willing to take a crack at this bug if nobody is looking already.

I'll start by following Brian's comment above (comment #11) -- I'm assuming it's still relevant/accurate?

Comment 20

[Brian Birtles (:birtles)]

CSS transforms work on SVG elements, but the 'transform' attribute doesn't map to the CSS 'transform' property.

I'm not aware of anyone looking at this so by all means please have a look. Thank you!

Comment 21

[Arnaud Brousseau]

Thanks for the quick response! And sorry for being imprecise. What I meant is that CSS's "transition: transform Xs" doesn't work in the context of SVG in Firefox. That's how I got to this bug in the first place :)

Here's a minimal repro to demonstrate what I'm talking about: https://repl.it/@arnaudbrousseau/bugzilla878346 (this correctly triggers animations in Chrome 71, but doesn't in Firefox 64). I'll have a look at what I can do to fix this!

Comment 22

[Brian Birtles (:birtles)]

> What I meant is that CSS's "transition: transform Xs" doesn't work in the context of SVG in Firefox.

It does work, but only of you're setting the transform values using the CSS property, rather than the SVG attribute.

> Here's a minimal repro to demonstrate what I'm talking about: https://repl.it/@arnaudbrousseau/bugzilla878346

If you just make this set the style instead it will work: https://repl.it/repls/FoolhardyWarlikeCharacterencoding

(But that said, we still need to fix this bug so that authors can mix-and-match setting the attribute and property as per your original test case.)

Comment 23

[Arnaud Brousseau]

> If you just make this set the style instead it will work: https://repl.it/repls/FoolhardyWarlikeCharacterencoding
Nice! That's good info, thanks for taking the time to put this together!

> we still need to fix this bug so that authors can mix-and-match setting the attribute and property
Indeed!

Comment 24

[Brian Birtles (:birtles)]

> Do you know what the status of animVal is other browsers?

Looking at this pen:

  https://codepen.io/birtles/pen/pqVPPr

It seems like everyone (Firefox Nightly, Chrome Dev, Safari TP) still supports animVal for SMIL content. i.e. no-one has bit the bullet and make animVal reflect the base value yet.

Comment 25

[Cameron McCormack (:heycam)]

Given that, it's worth considering just making the presentation attribute change and deferring the SVGAnimatedTransformList changes to another bug.

Note that it's not as simple as just adding transform as a mapped attribute, since per spec we need to parse the presentation attribute with the SVG transform syntax (e.g. translate values have no units).

Comment 26

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #25)

Given that, it's worth considering just making the presentation attribute change and deferring the SVGAnimatedTransformList changes to another bug.

Note that it's not as simple as just adding transform as a mapped attribute, since per spec we need to parse the presentation attribute with the SVG transform syntax (e.g. translate values have no units).

We already parse SVG presentation attributes with ParsingMode::AllowUnitlessLengths right?

Comment 27

[Cameron McCormack (:heycam)]

Yes, you're right. Though I think there may be other syntax differences we need to support, like not requiring commas between values.

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 6 duplicates.
:jwatt, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 31

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 878346 - Make transform a mapped attribute for SVG. r=longsonr,#style

The tricky bit is rotate() which in SVG means something different if
there's an origin (you translate-then-untranslate it).

But this seems to work off-hand, and fix the reminder of bug 1906261.

Comment 34

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #12)

The current SVG 2 spec text says that the SVGAnimatedTransformList's baseVal
and animVal should reflect the presentation attribute value. That is part
of the wider decision to get rid of animVal's functionality, but with lesser
breakage risk than removing the animVal property altogether. If we did
that, I think it would simplify the work in this bug, since we wouldn't have
to worry about correctly exposing and updating a DOMSVGTransformList to
script that represents the current computed value of the transform property.

So what do you think about doing that first? Do you know what the status of
animVal is other browsers?

Link to the spec text:

https://www.w3.org/TR/SVG2/coords.html#InterfaceSVGAnimatedTransformList

Comment 35

[nezriffic]

Hey Team, any progress with it?

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/246ed0e1a934
Make transform a mapped attribute for SVG. r=longsonr,firefox-style-system-reviewers,zrhoffman

Comment 37

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/47717 for changes under testing/web-platform/tests

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76f787922e86
Annotate a macOS pass.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7535b5db51e4
Skip a crashtest that now OOMs in win32 but isn't really a new regression.

Comment 40

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/246ed0e1a934
https://hg.mozilla.org/mozilla-central/rev/76f787922e86
https://hg.mozilla.org/mozilla-central/rev/7535b5db51e4

Comment 41

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed05ca77bac3
Disable 387290-1.svg on Android 7 for causing almost perma failure a=test-only CLOSED TREE

Comment 43

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ed05ca77bac3