1353918 - stylo: make all animatable properties *animatable*

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P5)

Description

[Hiroyuki Ikezoe (:hiro)]

There are some properties that have been implemented but not yet animatable.
http://searchfox.org/mozilla-central/search?q=FIXME%3A.*animatable&case=false&regexp=true&path=*.rs

This bug is a tracker bug for it.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> There are some properties that have been implemented but not yet animatable.
> http://searchfox.org/mozilla-central/search?q=FIXME%3A.
> *animatable&case=false&regexp=true&path=*.rs

I just fixed one of them in bug 1353921, the fix in bug 1353921 would be a reference for fixing rest of them by newbie for stylo.

Comment 2

[Boris Chiou [:boris]]

These properties also need to be animatable:
1. -moz-box-flex
2. -moz-image-region
3. -moz-outline-radius-{corner}
4. column-fill
5. clip-path
6. fill-opacity
7. flood-color
8. flood-opacity
9. lighting-color
10. stop-color
11. stop-opacity
12. stroke-{*}
13. -moz-tab-size

... Haven't checked all yet.

Many properties above are SVG properties, so we need to check them again after SMIL works?

Comment 3

[Brian Birtles (:birtles)]

You can animate SVG properties from CSS, so you don't need to wait for SMIL.

Comment 4

[Brian Birtles (:birtles)]

(Oh, sorry, I guess you mean we need to double-check the behavior is correct after implementing SMIL since we probably don't have good tests of them from CSS.)

Comment 5

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #4)
> (Oh, sorry, I guess you mean we need to double-check the behavior is correct
> after implementing SMIL since we probably don't have good tests of them from
> CSS.)

Yes. But as your suggestion, we can make them animatable first and test them from CSS locally.

Comment 6

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #2)
> These properties also need to be animatable:
> 1. -moz-box-flex
> 2. -moz-image-region
> 3. -moz-outline-radius-{corner}
> 4. column-fill
> 5. clip-path
> 6. fill-opacity
> 7. flood-color
> 8. flood-opacity
> 9. lighting-color
> 10. stop-color
> 11. stop-opacity
> 12. stroke-{*}
> 13. -moz-tab-size
> 
> ... Haven't checked all yet.
> 
> Many properties above are SVG properties, so we need to check them again
> after SMIL works?

14. filter (maybe together with flood-color, flood-opacity, and lighting-color.)
[1] https://drafts.fxtf.org/filters/#FilterProperty
[2] http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/servo/components/style/properties/longhand/effects.mako.rs#89-91

Comment 7

[Brian Birtles (:birtles)]

Just looking at the added w-p-t test here:

> +      'rect',
> +      { type: 'discrete', options: [ [ 'rect(10px, 10px, 10px, 10px)',
> +                                       'auto' ],
> +                                     [ 'rect(10px, 10px, 10px, 10px)',
> +                                       'rect(10px, 10px, 10px, auto)'] ] }

This is testing that we use discrete animation between 'rect(10px, 10px, 10px, 10px)' and 'rect(10px, 10px, 10px, auto)' but that's not what Gecko does. Gecko does linear interpolation of each length and discrete interpolation of the '10px' -> 'auto' pair only. By using the same '10px' values elsewhere though that's masked.

So it seems the Gecko and Servo interpolation of clip differs but these tests don't expose that. We should fix that.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #7)
> Just looking at the added w-p-t test here:
> 
> > +      'rect',
> > +      { type: 'discrete', options: [ [ 'rect(10px, 10px, 10px, 10px)',
> > +                                       'auto' ],
> > +                                     [ 'rect(10px, 10px, 10px, 10px)',
> > +                                       'rect(10px, 10px, 10px, auto)'] ] }
> 
> This is testing that we use discrete animation between 'rect(10px, 10px,
> 10px, 10px)' and 'rect(10px, 10px, 10px, auto)' but that's not what Gecko
> does. Gecko does linear interpolation of each length and discrete
> interpolation of the '10px' -> 'auto' pair only. By using the same '10px'
> values elsewhere though that's masked.
> 
> So it seems the Gecko and Servo interpolation of clip differs but these
> tests don't expose that. We should fix that.

Oh, right. I missed that.  We should write this kind of partial discrete in testInterpolation respectively.

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Brian Birtles (:birtles) from comment #7)
> Just looking at the added w-p-t test here:
> 
> > +      'rect',
> > +      { type: 'discrete', options: [ [ 'rect(10px, 10px, 10px, 10px)',
> > +                                       'auto' ],
> > +                                     [ 'rect(10px, 10px, 10px, 10px)',
> > +                                       'rect(10px, 10px, 10px, auto)'] ] }
> 
> This is testing that we use discrete animation between 'rect(10px, 10px,
> 10px, 10px)' and 'rect(10px, 10px, 10px, auto)' but that's not what Gecko
> does. Gecko does linear interpolation of each length and discrete
> interpolation of the '10px' -> 'auto' pair only. By using the same '10px'
> values elsewhere though that's masked.

The gecko's behavior is little strange. If we animate clip ['rect(0px, 0px, 0px, 0px)', 'rect(100px, auto, 100px, 100px)'], the gecko will discrete the whole rect(i.e. 49% -> rect(0px, 0px, 0px, 0px) / 50% -> rect(100px, auto, 100px, 100px)).
Because we check the each offset's unit in addWeighted[1].

[1] https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/layout/style/StyleAnimationValue.cpp#3090-3096

> 
> So it seems the Gecko and Servo interpolation of clip differs but these
> tests don't expose that. We should fix that.

If we animate clip ['rect(0px, auto, 0px, 0px)', 'rect(100px, auto, 100px, 100px)'], the gecko does linear interpolation each offset value as length except '0px' -> 'auto' pair since each offset's unit is same.

Comment 10

[Brian Birtles (:birtles)]

As far as I am aware and from having discussed with Daisuke, font-variation-settings was the last property we needed to make animatable so this is now done.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

\o/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Hiro, should we remove the check at https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/dom/smil/nsSMILCSSProperty.cpp#144?

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Yeah, I did confirm that all the properties that are considered as animatable in nsSMILCSSProperty::IsPropertyAnimatable() have been already animatable in servo side.