stylo: make all animatable properties *animatable*

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P5
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: hiro, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Depends on: 1353921
(Reporter)

Comment 1

a year ago
(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.
(Reporter)

Updated

a year ago
Depends on: 1353966
(Reporter)

Updated

a year ago
Depends on: 1354053
(Reporter)

Updated

a year ago
Depends on: 1354437
Depends on: 1355344
Depends on: 1355732
Depends on: 1356071
No longer depends on: 1356071
Depends on: 1356071
Depends on: 1356162
Depends on: 1357663
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?
You can animate SVG properties from CSS, so you don't need to wait for SMIL.
(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.)
(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.
(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
No longer depends on: 1360133, 1360144

Updated

a year ago
Depends on: 1360144, 1360133
(Reporter)

Updated

a year ago
Depends on: 1362897
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.
(Reporter)

Comment 8

a year ago
(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.
(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.
Depends on: 1363639
Depends on: 1369277
Depends on: 1369624
Depends on: 1369625

Updated

a year ago
Blocks: 1292283

Updated

a year ago
Depends on: 1370803

Updated

a year ago
Depends on: 1370808

Updated

a year ago
No longer blocks: 1292283

Updated

a year ago
Depends on: 1370845

Updated

a year ago
Depends on: 1370846
Depends on: 1376495
Depends on: 1390702
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.
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
(Reporter)

Comment 11

10 months ago
\o/
(Reporter)

Comment 13

2 months ago
Yeah, I did confirm that all the properties that are considered as animatable in nsSMILCSSProperty::IsPropertyAnimatable() have been already animatable in servo side.
Flags: needinfo?(hikezoe)
You need to log in before you can comment on or make changes to this bug.