1353921 - Make letter-spacing animatable

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

To make a property animatable we need to do:

1. set animatable variable to 'True'
2. implement clone_PROPERTY_NAME() method
3. implement Interpolate trait

This bug is a reference what we need to do specifically.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1353921 - Make letter-spacing animatable.

From the spec: 'normal' value computes to zero.

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

Comment 2

[Robert Longson [:longsonr]]

Comment on attachment 8855088 [details]
Bug 1353921 - Make letter-spacing animatable.

https://reviewboard.mozilla.org/r/126986/#review129680

::: servo/components/style/properties/longhand/inherited_text.mako.rs:408
(Diff revision 1)
>          }
>      % endif
>  </%helpers:longhand>
>  
>  // FIXME: This prop should be animatable.
> -<%helpers:longhand name="letter-spacing" boxed="True" animatable="False"
> +<%helpers:longhand name="letter-spacing" boxed="True" animatable="True"

presumably you can remove the FIXME now.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Robert Longson from comment #2)
> Comment on attachment 8855088 [details]
> Bug 1353921 - Make letter-spacing animatable.
> 
> https://reviewboard.mozilla.org/r/126986/#review129680
> 
> ::: servo/components/style/properties/longhand/inherited_text.mako.rs:408
> (Diff revision 1)
> >          }
> >      % endif
> >  </%helpers:longhand>
> >  
> >  // FIXME: This prop should be animatable.
> > -<%helpers:longhand name="letter-spacing" boxed="True" animatable="False"
> > +<%helpers:longhand name="letter-spacing" boxed="True" animatable="True"
> 
> presumably you can remove the FIXME now.

Oh yes. Thanks!

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8855088 [details]
Bug 1353921 - Make letter-spacing animatable.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I did confirm letter-spacing animatable now with testing/web-platform/tests/web-animations/animation-model/animation-types/interpolation-per-property.html .

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8855088 [details]
Bug 1353921 - Make letter-spacing animatable.

https://reviewboard.mozilla.org/r/126986/#review129702

Thanks for fixing this!

::: servo/components/style/properties/gecko.mako.rs:2890
(Diff revision 2)
> +    pub fn clone_letter_spacing(&self) -> longhands::letter_spacing::computed_value::T {
> +        use properties::longhands::letter_spacing::computed_value::T;
> +        match self.gecko.mLetterSpacing.as_value() {
> +            CoordDataValue::Normal => T(None),
> +            CoordDataValue::Coord(coord) => T(Some(Au(coord))),
> +            _ => unreachable!(),

Let's add a message here, like:

`unreachable!("Unexpected computed value for letter-spacing")`

or that sort of stuff.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Thank you Emilio for the quick review!

https://github.com/servo/servo/pull/16278

Comment 8

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/autoland/rev/ab65cc6fb5de