Closed Bug 1357663 Opened 7 years ago Closed 7 years ago

stylo: Make font-stretch 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: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1357663 - Enable web platform test of font-stretch property animation.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
Let's make font-stretch animatable.

This parameter's interpolate spec is as follow:

'The interpolation happens as though the ordered values are equally spaced real numbers. The interpolation result is rounded to the nearest value, with values exactly halfway between two values rounded towards the later value in the list above.'
(https://drafts.csswg.org/css-fonts-3/#font-stretch-animation)
For now, each UA's behavior is different.

Test page: http://output.jsbin.com/rarexix/69/

Firefox 57 / Safari Technology Preview 27 : animate based on specification.
Chrome 57 : animate as two value discrete(i.e. if we specified 'condensed' to 'expanded', animate as this two value discrete, not animate 'normal')
Edge / IE : Not animate.
Failed to build the stylo when I changed an animation type of font-strech to 'normal'：

---------------------------------------------------
 0:49.97 error: no method named `interpolate` found for type `&properties::longhands::font_stretch::computed_value::T` in the current scope
 0:49.97      --> /home/mantaroh/mozilla-central/obj-stylo/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-24d92e186fcca940/out/properties.rs:67769:52
 0:49.97       |
 0:49.97 67769 |                             let value = match from.interpolate(to, progress) {
 0:49.97       |                                                    ^^^^^^^^^^^
 0:49.97       |
 0:49.97       = help: items from traits can only be used if the trait is implemented and in scope; the following trait defines an item `interpolate`, perhaps you need to implement it:
 0:49.97       = help: candidate #1: `properties::animated_properties::Interpolate`
 0:49.97
 0:50.64 error: no method named `interpolate` found for type `&properties::longhands::font_stretch::computed_value::T` in the current scope
 0:50.64      --> /home/mantaroh/mozilla-central/obj-stylo/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-24d92e186fcca940/out/properties.rs:73310:34
 0:50.64       |
 0:50.64 73310 |                             from.interpolate(to, progress).map(AnimationValue::FontStretch)
 0:50.64       |                                  ^^^^^^^^^^^
 0:50.64       |
 0:50.64       = help: items from traits can only be used if the trait is implemented and in scope; the following trait defines an item `interpolate`, perhaps you need to implement it:
 0:50.64       = help: candidate #1: `properties::animated_properties::Interpolate`
 0:50.64
 0:54.84 error[E0308]: mismatched types
 0:54.84     --> /home/mantaroh/mozilla-central/obj-stylo/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-24d92e186fcca940/out/gecko_properties.rs:5893:13
 0:54.84      |
 0:54.84 5893 |             structs::NS_FONT_STRETCH_ULTRA_CONDENSED => Keyword::ultra_condensed,
 0:54.85      |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u32, found i32
 0:54.85
 0:54.85 error[E0308]: mismatched types
 0:54.85     --> /home/mantaroh/mozilla-central/obj-stylo/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-24d92e186fcca940/out/gecko_properties.rs:5894:13
 0:54.85      |
 0:54.85 5894 |             structs::NS_FONT_STRETCH_EXTRA_CONDENSED => Keyword::extra_condensed,
 0:54.85      |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u32, found i32
 0:54.85
 0:54.85 error[E0308]: mismatched types
 0:54.85     --> /home/mantaroh/mozilla-central/obj-stylo/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-24d92e186fcca940/out/gecko_properties.rs:5895:13
 0:54.85      |
 0:54.85 5895 |             structs::NS_FONT_STRETCH_CONDENSED => Keyword::condensed,
 0:54.85      |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u32, found i32
 0:54.85
 0:54.85 error[E0308]: mismatched types
 0:54.85     --> /home/mantaroh/mozilla-central/obj-stylo/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-24d92e186fcca940/out/gecko_properties.rs:5896:13
 0:54.85      |
 0:54.85 5896 |             structs::NS_FONT_STRETCH_SEMI_CONDENSED => Keyword::semi_condensed,
 0:54.85      |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u32, found i32
 0:54.85
 0:55.37 error: aborting due to 6 previous e
---------------------------------------------------

As far as I searched, NS_FONT_STRECH_** value is different from positive value and negative value[1]. So we can't cast to u32, we will need to this casting template code[2] or implement this clone function.
And we will need to implement the interpolate function.

[1] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/servo/components/style/gecko_bindings/structs_debug.rs#600-613
[2] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/servo/components/style/properties/gecko.mako.rs#281-295
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> For now, each UA's behavior is different.
> 
> Test page: http://output.jsbin.com/rarexix/69/
> 
> Firefox 57 / Safari Technology Preview 27 : animate based on specification.
> Chrome 57 : animate as two value discrete(i.e. if we specified 'condensed'
> to 'expanded', animate as this two value discrete, not animate 'normal')
> Edge / IE : Not animate.

Sorry, I think this comment 1 is wrong.

I tried example code: https://people-mozilla.org/~myoshinaga/font-stretch/

Firefox is not based on specification. If we run following code, Firefox animate like step-end function(i.e. the animation stays in its initial state until the end, where it directly jumps to its final position).

--------------------------
@keyframes anim {
  from { font-stretch: normal; }
  to   { font-stretch: semi-expanded; }
}
div {
  animation: anim 1s both linear;
}
--------------------------

The specification explain that 'The interpolation result is rounded to the nearest value'. [1]
[1] https://drafts.csswg.org/css-fonts-3/#font-stretch-animation

However gecko's implementation is using floor()[2], so animation's behave like step-end function. I think that We will need to use round().

[2] https://dxr.mozilla.org/mozilla-central/rev/c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e/layout/style/StyleAnimationValue.cpp#2887

And I think that we will need to implement interpolate function like this on stylo.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> > For now, each UA's behavior is different.
> > 
> > Test page: http://output.jsbin.com/rarexix/69/
> > 
> > Firefox 57 / Safari Technology Preview 27 : animate based on specification.
> > Chrome 57 : animate as two value discrete(i.e. if we specified 'condensed'
> > to 'expanded', animate as this two value discrete, not animate 'normal')
> > Edge / IE : Not animate.
> 
> Sorry, I think this comment 1 is wrong.
> 
> I tried example code: https://people-mozilla.org/~myoshinaga/font-stretch/
> 
> Firefox is not based on specification. 
>

This gecko part fixed at bug 1359281.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> As far as I searched, NS_FONT_STRECH_** value is different from positive
> value and negative value[1]. So we can't cast to u32, we will need to this
> casting template code[2] or implement this clone function.
> And we will need to implement the interpolate function.
>

The impl_keyword_clone generated following code, if we make font-stretch animatable:
---------------------------------------------------------------------------------------------------
    pub fn clone_font_stretch(&self) -> longhands::font_stretch::computed_value::T {
        use properties::longhands::font_stretch::computed_value::T as Keyword;
        // FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts
        match self.gecko.mFont.stretch as u32 {
            structs::NS_FONT_STRETCH_NORMAL => Keyword::normal,
            structs::NS_FONT_STRETCH_ULTRA_CONDENSED => Keyword::ultra_condensed,
            structs::NS_FONT_STRETCH_EXTRA_CONDENSED => Keyword::extra_condensed,
            structs::NS_FONT_STRETCH_CONDENSED => Keyword::condensed,
            structs::NS_FONT_STRETCH_SEMI_CONDENSED => Keyword::semi_condensed,
            structs::NS_FONT_STRETCH_SEMI_EXPANDED => Keyword::semi_expanded,
            structs::NS_FONT_STRETCH_EXPANDED => Keyword::expanded,
            structs::NS_FONT_STRETCH_EXTRA_EXPANDED => Keyword::extra_expanded,
            structs::NS_FONT_STRETCH_ULTRA_EXPANDED => Keyword::ultra_expanded,
            x => panic!("Found unexpected value in style struct for font_stretch property: {:?}", x),
        }
    }
---------------------------------------------------------------------------------------------------

This structs::NS_FONT_STRECTH_* is t_int16 if value is negative, otherwise positive value is t_uint16.
And servo's longhands::font_stretch::computed_value::T is enum(value is 0 to 8).

So I think that we will need to exclude font-stretch longhand generator like above code, or generate two the match expression based on value when generating above code(i.e. matching casted gecko's value if value is negative, otherwise matching uncasted value).

I tried two implementation, and I think that we had better to use longhand generator for consistency.
Comment on attachment 8863595 [details]
Bug 1357663 - Enable web platform test of font-stretch property animation.

https://reviewboard.mozilla.org/r/135122/#review138276

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1203
(Diff revision 1)
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      var animation = target.animate({ [idlName]:
> +                                         ['ultra-condensed',
> +					  'ultra-expanded'] },
> +                                     { duration: 800, fill: 'both' });

I am curious why you chose a different duration.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1206
(Diff revision 1)
> +                                         ['ultra-condensed',
> +					  'ultra-expanded'] },
> +                                     { duration: 800, fill: 'both' });
> +	testAnimationSamples(
> +          animation, idlName,
> +          [{ time: 400,  expected: 'normal' }]);

I don't think this test case is a good case becasue 'normal' is the initial value of font-stretch property. We should use other from and to values.
Also I think we should add more test cases here.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1221
(Diff revision 1)
> +                                         ['ultra-condensed',
> +					  'ultra-expanded'] },
> +                                     { duration: 800, composite: 'add' });
> +      testAnimationSamples(
> +        animation, idlName,
> +        [{ time: 0, expected: 'ultra-condensed' }]);

