1360144 - stylo: Make stroke-{*} animatable

Status: RESOLVED FIXED

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

Comment 1

[Boris Chiou [:boris]]

You are working on this, so mark P1.

Comment 2

[Boris Chiou [:boris]]

Please make sure these properties [1] are animatable: (reference test: [2])
1. stroke-dasharray
2. stroke-linecap
3. stroke-linejoin
4. stroke-miterlimit
5. stroke-opacity

[1] http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/servo/components/style/properties/longhand/inherited_svg.mako.rs#76-102
[2] http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/layout/style/test/test_transitions_per_property.html#237-253

Comment 3

[Jeremy Chen ([e-mail])]

Okay, here's the plan:

1. Enable wpt tests for stroke-{*} properties.
Note that tests for stroke-linecap and stroke-linejoin have been enabled already.
1.1 reuse existing positiveNumber for stroke-miterlimit
1.2 create a new type, say zeroToOneNumberType, for stroke-opacity
1.3 create a new type, say dasharrayType, for stroke-dasharray

2. Make stroke-{*} animatable for stylo.
2.1 For properties that already implemented Interpolate trait and clone() for glue code, we can just make them animatable by replacing the animation_value_type with proper type name. 
2.1.1 set animation_value_type to 'discrete' for stroke-linecap and stroke-linejoin
2.1.2 set animation_value_type to 'ComputedValue' for stroke-miterlimit and stroke-opacity
2.2 Implement Interpolate trait and glue codes for stroke-dasharray

Comment 4

[Jeremy Chen ([e-mail])]

Attachment: Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

In this patch, we added zeroToOneNumberType to enabled stroke-opacity's
animation tests. With the newly added zeroToOneNumberType, we should be
able to test other opacity like properties as well.

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

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: Bug 1360144 - enable WPT interpolation tests for stroke-miterlimit.

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

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1360144 - enable WPT interpolation tests for stroke-dasharray.

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

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: Bug 1360144 - make stroke-{*} animatable for stylo.

This part includes making stroke-linecap, stroke-linejoin, stroke-miterlimit,
stroke-opacity, and stroke-dasharray animatable.

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

Comment 8

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #3)
> Okay, here's the plan:
> 
> 1. Enable wpt tests for stroke-{*} properties.
> Note that tests for stroke-linecap and stroke-linejoin have been enabled
> already.
> 1.1 reuse existing positiveNumber for stroke-miterlimit
> 1.2 create a new type, say zeroToOneNumberType, for stroke-opacity
> 1.3 create a new type, say dasharrayType, for stroke-dasharray
> 
> 2. Make stroke-{*} animatable for stylo.
> 2.1 For properties that already implemented Interpolate trait and clone()
> for glue code, we can just make them animatable by replacing the
> animation_value_type with proper type name. 
> 2.1.1 set animation_value_type to 'discrete' for stroke-linecap and
> stroke-linejoin
> 2.1.2 set animation_value_type to 'ComputedValue' for stroke-miterlimit and
> stroke-opacity
> 2.2 Implement Interpolate trait and glue codes for stroke-dasharray

I've finished above all except for item 2.2. I guess I might need to implement interpolate for smallvec::SmallVec<[values::Either<f32, values::computed::length::LengthOrPercentage>; 1]>...

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #3)
> 2.1.1 set animation_value_type to 'discrete' for stroke-linecap and
> stroke-linejoin

Actually we planed to implement all of discrete type animation in bug 1336668 altogether, but it's OK if you've already implemented them. :-)

Comment 10

[Jeremy Chen ([e-mail])]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135464/diff/1-2/

Comment 11

[Jeremy Chen ([e-mail])]

Comment on attachment 8863665 [details]
Bug 1360144 - make stroke-{*} animatable for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135470/diff/1-2/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

https://reviewboard.mozilla.org/r/135464/#review138730

Thanks for fixing this!

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:1266
(Diff revision 2)
> -    types: [
> +    types: [ 'positiveNumber' ]
> -    ]
>    },
>    'stroke-opacity': {
>      // https://svgwg.org/svg2-draft/painting.html#StrokeOpacityProperty
> -    types: [
> +    types: [ 'zeroToOneNumber' ]

I think we can call this just 'opacity'.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1291
(Diff revision 2)
> +  testAddition: function(property, setup) {
> +    lengthType.testAddition(property, setup);
> +    percentageType.testAddition(property, setup);
> +    positiveNumberType.testAddition(property, setup);
> +
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      target.style[idlName] = '6, 30%, 2px';
> +      var animation = target.animate({ [idlName]:
> +                                         ['6, 30%, 2px',
> +                                          '2, 50%, 6px'] },
> +                                     { duration: 1000, fill: 'add' });
> +      testAnimationSamples(
> +          animation, idlName,
> +          [{ time: 0, expected: '12, 60%, 4px' }]);
> +    }, property + ': dasharray');

As per spec [1], stroke-dasharray is non-additive property.  So we have to do some tweaks that no 'testAddition' is expected for non-additive properties in the test [2] in addition-per-property.html.  Or, we should write an additive test case that animating value replaces underlying values. E.g;

 target.style[idlName] = '6, 30%, 2px';
 var animation = target.animate({ [idlName]: [ '1, 2, 3, 4, 5', ...
 
 ..., expected: '1, 2, 3, 4, 5' }]);
 
and mark this test FAIL.

[1] https://svgwg.org/svg2-draft/painting.html#StrokeDasharrayProperty

[2] https://hg.mozilla.org/mozilla-central/file/a748acbebbde/testing/web-platform/tests/web-animations/animation-model/animation-types/addition-per-property.html#l42

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8863665 [details]
Bug 1360144 - make stroke-{*} animatable for stylo.

https://reviewboard.mozilla.org/r/135470/#review138754

r=me with the following comments addressed. Besides, do you also verify this with their shorthand (i.e. stroke)?

::: servo/components/style/properties/gecko.mako.rs:3789
(Diff revision 2)
> +                    vec.push(Either::Second(LengthOrPercentage::Length(Au(coord)))),
> +                CoordDataValue::Percent(p) =>
> +                    vec.push(Either::Second(LengthOrPercentage::Percentage(p))),
> +                CoordDataValue::Calc(calc) =>
> +                    vec.push(Either::Second(LengthOrPercentage::Calc(calc.into()))),
> +                _ => debug_assert!(false),

nit:
You can also use ```unreachable!()```

::: servo/components/style/properties/helpers/animated_properties.mako.rs:634
(Diff revision 2)
> +impl Interpolate for SmallVec<[Either<f32, LengthOrPercentage>; 1]> {
> +    fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
> +        use num_integer::lcm;
> +        let len = lcm(self.len(), other.len());
> +        self.iter().cycle().zip(other.iter().cycle()).take(len).map(|(me, you)| {
> +            me.interpolate(you, progress)
> +        }).collect()
> +    }
> +}

We already have implemented Interpolate for ```SmallVec<[T; 1]>```, so I think you can just implement ```RepeatableListInterpolate``` for ```Either<T, U>```.

Note: we have implemented Interpolate for Either, so I guess it is very simple.

Comment 14

[Jeremy Chen ([e-mail])]

Comment on attachment 8863665 [details]
Bug 1360144 - make stroke-{*} animatable for stylo.

https://reviewboard.mozilla.org/r/135470/#review138754

I only checked each longhand, I'll double checked the rendering results before landing. Thanks.

> nit:
> You can also use ```unreachable!()```

Will do.

> We already have implemented Interpolate for ```SmallVec<[T; 1]>```, so I think you can just implement ```RepeatableListInterpolate``` for ```Either<T, U>```.
> 
> Note: we have implemented Interpolate for Either, so I guess it is very simple.

Oh, I see. Thak you for the review.

Comment 15

[Jeremy Chen ([e-mail])]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

https://reviewboard.mozilla.org/r/135464/#review138730

> I think we can call this just 'opacity'.

Okay.

> As per spec [1], stroke-dasharray is non-additive property.  So we have to do some tweaks that no 'testAddition' is expected for non-additive properties in the test [2] in addition-per-property.html.  Or, we should write an additive test case that animating value replaces underlying values. E.g;
> 
>  target.style[idlName] = '6, 30%, 2px';
>  var animation = target.animate({ [idlName]: [ '1, 2, 3, 4, 5', ...
>  
>  ..., expected: '1, 2, 3, 4, 5' }]);
>  
> and mark this test FAIL.
> 
> [1] https://svgwg.org/svg2-draft/painting.html#StrokeDasharrayProperty
> 
> [2] https://hg.mozilla.org/mozilla-central/file/a748acbebbde/testing/web-platform/tests/web-animations/animation-model/animation-types/addition-per-property.html#l42

Ok, I'll go with the second option. Add an additive test and makr it test FAIL.

Comment 16

[Jeremy Chen ([e-mail])]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135464/diff/2-3/

Comment 17

[Jeremy Chen ([e-mail])]

Comment on attachment 8863665 [details]
Bug 1360144 - make stroke-{*} animatable for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135470/diff/2-3/

Comment 18

[Jeremy Chen ([e-mail])]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

https://reviewboard.mozilla.org/r/135464/#review138808

As for the length interpolation tests for stroke-dasharray, the result of interpolation would be unitless length in Gecko, whereas the result of interpolation would be unit length in Servo/Stylo. So, I think it would be better to add length tests later, until we can support unitless length in Servo/Stylo.

Comment 19

[Jeremy Chen ([e-mail])]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

https://reviewboard.mozilla.org/r/135464/#review138808

I also wonder why stroke-dasharray should accept unitless length? I can't find it in the https://quirks.spec.whatwg.org/#the-unitless-length-quirk...

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

https://reviewboard.mozilla.org/r/135464/#review138810

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1280
(Diff revisions 2 - 3)
> +  // additive test case that animating value replaces underlying values, and mark
> +  // this test FAIL. See https://www.w3.org/TR/SVG2/painting.html#StrokeDashing.

Please drop ', and mark this test FAIL'.  Comments in web platform tests should not mention about behaviors on a speficic browsers.

::: testing/web-platform/meta/web-animations/animation-model/animation-types/addition-per-property.html.ini:10
(Diff revision 3)
>      expected: FAIL
>      bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1356241
> +
> +  [stroke-dasharray: dasharray]
> +    expected: FAIL
> +    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1360144

This link is not correct, please open a new bug for it.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:408
(Diff revision 3)
>      }, property + ': positive number');
>    },
>  
>  };
>  
> +// Test using float values in the range [0, 1] (e.g. opacity)

'(e.g. opacity)' is not needed any more?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:426
(Diff revision 3)
> +  testAddition: function(property, setup) {
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      target.style[idlName] = 0.3;
> +      var animation = target.animate({ [idlName]: [0.3, 0.8] },

Oops. Sorry, I forgot to mention this test values. We should test values that produce > 1 by adding and check the result is clamped. E.g. 0.3 + 0.8.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #19)
> Comment on attachment 8863662 [details]
> Bug 1360144 - enable WPT interpolation tests for some SVG stroke related
> properties.
> 
> https://reviewboard.mozilla.org/r/135464/#review138808
> 
> I also wonder why stroke-dasharray should accept unitless length? I can't
> find it in the https://quirks.spec.whatwg.org/#the-unitless-length-quirk...

https://www.w3.org/TR/SVG/types.html#DataTypeLength
SVG length was different from CSS one?

Comment 22

[Cameron McCormack (:heycam)]

Yes.  Or with a more up to date definition, it explicitly allows <number> or the standard CSS <length> and <percentage>:

https://svgwg.org/svg2-draft/painting.html#StrokeDasharrayProperty

Comment 23

[Jeremy Chen ([e-mail])]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

https://reviewboard.mozilla.org/r/135464/#review138810

> This link is not correct, please open a new bug for it.

Filed Bug 1361738 and updated the link.

> Oops. Sorry, I forgot to mention this test values. We should test values that produce > 1 by adding and check the result is clamped. E.g. 0.3 + 0.8.

Okay, I'll add an extra test for the clamping check.

Comment 24

[Jeremy Chen ([e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> https://www.w3.org/TR/SVG/types.html#DataTypeLength
> SVG length was different from CSS one?

(In reply to Cameron McCormack (:heycam) from comment #22)
> Yes.  Or with a more up to date definition, it explicitly allows <number> or
> the standard CSS <length> and <percentage>:
> 
> https://svgwg.org/svg2-draft/painting.html#StrokeDasharrayProperty

I see. Thank you for the explanation. :-)

Comment 25

[Jeremy Chen ([e-mail])]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135464/diff/3-4/

Comment 26

[Jeremy Chen ([e-mail])]

Comment on attachment 8863665 [details]
Bug 1360144 - make stroke-{*} animatable for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135470/diff/3-4/

Comment 27

[Jeremy Chen ([e-mail])]

Another round of try for the additional test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b548095f1fa

Comment 28

[Jeremy Chen ([e-mail])]

Servo PR: https://github.com/servo/servo/pull/16711

Comment 29

[Jeremy Chen ([e-mail])]

Comment on attachment 8863662 [details]
Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135464/diff/4-5/

Comment 30

[Jeremy Chen ([e-mail])]

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

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c8c40eddb5e
enable WPT interpolation tests for some SVG stroke related properties. r=hiro

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c8c40eddb5e