1388652 - stylo: list-style-type is not animatable yet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

The test cases are filtered out by isSupported() check somehow, so the failures have not bee n appeared.

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Daisuke, could you please check where there are other properties that are filtered out?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388652 - Drop AnimatedProperty for stylo.

set_list_style_type() for gecko needs an additional argument 'Device' [1], and
making list-style-type animtable makes AnimatedProperty::update() call the
set_list_style_type(). To avoid introducing the Device argument to the setter in
servo, this patch makes simply PropertyAnimation conditional build.
(Whereas in stylo, we don't use the setter for animation, we use clone_xx()
instead.)

[1] https://hg.mozilla.org/mozilla-central/file/1d042bcb2632/servo/components/style/properties/properties.mako.rs#l2667

Review commit: https://reviewboard.mozilla.org/r/166452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166452/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1388652 - Make list-style-type animatable

Review commit: https://reviewboard.mozilla.org/r/166454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166454/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8895275 [details]
Bug 1388652 - Drop AnimatedProperty for stylo.

https://reviewboard.mozilla.org/r/166452/#review171598

Well, I guess. Adding the extra argument isn't a huge deal either, and maybe it'd be preferrable... The less conditionally compiled stuff the better, but anyway it's fine :).

Comment 5

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8895276 [details]
Bug 1388652 - Make list-style-type animatable

https://reviewboard.mozilla.org/r/166454/#review171600

Comment 6

[Daisuke Akatsuka (:daisuke)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Daisuke, could you please check where there are other properties that are
> filtered out?

OHH,, Okay!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/mozilla-central/rev/1ba045f4b9f4

Comment 8

[Daisuke Akatsuka (:daisuke)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Daisuke, could you please check where there are other properties that are
> filtered out?

Currently, all filtered properties are logical properties. Since they are not animatable yet.

However, font-variation-settings[1] property is not register in the list, also does not support the animation.
(Gecko defines as eStyleAnimType_Custom)
Also, flex-box is not register as well, and does not implement for Servo yet[2]. 

I'll file a bug for font-variation-settings at least, since looks there is not such the bug.

[1] https://drafts.csswg.org/css-fonts-4/#font-variation-settings-def
[2] https://searchfox.org/mozilla-central/source/servo/components/style/properties/longhand/position.mako.rs#177

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > Daisuke, could you please check where there are other properties that are
> > filtered out?
> 
> Currently, all filtered properties are logical properties. Since they are
> not animatable yet.
> 
> However, font-variation-settings[1] property is not register in the list,
> also does not support the animation.
> (Gecko defines as eStyleAnimType_Custom)
> Also, flex-box is not register as well, and does not implement for Servo
> yet[2]. 

I think you mean flex-basis.
https://searchfox.org/mozilla-central/source/servo/components/style/properties/longhand/position.mako.rs#165