1486094 - Make path() animatable

Status: RESOLVED FIXED

(Core :: CSS Transitions and Animations, enhancement, P3)

Description

[Boris Chiou [:boris]]

Now we support SVG path for <offset-path> and <clip-path> properties on HTML elements. The interpolation of SVGPathData is not implemented yet, and according to the spec [1], the SVG path should be animatable. So here, let's make it animatable.

[1] https://www.w3.org/TR/SVG/paths.html#DAttribute

Comment 1

[Boris Chiou [:boris]]

About the discussion of the serialization and the interpolation: https://github.com/w3c/svgwg/issues/321

Comment 2

[Boris Chiou [:boris]]

In order to implement the interpolation of svg path, there is a discussion about the format and should we promote the svg path to a more general format? (https://github.com/w3c/svgwg/issues/321). The issue is not resolved yet, but we still could implement the interpolation by some limitations.

That said:

1. We normalize both the start and end svg paths to absolute instead of relative coordinates.
2. We don't do any promotion for now. (If the spec updates and want to do promotions, let's fill a bug to fix it.)

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1486094 - Part 1: Make SVGPathData and |clip-path:path()| animatable.

Implement Animate trait for SVGPathData. ToAnimatedZero trait of
SVGPathData returns Err(()) because it doesn't make sense to get a zero
value of an SVG Path. We will implement ComputeSquaredDistance for
SVGPathData in a later patch.

The basic idea is: we normalize |this| and |other| svg paths, and then
do interpolation on the normalized svg paths. The normalization is to
convert relative coordinates into absolute coordinates, so we could do
real number interpolation on each path command directly. For the flag in
Arc, we follow the way what wpt wants, i.e. Set true if the interpolated
result is >= 0.5.

Comment 4

[Boris Chiou [:boris]]

Attachment: Bug 1486094 - Part 2: Make offset-path:path() animatable.

Here, we change the animation type of offset-path as CompputedValue, so
we could do animation on it. Also enable the wpt for offset-path
interpolation. In test_transition_per_property.html, we add some basic tests
for offset-path.

Depends on D4786

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1486094 - Part 4: Write tests for ComputeSquaredDistance on SVGPathData.

Use the same logic as other basic shapes to implement
ComputeSquaredDistance for SVGPathData, and add tests for it.

Depends on D4787

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1486094 - Part 3: Add reftests for interpolations of offset-path and clip-path.

In order to make sure the rendering result is correct, we add some
reftests here.

Depends on D4787

Comment 7

[Boris Chiou [:boris]]

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

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1486094 - Part 5: Use the standalone struct and enum for the flags in SVG path.

Therefore, it would be easier to use Animate trait for PathCommand and
drop a lot redundant code.

Depends on D4788

Comment 9

[Robert Longson [:longsonr]]

I think we should push back on the flag discrete/continuous issue rather than just accept it.

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 9005703 [details]
Bug 1486094 - Part 3: Add reftests for interpolations of offset-path and clip-path.

Brian Birtles (:birtles) has approved the revision.

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 9005699 [details]
Bug 1486094 - Part 4: Write tests for ComputeSquaredDistance on SVGPathData.

Brian Birtles (:birtles) has approved the revision.

Comment 12

[Boris Chiou [:boris]]

(In reply to Robert Longson [:longsonr] from comment #9)
> I think we should push back on the flag discrete/continuous issue rather
> than just accept it.

I see. Probably I will follow the current implementation of SMIL interpolation. Let bug 653928 keeps following this problem.

Comment 13

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris] from comment #12)
> (In reply to Robert Longson [:longsonr] from comment #9)
> > I think we should push back on the flag discrete/continuous issue rather
> > than just accept it.
> 
> I see. Probably I will follow the current implementation of SMIL
> interpolation. Let bug 653928 keeps following this problem.

I'm having trouble understanding the different variations here. For these boolean flags like sweep etc. it seems like there are three behaviors:

a) 50% flip behavior (i.e. typical discrete behavior) on the flag. This is what Blink does and WPT expects.
b) Interpolate as float, treat anything >= 0 as true (similar to 'visibility'). This is what the SVG2 spec defines.
c) If the boolean values don't match, treat the whole path as not interpolable and fall back to discrete animation.

In bug 653928 we tried to change SMIL from (c) to (b) but jwatt was uncomfortable with it and we decided to back it out.

Boris, Robert, is that right?

Robert, which of these are you suggesting we push for?

Comment 14

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #13)
> I'm having trouble understanding the different variations here. For these
> boolean flags like sweep etc. it seems like there are three behaviors:
> 
> a) 50% flip behavior (i.e. typical discrete behavior) on the flag. This is
> what Blink does and WPT expects.
> b) Interpolate as float, treat anything >= 0 as true (similar to
> 'visibility'). This is what the SVG2 spec defines.
> c) If the boolean values don't match, treat the whole path as not
> interpolable and fall back to discrete animation.
> 
> In bug 653928 we tried to change SMIL from (c) to (b) but jwatt was
> uncomfortable with it and we decided to back it out.
> 
> Boris, Robert, is that right?

(a) and (b) are what I thought. For (c), I checked the current implementation [1], it seem we only treat the "flag" as discrete animation, and other parameters of the arc type are real numbers and animatable, right? Maybe Daniel could also give us some advises.

[1] https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/dom/svg/SVGPathSegListSMILType.cpp#190-207

Comment 15

[Brian Birtles (:birtles)]

Oh, so (c) and (a) are actually the same then I guess. And SMIL does (a)?

If that's the case, it might be simplest to just get SVG2 updated.

(Generally I think the 50% flip behavior is rarely useful but it sounds like Jonathan has analyzed this case more carefully and concluded otherwise.)

Comment 16

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #15)
> Oh, so (c) and (a) are actually the same then I guess. And SMIL does (a)?
> 
> If that's the case, it might be simplest to just get SVG2 updated.
> 
> (Generally I think the 50% flip behavior is rarely useful but it sounds like
> Jonathan has analyzed this case more carefully and concluded otherwise.)

Gecko SMIL is very similar to (a). It flips the flag from the beginning of the animation (directly copy), instead of 50% progress. Blink does (a) exactly because they convert the bool into float (0.0 or 1.0), and do interpolation on it, and then convert it back to bool by comparing it with "0.5" [1]. So maybe SVG2 really need to be updated.

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/animation/svg_path_seg_interpolation_functions.cc?rcl=d1d3afd487e42a0d159049ee5133f6be1be81dd0&l=212

Comment 17

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris] from comment #16)
> (In reply to Brian Birtles (:birtles) from comment #15)
> > Oh, so (c) and (a) are actually the same then I guess. And SMIL does (a)?
> > 
> > If that's the case, it might be simplest to just get SVG2 updated.
> > 
> > (Generally I think the 50% flip behavior is rarely useful but it sounds like
> > Jonathan has analyzed this case more carefully and concluded otherwise.)
> 
> Gecko SMIL is very similar to (a). It flips the flag from the beginning of
> the animation (directly copy), instead of 50% progress.

I don't understand. So it always just jumps directly to the end value? Like a kind of step-start animation for that value? (If so, that sounds wrong.)

Comment 18

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #17)
> I don't understand. So it always just jumps directly to the end value? Like
> a kind of step-start animation for that value? (If so, that sounds wrong.)

Oh. Sorry, please ignore my comments. I misread the code. We only do the interpolation if the flags are match. In other words, your (c) is correct. We don't do interpolation if the boolean values don't match.

Comment 19

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #18)
> (In reply to Brian Birtles (:birtles) from comment #17)
> > I don't understand. So it always just jumps directly to the end value? Like
> > a kind of step-start animation for that value? (If so, that sounds wrong.)
> 
> Oh. Sorry, please ignore my comments. I misread the code. We only do the
> interpolation if the flags are match. In other words, your (c) is correct.
> We don't do interpolation if the boolean values don't match.

That means (c) -> (b) was backed out. Probably this is an acceptable solution for now because it seems mismatch flags makes it harder to use.

Comment 20

[Boris Chiou [:boris]]

Filed issue, https://github.com/w3c/svgwg/issues/543, to track the behavior of the interpolation of mismatched flags in arc.

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 9005784 [details]
Bug 1486094 - Part 5: Use the standalone struct and enum for the flags in SVG path.

Brian Birtles (:birtles) has approved the revision.

Comment 22

[Chris Mills [:cmills]]

Check whether our data lists this as animatable.

Comment 23

[Brian Birtles (:birtles)]

Comment on attachment 9005698 [details]
Bug 1486094 - Part 2: Make offset-path:path() animatable.

Brian Birtles (:birtles, PTO until 14 Sep) has approved the revision.

Comment 24

[Brian Birtles (:birtles)]

Comment on attachment 9005696 [details]
Bug 1486094 - Part 1: Make SVGPathData and |clip-path:path()| animatable.

Brian Birtles (:birtles, PTO until 14 Sep) has approved the revision.

Comment 25

[Brian Birtles (:birtles)]

I'm satisfied that the spec issues are filed and are proceeding. However, I really want to make sure we sort them out and fix them before shipping (and preferably soon while there is some momentum here and Blink folks are engaged).

As I understand it the issues are:

* Are paths made absolute at computed value time? i.e. reflected in getComputedStyle even when there is no animation?
  https://github.com/w3c/svgwg/issues/321
* Are path functions serialized with double quotes?
  Also https://github.com/w3c/svgwg/issues/321 (see issue #320 too)
* How are mismatched boolean flags handled? 50% flip for value? command? or path?
  https://github.com/w3c/svgwg/issues/543

We really need to make sure the spec, WPT, and Gecko implementation all agree here and then do what we can to get Blink to match.

Comment 26

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, PTO until 14 Sep) from comment #25)
> I'm satisfied that the spec issues are filed and are proceeding. However, I
> really want to make sure we sort them out and fix them before shipping (and
> preferably soon while there is some momentum here and Blink folks are
> engaged).
> 
> As I understand it the issues are:
> 
> * Are paths made absolute at computed value time? i.e. reflected in
> getComputedStyle even when there is no animation?
>   https://github.com/w3c/svgwg/issues/321
> * Are path functions serialized with double quotes?
>   Also https://github.com/w3c/svgwg/issues/321 (see issue #320 too)
> * How are mismatched boolean flags handled? 50% flip for value? command? or
> path?
>   https://github.com/w3c/svgwg/issues/543
> 
> We really need to make sure the spec, WPT, and Gecko implementation all
> agree here and then do what we can to get Blink to match.

Thanks, Brian. For now, there is one further issue we have. We fall back to discrete animations if the flags don't match, and we do normalization during animation time, so what kind of path format should we output in this case? That is, should getComputedStyle(div).offsetPath always return the path with absolute coordinates or just return the corresponding computed value if we fall back to discrete animations?

e.g.
path("M 100 100 A ... 0 0 200 200") -> path("M 120 120 a ... 1 1 150 150") at 75%,
the output should be:
a) path("M 120 120 a ... 1 1 150 150") // relative coordinates
or
b) path("M 120 120 A ... 1 1 270 270") // absolute coordinates

Our implementation returns (a) for now because we directly copy the computed values for discrete animations. However, this issue could be fixed after we decide to do normalization at computed time. Let's keep tracking these issues.

Comment 27

[Brian Birtles (:birtles)]

Yes, I think ultimately getComputedStyle should always return absolute coordinates. (In fact, I'd gladly accept a patch for that.) If you want to land this now and fix it in a separate bug though that's fine.

Comment 28

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, PTO until 14 Sep) from comment #27)
> Yes, I think ultimately getComputedStyle should always return absolute
> coordinates. (In fact, I'd gladly accept a patch for that.) If you want to
> land this now and fix it in a separate bug though that's fine.

Thanks. I'd like to fix that in a follow-up bug.

Comment 29

[Boris Chiou [:boris]]

Comment on attachment 9005784 [details]
Bug 1486094 - Part 5: Use the standalone struct and enum for the flags in SVG path.

Emilio, if you don't have any other concern on this patch. I'd like to land it soon. Thanks.

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9005784 [details]
Bug 1486094 - Part 5: Use the standalone struct and enum for the flags in SVG path.

Ah, sorry! Phabricator wasn't showing me this patch, because I'm not tagged as a 'blocking' reviewer. You can do that putting an exclamation mark after my name from `arc` and such, fwiw.

Will review in Phab, thanks!

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9005784 [details]
Bug 1486094 - Part 5: Use the standalone struct and enum for the flags in SVG path.

Emilio Cobos Álvarez (:emilio) has approved the revision.

Comment 32

[Boris Chiou [:boris]]

hehe, let me try to lando this patch series.
https://lando.services.mozilla.com/D4813/

Comment 33

[Boris Chiou [:boris]]

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

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de616e78d8cd
Part 1: Make SVGPathData and |clip-path:path()| animatable. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d806177c1b72
Part 2: Make offset-path:path() animatable. r=birtles
https://hg.mozilla.org/integration/autoland/rev/e48a78215b63
Part 3: Add reftests for interpolations of offset-path and clip-path. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d4ac69c6dab5
Part 4: Write tests for ComputeSquaredDistance on SVGPathData. r=birtles
https://hg.mozilla.org/integration/autoland/rev/7395025cecec
Part 5: Use the standalone struct and enum for the flags in SVG path. r=emilio,birtles

Comment 35

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

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

Comment 36

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 37

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/de616e78d8cd
https://hg.mozilla.org/mozilla-central/rev/d806177c1b72
https://hg.mozilla.org/mozilla-central/rev/e48a78215b63
https://hg.mozilla.org/mozilla-central/rev/d4ac69c6dab5
https://hg.mozilla.org/mozilla-central/rev/7395025cecec

Comment 38

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

Upstream PR merged

Comment 39

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #25)
> * Are paths made absolute at computed value time? i.e. reflected in
> getComputedStyle even when there is no animation?
>   https://github.com/w3c/svgwg/issues/321

We agree to make the path absolute for both specified and computed value. (Bug 1489392)

> * Are path functions serialized with double quotes?
>   Also https://github.com/w3c/svgwg/issues/321 (see issue #320 too)

Both blink and gecko agree and use double quotes now.

> * How are mismatched boolean flags handled? 50% flip for value? command? or
> path?
>   https://github.com/w3c/svgwg/issues/543

RESOLUTION: Flipping boolean and integers half way for path interpolation. I filed bug 1491954.

Comment 40

[Chris Mills [:cmills]]

Note to docs team:

I've added a note covering this to the Fx64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#CSS

I think mainly we need to update the compat data to specify that path()s are now animatable, but at the same time take a look at the clip-path page — it doesn't seem to mention path() as a syntax option?

Comment 41

[Chris Mills [:cmills]]

>> take a look at the clip-path page — it doesn't seem to mention path() as a syntax option?

This is preffed off for now, but we still ought to investigate whether it is available in other browsers.

Comment 42

[Boris Chiou [:boris]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #41)
> >> take a look at the clip-path page — it doesn't seem to mention path() as a syntax option?
> 
> This is preffed off for now, but we still ought to investigate whether it is
> available in other browsers.

It seems we only have an example about clip-path: path() in MDN. path() is defined in a very new spec, css-shapes-2, and IIRC, only firefox supports it for now.

Comment 43

[Sebastian Zartner [:sebo]]

On https://developer.mozilla.org/en-US/docs/Web/CSS/offset-path there's also just an example for animating offset-distance, which is not implemented yet. So  there is no page yet showing how to animate this function.

What's actually needed is a separate page for the path() function, i.e. https://developer.mozilla.org/en-US/docs/Web/CSS/path resp. https://developer.mozilla.org/en-US/docs/Web/CSS/path, like for other CSS functions.

Sebastian

Comment 44

[aarthi0808]

From Mozilla version 64, svg paths used in offsetpath or clip-path are animatable. (If transition applied in svg paths)
But is there any way to off the transition for clip paths?

Comment 45

[Boris Chiou [:boris]]

(In reply to aarthi0808 from comment #44)

From Mozilla version 64, svg paths used in offsetpath or clip-path are animatable. (If transition applied in svg paths)
But is there any way to off the transition for clip paths?

Unfortunately, there is no extra preference to disable the transition on |clip-path:path()|. We only have a preference to determinate if the path() function could be used on clip-path (i.e. layout.css.clip-path-path.enabled), which is disabled by default.

Comment 46

[Chris Mills [:cmills]]

Changing to dev-doc-complete — path() is defined on the basic-shape page — https://developer.mozilla.org/en-US/docs/Web/CSS/basic-shape, which also shows an example of it being animated.