Make letter-spacing animatable

RESOLVED FIXED

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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 hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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.
(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!
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
I did confirm letter-spacing animatable now with testing/web-platform/tests/web-animations/animation-model/animation-types/interpolation-per-property.html .

Comment 6

a year ago
mozreview-review
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.
Attachment #8855088 - Flags: review?(emilio+bugs) → review+
Thank you Emilio for the quick review!

https://github.com/servo/servo/pull/16278
https://hg.mozilla.org/integration/autoland/rev/ab65cc6fb5de
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.