I don't think this expected value is correct.
I guess it should be 'semi-expanded'. I think we don't implement additive for font-stretch yet.
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review138280

::: servo/components/style/properties/gecko.mako.rs:259
(Diff revision 1)
> +    <%
> +        if cast_type is None:
> +           cast_type = 'u8'
> +    %>

I don't quite understand why we need this change.
Could you explain the reason?

::: servo/components/style/properties/gecko.mako.rs:295
(Diff revision 1)
> +            % for value in keyword.values_for('gecko'):
> +            const ${keyword.gecko_constant(value)} : ${cast_type} = structs::${keyword.gecko_constant(value)} ${keyword.maybe_cast(cast_type)};
> +            % endfor

If we need this variables for sigined/unsigned mixed NS_FONT_STRECH_XX , I guess, we should check the cast_type is singed or not instead of adding 'cast_type is None' branch.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2131
(Diff revision 1)
>              Ok(IntermediateRGBA::new(red, green, blue, alpha))
>          }
>      }
>  }
>  
> +impl Interpolate for FontStretch {

This should be right after FontWeight.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2134
(Diff revision 1)
> +        let a = *self as i16 as f64;
> +	let b = *other as i16 as f64;
> +	let mut interpolated_value = ((a + (b - a) * progress) + 0.5).floor() as i16;

I don't think casting i16 is right thing to do. I am really wondering who ensures the order of FontStretch enum?  It seems to me that 'normal' is the first one.

::: servo/components/style/properties/longhand/font.mako.rs:1153
(Diff revision 1)
> -                                "normal ultra-condensed extra-condensed condensed \
> -                                 semi-condensed semi-expanded expanded extra-expanded \
> +                                "ultra-condensed extra-condensed condensed semi-condensed\
> +                                 normal semi-expanded expanded extra-expanded \
>                                   ultra-expanded",

Oh, reordered...  IIUC, the first value is for initial value. You should not change it.
Just FYI, we should define a static variable to map integers to font stretch enums just like font-size does [1].  Also, I guess the first index (for ultra-condensed) should be 1 if we want to get extra-condensed = ultra-condensed + ultra-condensed for additive calculation.

[1] https://hg.mozilla.org/mozilla-central/file/17d8a1e278a9/servo/components/style/properties/longhand/font.mako.rs#l719
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Just FYI, we should define a static variable to map integers to font stretch
> enums just like font-size does [1].  Also, I guess the first index (for
> ultra-condensed) should be 1 if we want to get extra-condensed =
> ultra-condensed + ultra-condensed for additive calculation.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/17d8a1e278a9/servo/components/
> style/properties/longhand/font.mako.rs#l719

Thanks hiro,

I'm writing the additive testcase. However I think that gecko's additive of font-stretch is not support.
For example, we'll expect that computed value is 'condensed' when example code is as follow:
----------------------
var p = document.getElementById("target"); // <p id="target">
p.style.fontStretch = 'normal';
var anim = p.animate({fontStretch: ['ultra-condensed', 'ultra-expanded']}, {duration: 1000, composite: 'add'});
anim.pause();
anim.currentTime;
console.log(getComputedStyle(p).fontStretch);  // displayed 'ultra-condensed', not 'condensed'
----------------------

So I'll submit this problem another bug, and I'll skip this font-stretch test until fix it.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #11)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> > Just FYI, we should define a static variable to map integers to font stretch
> > enums just like font-size does [1].  Also, I guess the first index (for
> > ultra-condensed) should be 1 if we want to get extra-condensed =
> > ultra-condensed + ultra-condensed for additive calculation.
> > 
> > [1]
> > https://hg.mozilla.org/mozilla-central/file/17d8a1e278a9/servo/components/
> > style/properties/longhand/font.mako.rs#l719
> 
> Thanks hiro,
> 
> I'm writing the additive testcase. However I think that gecko's additive of
> font-stretch is not support.
> For example, we'll expect that computed value is 'condensed' when example
> code is as follow:
> ----------------------
> var p = document.getElementById("target"); // <p id="target">
> p.style.fontStretch = 'normal';
> var anim = p.animate({fontStretch: ['ultra-condensed', 'ultra-expanded']},
> {duration: 1000, composite: 'add'});
> anim.pause();
> anim.currentTime;
> console.log(getComputedStyle(p).fontStretch);  // displayed
> 'ultra-condensed', not 'condensed'
> ----------------------

I think it should be 'semi-expanded'.
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review138280

Thank you for review.

> I don't quite understand why we need this change.
> Could you explain the reason?

I though that we can't specify the default value, but we can specify it.
I fixed it.

> If we need this variables for sigined/unsigned mixed NS_FONT_STRECH_XX , I guess, we should check the cast_type is singed or not instead of adding 'cast_type is None' branch.

I address it using by string compare. If first character of cast_type is 'u', we will assume that this type is unsigned.
Comment on attachment 8863595 [details]
Bug 1357663 - Enable web platform test of font-stretch property animation.

https://reviewboard.mozilla.org/r/135122/#review138276

> I am curious why you chose a different duration.

I choose this duration in order to make the numbers balance with font-stretch's keyword.
A start time is zero and end time is 800ms, So each step is 100ms or 50ms. (i.e. 0ms-49ms / 50ms-149ms / 150ms-249ms / ... / 750ms-800ms)

> I don't think this expected value is correct.
> I guess it should be 'semi-expanded'. I think we don't implement additive for font-stretch yet.

I misunderstand additive animation value, I think that this value should be initial value when current time is zero.
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review140408

::: servo/components/style/properties/gecko.mako.rs:259
(Diff revision 2)
>                                      "*self.gecko.__bindgen_anon_1.mBorderColor.as_mut()")
>          return "unsafe { %s = %s };" % (ffi_name, expr)
>      return "self.gecko.%s = %s;" % (ffi_name, expr)
>  %>
>  
> -<%def name="impl_keyword_setter(ident, gecko_ffi_name, keyword, cast_type='u8', on_set=None)">
> +<%def name="impl_keyword_setter(ident, gecko_ffi_name, keyword, cast_type, on_set=None)">

I wonder why the default value 'u8' was dropped?

::: servo/components/style/properties/gecko.mako.rs:277
(Diff revision 2)
>          self.${on_set}();
>          % endif
>      }
>  </%def>
>  
> -<%def name="impl_keyword_clone(ident, gecko_ffi_name, keyword)">
> +<%def name="impl_keyword_clone(ident, gecko_ffi_name, keyword, cast_type)">

Should we specify default value u32?

::: servo/components/style/properties/gecko.mako.rs:283
(Diff revision 2)
>      #[allow(non_snake_case)]
>      pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
>          use properties::longhands::${ident}::computed_value::T as Keyword;
>          // FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts
> +        % if cast_type[0] == 'u' :
>          match ${get_gecko_property(gecko_ffi_name)} ${keyword.maybe_cast("u32")} {

We should use cast_type instead of "u32" to use the default value or specified cast_type.

::: servo/components/style/properties/gecko.mako.rs:291
(Diff revision 2)
> +        % else:
> +            % for value in keyword.values_for('gecko'):
> +            const ${keyword.gecko_constant(value)} : ${cast_type} = structs::${keyword.gecko_constant(value)} ${keyword.maybe_cast(cast_type)};
> +            % endfor

I guess we can define these const values whatever cast_type.  I mean we can define them even if cast_type is unsigned value, no?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:989
(Diff revision 2)
> +        static FONT_STRETCH_FACTOR: [i32; 9] = [0,  // normal
> +                                                -4, // ultra-condensed
> +                                                -3, // extra-condensed
> +                                                -2, // condensed
> +                                                -1, // semi-condensed
> +                                                1,  // semi-expanded
> +                                                2,  // expanded
> +                                                3,  // extra-expanded
> +                                                4]; // ultra-expanded

I don't think that using gecko's specific values is a good idea.  Also I guess we need to define a function that converts font-stretch enum to integer with match for this case.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1003
(Diff revision 2)
> +        let interpolated_enum_index = match FONT_STRETCH_FACTOR.iter().position(|&x| x == (interpolated_value as i32)) {
> +            Some(n) => n as u16,
> +            _ => panic!("Can't find interpolated value from font-stretch enum"),

I think we don't need to iterate over FONT_STRETCH_FACTOR.  We can define a map from  the index to the enum. static FONT_STRETCH_ENUM_MAP: [FontStretch; 9] = [ FontStretch::ultra_condensed, ...];
Attachment #8863594 - Flags: review?(hikezoe)
Comment on attachment 8863595 [details]
Bug 1357663 - Enable web platform test of font-stretch property animation.

https://reviewboard.mozilla.org/r/135122/#review140420

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1315
(Diff revision 2)
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      var animation = target.animate({ [idlName]:
> +                                         ['ultra-condensed',
> +					  'ultra-expanded'] },
> +                                     { duration: 800, fill: 'both' });

I'd prefer to match the duration with other test cases.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1318
(Diff revision 2)
> +                                         ['ultra-condensed',
> +					  'ultra-expanded'] },
> +                                     { duration: 800, fill: 'both' });
> +	testAnimationSamples(
> +          animation, idlName,
> +          [{ time: 450,  expected: 'semi-expanded' }]);

Likewise here.
Attachment #8863595 - Flags: review?(hikezoe)
Comment on attachment 8863595 [details]
Bug 1357663 - Enable web platform test of font-stretch property animation.

https://reviewboard.mozilla.org/r/135122/#review140424

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1333
(Diff revision 2)
> +                                         ['ultra-condensed',
> +					  'ultra-expanded'] },
> +                                     { duration: 800, composite: 'add' });
> +      testAnimationSamples(
> +        animation, idlName,
> +        [{ time: 0, expected: 'semi-condensed' }]);

This value looks wrong to me.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #16)
> Comment on attachment 8863595 [details]
> Bug 1357663 - Enable web platform test of font-stretch property animation.
> 
> https://reviewboard.mozilla.org/r/135122/#review138276
> 
> > I am curious why you chose a different duration.
> 
> I choose this duration in order to make the numbers balance with
> font-stretch's keyword.
> A start time is zero and end time is 800ms, So each step is 100ms or 50ms.
> (i.e. 0ms-49ms / 50ms-149ms / 150ms-249ms / ... / 750ms-800ms)

I don't think we need to choose both edge values for 'from' and 'to' values.  I think we just need two test cases;

1) condensed and semi-condensed; adjacent values
2) extra-condensed and semi-condensed; a value between them
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review140408

Thanks, hiro.

> I wonder why the default value 'u8' was dropped?

We specified the default value 'u8' at impl_keyword mako template definition.

> Should we specify default value u32?

Ah, I think that this name brought confuse.
This cast_type is gecko's type, then u32 is servo's type.
Almost of gecko's keyword defined as uint8_t except font-stretch. The font-stretch is defined as int16_t.

And servo's keyword enum is defined as u32 always.
So I think that we should be default value is u8. But I modified that we specify the this default type at impl_keyword template. So I think that we will not need to specify it here.

> I guess we can define these const values whatever cast_type.  I mean we can define them even if cast_type is unsigned value, no?

I don't think we need to define these const values always.
There are some reasons:
 - The unsinged type does not need to cast to cast_type. As mentioned above comment, cast_type is gecko's type.
 - We cant cast the keyword which have gecko_enum_prefix. We prevent to cast these keyword's enum on maybe_cast() when keyword has this prefix[1], and these keyword type is u8. So I think that we will need to check further more that keyword has gecko_enum_prefix if we define these const values on unsinged type.

[1] http://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/servo/components/style/properties/data.py#126
Comment on attachment 8863595 [details]
Bug 1357663 - Enable web platform test of font-stretch property animation.

https://reviewboard.mozilla.org/r/135122/#review140424

> This value looks wrong to me.

An Initial value is 'semi-condensed', it is defined as -1 on gecko. And animation's distance is 8 (ultra-condensed to ultra-expanded).
I thought that the computed value which curentTime is zero will be -1(i.e. 'semi-condensed'). Is it wrong?
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #24)
> Comment on attachment 8863595 [details]
> Bug 1357663 - Enable web platform test of font-stretch property animation.
> 
> https://reviewboard.mozilla.org/r/135122/#review140424
> 
> > This value looks wrong to me.
> 
> An Initial value is 'semi-condensed', it is defined as -1 on gecko. And
> animation's distance is 8 (ultra-condensed to ultra-expanded).
> I thought that the computed value which curentTime is zero will be -1(i.e.
> 'semi-condensed'). Is it wrong?

I am really confused. Where did the 'semi-condensed' value come from at 0ms? Also, what does the distance mean?
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #23)
> Comment on attachment 8863594 [details]
> Bug 1357663 - Make font-stretch animatable.
> 
> https://reviewboard.mozilla.org/r/135120/#review140408
> 
> Thanks, hiro.
> 
> > I wonder why the default value 'u8' was dropped?
> 
> We specified the default value 'u8' at impl_keyword mako template definition.
> 
> > Should we specify default value u32?
> 
> Ah, I think that this name brought confuse.
> This cast_type is gecko's type, then u32 is servo's type.
> Almost of gecko's keyword defined as uint8_t except font-stretch. The
> font-stretch is defined as int16_t.
> 
> And servo's keyword enum is defined as u32 always.
> So I think that we should be default value is u8. But I modified that we
> specify the this default type at impl_keyword template. So I think that we
> will not need to specify it here.
> 
> > I guess we can define these const values whatever cast_type.  I mean we can define them even if cast_type is unsigned value, no?
> 
> I don't think we need to define these const values always.
> There are some reasons:
>  - The unsinged type does not need to cast to cast_type. As mentioned above
> comment, cast_type is gecko's type.
>  - We cant cast the keyword which have gecko_enum_prefix. We prevent to cast
> these keyword's enum on maybe_cast() when keyword has this prefix[1], and
> these keyword type is u8. So I think that we will need to check further more
> that keyword has gecko_enum_prefix if we define these const values on
> unsinged type.

What I meant is that if we define the const values (even if it's not really needed), we can avoid the if else block entirely.  The codes inside if else block is pretty similar, we should avoid the duplicated code.
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review141456

::: servo/components/style/properties/helpers/animated_properties.mako.rs:983
(Diff revision 3)
>  }
>  
> +impl Interpolate for FontStretch {
> +    #[inline]
> +    fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
> +        use self::FontStretch::{normal, ultra_condensed, extra_condensed, condensed,

Nit: FontStretch::*;

::: servo/components/style/properties/helpers/animated_properties.mako.rs:986
(Diff revision 3)
> +        // An enum of FontStreatch is different from gecko's constant definition
> +        // since servo's first keyword is used to initial value.
> +        // So we should treat 'normal' as the value which between semi-condensed
> +        // and semi-expanded.

I don't think we need gecko specific comments here.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:993
(Diff revision 3)
> +        let a = match FONT_STRETCH_ENUM_MAP.iter().position(|&x| x == *self) {
> +            Some(n) => n as f64,
> +            None => panic!("Can't find enum value of self value. "),
> +        };

Instead of iterating over FONT_STRETCH_ENUM_MAP, can't we simply do just like below?

let this = match *self {
  ultra_condensed => 1,
  ...
  ultra_expanded => 9,
};

Also, I think we can this match as a closure function and can be reused for *other*.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2634
(Diff revision 3)
> +        // An enum of FontStreatch is different from gecko's constant definition since
> +        // servo's first keyword is used to initial value.
> +        // So we should treat normal as the value which between semi-condensed and
> +        // semi-expanded.

I don't think this comment is needed.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2638
(Diff revision 3)
> +        static FONT_STRETCH_ENUM_MAP: [FontStretch; 9] =
> +            [ ultra_condensed, extra_condensed, condensed, semi_condensed, normal,
> +              semi_expanded, expanded, extra_expanded, ultra_expanded ];
> +        let a = match FONT_STRETCH_ENUM_MAP.iter().position(|&x| x == *self) {
> +            Some(n) => n as f64,
> +            None => panic!("Can't find enum value of self value. "),
> +        };
> +        let b = match FONT_STRETCH_ENUM_MAP.iter().position(|&x| x == *other) {
> +            Some(n) => n as f64,
> +            None => panic!("Can't find enum value of other value. "),
> +        };

This is pretty similar to the above code. We should factor this out.
Attachment #8863594 - Flags: review?(hikezoe)
Comment on attachment 8863595 [details]
Bug 1357663 - Enable web platform test of font-stretch property animation.

https://reviewboard.mozilla.org/r/135122/#review140424

Thanks.

> An Initial value is 'semi-condensed', it is defined as -1 on gecko. And animation's distance is 8 (ultra-condensed to ultra-expanded).
> I thought that the computed value which curentTime is zero will be -1(i.e. 'semi-condensed'). Is it wrong?

As mentioned at bug 1363246 comment 3, css-fonts-4 specification explain that we will need to treat font-stretch as number, but this spec is editor's draft. As discussed with you, we will drop this additive test.
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review142912

::: servo/components/style/properties/gecko.mako.rs:1888
(Diff revision 4)
>                                              "ruby-text ruby-text-container contents flow-root -webkit-box " +
>                                              "-webkit-inline-box -moz-box -moz-inline-box -moz-grid -moz-inline-grid " +
>                                              "-moz-grid-group -moz-grid-line -moz-stack -moz-inline-stack -moz-deck " +
>                                              "-moz-popup -moz-groupbox",
>                                              gecko_enum_prefix="StyleDisplay",
> +                                            gecko_inexhaustive="True",

I think we will change to the display property if we want to use same clone function.

The display property should have the gecko_inexhaustive='True'. It was removed at bug 1322185 patch 3. But I think that it will need this property.
[1] https://reviewboard.mozilla.org/r/97428/diff/1#index_header

We need this gecko_inexhaustive for covering all value when matching gecko's value since we will treat enum variable[2] as u8 here in order to use same function. 
[2] http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/servo/components/style/gecko/generated/structs_debug.rs#22911

Other properties which has gecko_enum_prefix are specified gecko_inexhaustive='True', or it will not need to generate clone function since property is not animatable. (e.g. animation-direction property)
Comment on attachment 8863595 [details]
Bug 1357663 - Enable web platform test of font-stretch property animation.

https://reviewboard.mozilla.org/r/135122/#review142914

IIRC, we need to annotate FAIL for missing testAddition of this font-stretch property.
r=me with that.
Attachment #8863595 - Flags: review?(hikezoe) → review+
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review142916

::: servo/components/style/properties/gecko.mako.rs:294
(Diff revision 4)
>              % endfor
>              % if keyword.gecko_inexhaustive:
>              x => panic!("Found unexpected value in style struct for ${ident} property: {:?}", x),
>              % endif
>          }
> +        % endif

Where did this 'endif' come from?
Attachment #8863594 - Flags: review?(hikezoe)
Comment on attachment 8863595 [details]
Bug 1357663 - Enable web platform test of font-stretch property animation.

https://reviewboard.mozilla.org/r/135122/#review142914

Thanks.

I added anotate for it.
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review142916

> Where did this 'endif' come from?

Sorry. I forgot removing experimental code.

Could you please confirm again?
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review142926

::: servo/components/style/properties/gecko.mako.rs:277
(Diff revision 5)
>          self.${on_set}();
>          % endif
>      }
>  </%def>
>  
> -<%def name="impl_keyword_clone(ident, gecko_ffi_name, keyword)">
> +<%def name="impl_keyword_clone(ident, gecko_ffi_name, keyword, cast_type)">

Can't we define the default argument for cast_type? I mean we should use cast_type='u8' here.

::: servo/components/style/properties/gecko.mako.rs:283
(Diff revision 5)
>      pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
>          use properties::longhands::${ident}::computed_value::T as Keyword;
>          // FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts
> -        match ${get_gecko_property(gecko_ffi_name)} ${keyword.maybe_cast("u32")} {
> -            % for value in keyword.values_for('gecko'):
> +        % for value in keyword.values_for('gecko'):
> -            structs::${keyword.gecko_constant(value)} => Keyword::${to_rust_ident(value)},
> +        const CLONE_${keyword.gecko_constant(value).upper().replace("::", "_")} : ${cast_type} = structs::${keyword.gecko_constant(value)} as ${cast_type};

We should define a new method to Keyword class [1], I guess in the case of non-gecko_enum_prefix type we can use gecko_constant as it is.

[1] https://hg.mozilla.org/mozilla-central/file/3e166b683893/servo/components/style/properties/data.py#l57

Also, CLONE_ prefix does not represent what the value means actually.  How about using hungarian notation?  Honestelly I don't generally prefer hubgarian notation, but in this case I am OK with that.

::: servo/components/style/properties/gecko.mako.rs:1924
(Diff revision 5)
>      pub fn copy_display_from(&mut self, other: &Self) {
>          self.gecko.mDisplay = other.gecko.mDisplay;
>          self.gecko.mOriginalDisplay = other.gecko.mDisplay;
>      }
>  
> -    <%call expr="impl_keyword_clone('display', 'mDisplay', display_keyword)"></%call>
> +    <%call expr="impl_keyword_clone('display', 'mDisplay', display_keyword, 'u8')"></%call>

If we define the default argument for cast_type as I mentioned above, we don't need this 'u8' here.  Actually this 'u8' is hard to understand what it means, if we really need this here, we should use "cast_type='u8'" instead.

::: servo/components/style/properties/gecko.mako.rs:3388
(Diff revision 5)
>  
>      <% text_align_keyword = Keyword("text-align",
>                                      "start end left right center justify -moz-center -moz-left -moz-right char",
>                                      gecko_strip_moz_prefix=False) %>
>      ${impl_keyword('text_align', 'mTextAlign', text_align_keyword, need_clone=False)}
> -    ${impl_keyword_clone('text_align', 'mTextAlign', text_align_keyword)}
> +    ${impl_keyword_clone('text_align', 'mTextAlign', text_align_keyword, 'u8')}

Likewise.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1366
(Diff revision 5)
> +fn convert_font_stretch_from_enum(stretch: FontStretch) -> f64 {
> +    use self::FontStretch::*;
> +    match stretch {
> +        ultra_condensed => 1.0,
> +        extra_condensed => 2.0,
> +        condensed       => 3.0,
> +        semi_condensed  => 4.0,
> +        normal          => 5.0,
> +        semi_expanded   => 6.0,
> +        expanded        => 7.0,
> +        extra_expanded  => 8.0,
> +        ultra_expanded  => 9.0,
> +    }
> +}

I think we can implement From trait instead of this function?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1381
(Diff revision 5)
> +        extra_expanded  => 8.0,
> +        ultra_expanded  => 9.0,
> +    }
> +}
> +
> +fn convert_font_stretch_to_enum(stretch_number: f64) -> FontStretch {

Likewise this.
Attachment #8863594 - Flags: review?(hikezoe)
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review142926

> We should define a new method to Keyword class [1], I guess in the case of non-gecko_enum_prefix type we can use gecko_constant as it is.
> 
> [1] https://hg.mozilla.org/mozilla-central/file/3e166b683893/servo/components/style/properties/data.py#l57
> 
> Also, CLONE_ prefix does not represent what the value means actually.  How about using hungarian notation?  Honestelly I don't generally prefer hubgarian notation, but in this case I am OK with that.

I think we can't lower-case constant in rust, so I think that we can't use hungarian notation proper. In this patch, I changed prefix to "K_" which mean constant prefix "k" of hungarian notation.
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review143344

Thanks for doing!

r=me with comments addressed

::: servo/components/style/properties/data.py:130
(Diff revision 6)
> +    def definable_const_id(self, value):
> +        if self.gecko_enum_prefix is None:
> +            return self.gecko_constant(value)
> +        else:
> +            return self.gecko_constant(value).upper().replace("::", "_")

Hungarian what I meant is something like 'U8_XX', so this function should return the cast_type as prefix.  Also this function should raise TypeError() if cast_type is not defined.

::: servo/components/style/properties/data.py:130
(Diff revision 6)
>          return self.gecko_enum_prefix is None
>  
>      def maybe_cast(self, type_str):
>          return "as " + type_str if self.needs_cast() else ""
>  
> +    def definable_const_id(self, value):

The function name should be casted_constant_name or something.

::: servo/components/style/properties/gecko.mako.rs:259
(Diff revision 6)
>                                      "*self.gecko.__bindgen_anon_1.mBorderColor.as_mut()")
>          return "unsafe { %s = %s };" % (ffi_name, expr)
>      return "self.gecko.%s = %s;" % (ffi_name, expr)
>  %>
>  
> -<%def name="impl_keyword_setter(ident, gecko_ffi_name, keyword, cast_type='u8', on_set=None)">
> +<%def name="impl_keyword_setter(ident, gecko_ffi_name, keyword, cast_type, on_set=None)">

I don't think removing the default argument is a good idea.  What happens if someone calls this function directly?

::: servo/components/style/properties/gecko.mako.rs:283
(Diff revision 6)
>      pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
>          use properties::longhands::${ident}::computed_value::T as Keyword;
>          // FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts
> -        match ${get_gecko_property(gecko_ffi_name)} ${keyword.maybe_cast("u32")} {
> -            % for value in keyword.values_for('gecko'):
> +        % for value in keyword.values_for('gecko'):
> -            structs::${keyword.gecko_constant(value)} => Keyword::${to_rust_ident(value)},
> +        const K_${keyword.definable_const_id(value)} : ${cast_type} = structs::${keyword.gecko_constant(value)} as ${cast_type};

We should add comment about why we need to cast here.  Something like thie:

Some constant macro in gecko are defined as negative integer and they are converted to signed integer in Rust bindings.  In the case where we have both signed/unsigned integers here, we need to cast them as signed type so that we can use them as match's arms.  Also, to avoid signed/unsigned type branches we force to cast even if we have only signed values.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1373
(Diff revision 6)
> +    #[inline]
> +    fn add_weighted(&self, other: &Self, self_portion: f64, other_portion: f64) -> Result<Self, ()> {
> +        let from = f64::from(*self);
> +        let to   = f64::from(*other);
> +        let interpolated_mapped_index = ((from * self_portion + to * other_portion) + 0.5).floor();
> +        Ok(FontStretch::from(interpolated_mapped_index))

Nit: we can use into() here.

interpolated_mapped_index.into()

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2347
(Diff revision 6)
>  
>          matrix
>      }
>  }
>  
> +impl From<FontStretch> for f64 {

We should add some comments about this trait, it's used for interpolation of font-stretch property as interger number.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2364
(Diff revision 6)
> +impl From<f64> for FontStretch {
> +    fn from(stretch_number: f64) -> FontStretch {
> +        use self::FontStretch::*;
> +        debug_assert!(stretch_number >= 1.0 && stretch_number <= 9.0);
> +        static FONT_STRETCH_ENUM_MAP: [FontStretch; 9] =
> +            [ ultra_condensed, extra_condensed, condensed, semi_condensed, normal,
> +              semi_expanded, expanded, extra_expanded, ultra_expanded ];
> +        FONT_STRETCH_ENUM_MAP[(stretch_number - 1.0) as usize]
> +    }
> +}

You can write this from() as into() in above From trait.
Attachment #8863594 - Flags: review?(hikezoe) → review+
Comment on attachment 8863594 [details]
Bug 1357663 - Make font-stretch animatable.

https://reviewboard.mozilla.org/r/135120/#review143344

Thank you so much.

> We should add some comments about this trait, it's used for interpolation of font-stretch property as interger number.

I added the comment and moved this trait under the Animatable trait of FontStretch.
However we should treat font-streth proerty as 'real number' not 'integer number' according to css-fonts-3 and css-fonts-4.
Priority: -- → P1
Attachment #8863594 - Attachment is obsolete: true
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #48)
> Comment on attachment 8863595 [details]
> Bug 1357663 - Enable web platform test of font-stretch property animation.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/135122/diff/8-9/

Try result of previous patch is failed.(accumulation-per-property.html)
I added meta information of this fail since same reason of comment 30.

Retry:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05668bed9fffa1d23dbd77631548918855c28dd6
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b4a8c54c2b05
Enable web platform test of font-stretch property animation. r=hiro
https://hg.mozilla.org/mozilla-central/rev/b4a8c54c2b05
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: