Closed Bug 1486094 Opened 6 years ago Closed 6 years ago

Make path() animatable

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

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
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
See Also: → 1486646
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.)
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.
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
Use the same logic as other basic shapes to implement
ComputeSquaredDistance for SVGPathData, and add tests for it.

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

Depends on D4787
Attachment #9005699 - Attachment description: Bug 1486094 - Part 3: Implement ComputeSquaredDistance for SVGPathData. → Bug 1486094 - Part 4: Implement ComputeSquaredDistance for SVGPathData.
See Also: → 653928
Therefore, it would be easier to use Animate trait for PathCommand and
drop a lot redundant code.

Depends on D4788
I think we should push back on the flag discrete/continuous issue rather than just accept it.
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.
Attachment #9005703 - Flags: review+
Comment on attachment 9005699 [details]
Bug 1486094 - Part 4: Write tests for ComputeSquaredDistance on SVGPathData.

Brian Birtles (:birtles) has approved the revision.
Attachment #9005699 - Flags: review+
Attachment #9005699 - Attachment description: Bug 1486094 - Part 4: Implement ComputeSquaredDistance for SVGPathData. → Bug 1486094 - Part 4: Write tests for ComputeSquaredDistance on SVGPathData.
(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.
(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?
(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
Flags: needinfo?(dholbert)
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.)
(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
(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.)
(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.
(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.
Flags: needinfo?(dholbert)
Blocks: 1488868
Attachment #9005696 - Attachment description: Bug 1486094 - Part 1: Make SVGPathData animatable. → Bug 1486094 - Part 1: Make SVGPathData and |clip-path:path()| animatable.
Attachment #9005784 - Attachment description: Bug 1486094 - Part 5: Use a standalone struct for EllipticalArc. → Bug 1486094 - Part 5: Use a standalone struct for the flags in EllipticalArc.
Filed issue, https://github.com/w3c/svgwg/issues/543, to track the behavior of the interpolation of mismatched flags in arc.
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.
Attachment #9005784 - Flags: review+
Check whether our data lists this as animatable.
Keywords: dev-doc-needed
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.
Attachment #9005698 - Flags: review+
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.
Attachment #9005696 - Flags: review+
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.
(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.
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.
(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.
Blocks: 1489392
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.
Attachment #9005784 - Flags: review?(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!
Attachment #9005784 - Flags: review?(emilio)
Attachment #9005784 - Attachment description: Bug 1486094 - Part 5: Use a standalone struct for the flags in EllipticalArc. → Bug 1486094 - Part 5: Use the standalone struct and enum for the flags in SVG path.
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.
Attachment #9005784 - Flags: review+
hehe, let me try to lando this patch series.
https://lando.services.mozilla.com/D4813/
Pushed by boris.chiou@gmail.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12905 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Blocks: 1491954
(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.
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?
>> 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.
(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.
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

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?

(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.

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.

Blocks: 1582554
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: