Closed Bug 1376495 Opened 7 years ago Closed 7 years ago

stylo: make clip-path, shape-outside animatable

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(1 file, 4 obsolete files)

Bug 1376495: Add expected value on traistion test for Servo.
59 bytes, text/x-review-board-request
boris
: review+
Details
See Also: → 1289049
Daisuke, what happens if we try to interpolate these properties now in Stylo and Gecko? Do we get discrete animation in Gecko and nothing in Stylo?
Flags: needinfo?(dakatsuka)
(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #1)
> Daisuke, what happens if we try to interpolate these properties now in Stylo
> and Gecko? Do we get discrete animation in Gecko and nothing in Stylo?

Hi Brian! 
Both clip-path and shape-outside should animate not only discretely, but also on <basic-shape>.
We already implemented clip-path completely, but shape-outside supports discrete animation only ( bug 1289049 ) in Gecko.
Stylo, unfortunately, not support animation at all yet.
Flags: needinfo?(dakatsuka)
Thanks. So we probably want to make this at least discretely animatable in order to avoid changing behavior when we ship Stylo.
Priority: -- → P3
Assignee: nobody → dakatsuka
clip-path and shape-outside in servo use pretty similar types, so I did suggest daisuke to make them animatable together when we discussed this on IRC. Also, at that time, I did ask TYLin, who is the implementer of shape-outside, whether we need to modify gecko side when we make shape-outside animatable, he thinks it should work as it is.
Blocks: 1387952
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> clip-path and shape-outside in servo use pretty similar types, so I did
> suggest daisuke to make them animatable together when we discussed this on
> IRC. Also, at that time, I did ask TYLin, who is the implementer of
> shape-outside, whether we need to modify gecko side when we make
> shape-outside animatable, he thinks it should work as it is.

Making both animatable sounds good, but 'clip-path' blocks test_transitions_per_property.html, so I think if this takes much time, it's better to fix clip-path first.
(In reply to Boris Chiou [:boris] from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > clip-path and shape-outside in servo use pretty similar types, so I did
> > suggest daisuke to make them animatable together when we discussed this on
> > IRC. Also, at that time, I did ask TYLin, who is the implementer of
> > shape-outside, whether we need to modify gecko side when we make
> > shape-outside animatable, he thinks it should work as it is.
> 
> Making both animatable sounds good, but 'clip-path' blocks
> test_transitions_per_property.html, so I think if this takes much time, it's
> better to fix clip-path first.

Thanks, Boris!
Okay. I'm fixing this bug now, it looks we can implement both at the same time since the data structure is almost same.
However, if face to some problems, will fix 'clip-path' first.
Blocks: 1388601
Ah, I wasn't aware of this ticket and I already started working on it yesterday. :/
(In reply to Anthony Ramine [:nox] from comment #7)
> Ah, I wasn't aware of this ticket and I already started working on it
> yesterday. :/

Oh,, I almost implemented..
(In reply to Daisuke Akatsuka (:daisuke) from comment #8)
> (In reply to Anthony Ramine [:nox] from comment #7)
> > Ah, I wasn't aware of this ticket and I already started working on it
> > yesterday. :/
> 
> Oh,, I almost implemented..

Same, hah. I also refactored code and moved it around to abstract things more between clip-path and shape-outside. We can compare once we push our code.
Hi nox!
I pushed my patches to reviewboard.

Now, I'm looking at some test fails in layout/style/test_transitions_per_property.html
Those cause looks a difference the serialization both Gecko and Servo.
e.g. 
Servo: circle(500px at 500px 500px)
Gecko( expected value in the test ): circle(500px at calc(500px + 0%) calc(500px + 0%))

May we modify the test code?

Thanks!
I compared your code to mine and I don't think I forgot anything.

https://github.com/servo/servo/pull/18035
Thank you very much, nox!
I'll make clip-path and shape-outside animatable after introduce the your code.
(In reply to Daisuke Akatsuka (:daisuke) from comment #17)
> Thank you very much, nox!
> I'll make clip-path and shape-outside animatable after introduce the your
> code.

My PR takes care of it, but I probably forgot some compute_distance and whatnot (which is what bug 1388601) is about. Feel free to extract these from your patches once my PR lands and I'll review them ASAP.
My PR landed, in the end I added all the compute_distance methods while I was at it, feel free to tell me if there are remaining failures.
I did confirm that some test cases for clip-path in dom/smil/test/test_smilCSSFromTo.xhtml passed locally (the test file has been skipped on our CI for now) on autoland.  Thank you nox!
Attachment #8895720 - Attachment is obsolete: true
Attachment #8895721 - Attachment is obsolete: true
Attachment #8895722 - Attachment is obsolete: true
Attachment #8895723 - Attachment is obsolete: true
Comment on attachment 8895719 [details]
Bug 1376495: Add expected value on traistion test for Servo.

https://reviewboard.mozilla.org/r/167042/#review173228

Looks good to me. Thanks for updating this test.
Attachment #8895719 - Flags: review?(boris.chiou) → review+
Thanks, Boris.

For now, although shape-outside animate discretely, we should animate on <basic-shape> same as clip-path.
However, we got test fails in test_transition_per_property.html if we make shape-outside animatable on <basic-shape>.
One of the reason is the serialization of calc as same as clip-path.
Another one looks like font size resolving.
e.g.  
INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | transitions not supported for property shape-outside value polygon(20rem 20em) - got "polygon(20px 20px)", expected "polygon(320px 400px)"

I'll file another bug for this problem, then please let me close this bug. 

Thanks.
See Also: → 1390026
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b061ddd698e2
Add expected value on traistion test for Servo. r=boris
https://hg.mozilla.org/mozilla-central/rev/b061ddd698e2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: