Closed Bug 1360144 Opened 6 years ago Closed 6 years ago

stylo: Make stroke-{*} animatable

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: chenpighead, Assigned: chenpighead)

References

(
URL
)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1360144 - enable WPT interpolation tests for some SVG stroke related properties.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
      No description provided.
You are working on this, so mark P1.
Priority: -- → P1
No longer blocks: 1353918
Blocks: 1353918
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
(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]>...
(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. :-)
Attachment #8863663 - Attachment is obsolete: true
Attachment #8863664 - Attachment is obsolete: true
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
Attachment #8863662 - Flags: review?(hikezoe)
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.
Attachment #8863665 - Flags: review?(boris.chiou) → review+
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 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 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 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 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.
Attachment #8863662 - Flags: review?(hikezoe) → review+
(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?
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 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.
(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. :-)
Attachment #8863665 - Attachment is obsolete: true
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c8c40eddb5e
enable WPT interpolation tests for some SVG stroke related properties. r=hiro
https://hg.mozilla.org/mozilla-central/rev/0c8c40eddb5e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1364797
You need to log in before you can comment on or make changes to this bug.