Closed Bug 1354876 Opened 7 years ago Closed 7 years ago

stylo: implement font-variant

Categories

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

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

Tracking

()

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

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(3 files, 2 obsolete files)

Bug 1354876 - Update tests expectations for font-variant.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1354876 - Use literal numbers to shift bit for macros of font-variant-{alternates, east-asian, numeric, ligatures}.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1354876 - Add variables for represents 'normal' value for font-variant-{alternates, east-asian, numeric, ligatures}.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
      No description provided.
Priority: -- → P2
Blocks: 1351978
Assignee: nobody → hikezoe
This needs for checking whether font parsing for canvas stuff (bug 1324700) works fine or not.
Comment on attachment 8856322 [details]
Bug 1354876 - Update tests expectations for font-variant.

https://reviewboard.mozilla.org/r/128220/#review130724

::: layout/style/test/stylo-failures.md:196
(Diff revision 1)
>      * test_compute_data_with_start_struct.html `font-variant` [8]
>      * test_inherit_computation.html `font-variant` [20]
>      * test_inherit_storage.html `font-variant` [36]
> -    * test_initial_computation.html `font-variant` [10]
> +    * test_initial_computation.html `font-variant` [8]
>      * test_initial_storage.html `font-variant` [18]
> -    * test_value_storage.html `font-variant` [332]
> +    * test_value_storage.html `font-variant` [333]

I actually have no idea why the failure count is increased, but I suspect a test starts working with handling font-variant, and as a result the test failed because of other reasons.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> ::: layout/style/test/stylo-failures.md:196
> (Diff revision 1)
> >      * test_compute_data_with_start_struct.html `font-variant` [8]
> >      * test_inherit_computation.html `font-variant` [20]
> >      * test_inherit_storage.html `font-variant` [36]
> > -    * test_initial_computation.html `font-variant` [10]
> > +    * test_initial_computation.html `font-variant` [8]
> >      * test_initial_storage.html `font-variant` [18]
> > -    * test_value_storage.html `font-variant` [332]
> > +    * test_value_storage.html `font-variant` [333]
> 
> I actually have no idea why the failure count is increased, but I suspect a
> test starts working with handling font-variant, and as a result the test
> failed because of other reasons.

Yeah that sounds reasonable. Adding 1 failure to 332 existing ones probably isn't worth worrying about.
Attachment #8856321 - Flags: review?(cam)
Attachment #8856322 - Flags: review?(cam)
Currently:

* Gecko supports a font-variant shorthand (from CSS Fonts 3), whose longhand components are: font-variant-alternates, font-variant-caps, font-variant-east-asian, font-variant-numeric, font-variant-position
* Servo itself supports font-variant as a longhand (from CSS 2.1)
* Stylo (Servo with the Gecko product) supports font-variant-caps

We need to support font-variant as a shorthand (and all of its longhand components) for Stylo at some point, so I think we should do that now, rather than add support for the CSS 2.1 font-variant.

One other thing to note is that the set of font variant values supported in font-variant in CSS 2.1 (normal, small-caps) is a subset of those supported by font-variant-caps in CSS Fonts 3 (normal, small-caps, all-small, petite-caps, unicase, titling-caps).

Since adding support for the other font-variant-XXX longhand components to Servo itself would be hard (we need to add the actual font/layout support), I think we should do this:

* make font-variant a shorthand for both products in Servo
* use products="gecko" to add support for the remaining longhand components (font-variant-alternates, font-variant-east-asian, font-variant-numeric, font-variant-position) only for Stylo
* use conditional compilation to make it so that for the Gecko product, we have all 5 font-variant-XXX longhands be components of the font-variant shorthand, and for the Servo product, have font-variant-caps be the only longhand component of font-variant
* use extra_gecko_values="" on the font-variant-caps definition to make it so that only all-small, petite-caps, unicase and titling-caps are supported in the Gecko product
Thank you Cameron. I decided to not implement glue code for font-variant-alternates perfectly. That's because it's a bit tricky and servo does not parse it yet, so the glue code will depends on the parser, I guess.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e12125572c3f8d874d77b7129b054c0a4b17942f
Comment on attachment 8857263 [details]
Bug 1354876 - Use literal numbers to shift bit for macros of font-variant-{alternates, east-asian, numeric, ligatures}.

https://reviewboard.mozilla.org/r/129194/#review131786

::: gfx/thebes/gfxFontConstants.h:63
(Diff revision 1)
>  
>      eFeatureAlternates_numFeatures
>  };
>  
>  // alternates - simple enumerated values
> -#define NS_FONT_VARIANT_ALTERNATES_HISTORICAL        (1 << eFeatureAlternates_historical)
> +const uint8_t NS_FONT_VARIANT_ALTERNATES_HISTORICAL        = (1 << eFeatureAlternates_historical);

I am worried that the C++ compiler will generate storage for these constants, and that C++ code that refers to them will look up memory instead of using the constant value.

There are no existing uses of the eFeatureXXX_YYY values, apart from the #defines for the NS_FONT_VARIANT_* macros, and the use of eFeatureXXX_numFeatures in nsFont.cpp.

So I suggest we get rid of the enums and make the #defines look like this:

  #define NS_FONT_VARIANT_ALTERNATES_HISTORICAL  (1 << 0) 
  #define NS_FONT_VARIANT_ALTERNATES_STYLISTIC   (1 << 1)
  ...
  #define NS_FONT_VARIANT_ALTERNATES_ANNOTATION  (1 << 6)
  #define NS_FONT_VARIANT_ALTERNATES_COUNT       7

rust-bindgen does support expressions like this in #defines (for example we do that for NS_STYLE_WILL_CHANGE_* values, and they are generated correctly).
Attachment #8857263 - Flags: review?(cam) → review-
Comment on attachment 8857264 [details]
Bug 1354876 - Add variables for represents 'normal' value for font-variant-{alternates, east-asian, numeric, ligatures}.

https://reviewboard.mozilla.org/r/129196/#review131788
Attachment #8857264 - Flags: review?(cam) → review+
Comment on attachment 8857265 [details]
Bug 1354876 - Add font-variant-{alternates,east-asian,ligutures,numeric}.

https://reviewboard.mozilla.org/r/129198/#review131810

::: servo/components/style/properties/gecko.mako.rs:1406
(Diff revision 1)
> +        let result = match v {
> +            Keyword::normal => 0 as u16,
> +            Keyword::historical_forms => NS_FONT_VARIANT_ALTERNATES_HISTORICAL as u16,
> +            Keyword::stylistic => NS_FONT_VARIANT_ALTERNATES_STYLISTIC as u16,
> +            Keyword::styleset => NS_FONT_VARIANT_ALTERNATES_STYLESET as u16,
> +            Keyword::character_variant => NS_FONT_VARIANT_ALTERNATES_CHARACTER_VARIANT as u16,
> +            Keyword::swash => NS_FONT_VARIANT_ALTERNATES_SWASH as u16,
> +            Keyword::ornaments => NS_FONT_VARIANT_ALTERNATES_ORNAMENTS as u16,
> +            Keyword::annotation => NS_FONT_VARIANT_ALTERNATES_ANNOTATION as u16,
> +        };
> +        self.gecko.mFont.variantAlternates = result;

You can remove all of the "as u16"s, and do it once in the assignment to variantAlternates.

::: servo/components/style/properties/longhand/font.mako.rs:862
(Diff revision 1)
> +${helpers.single_keyword("font-variant-alternates",
> +                         "normal historical-forms stylistic styleset character-variant swash ornaments annotation",
> +                         products="gecko",
> +                         spec="https://drafts.csswg.org/css-fonts/#propdef-font-variant-alternates",
> +                         animation_type="none")}

I don't think using single_keyword is right here, since that is only for properties that take a single keyword.  Only normal and historical-forms can be specified as a keyword -- all the rest must be specified as functions.

::: servo/components/style/properties/longhand/font.mako.rs:869
(Diff revision 1)
> +${helpers.single_keyword("font-variant-east-asian",
> +                         "normal jis78 jis83 jis90 jis04 simplified traditional full-width proportional-width ruby",
> +                         products="gecko",
> +                         gecko_ffi_name="mFont.variantEastAsian",
> +                         gecko_constant_prefix="NS_FONT_VARIANT_EAST_ASIAN",
> +                         custom_consts=font_variant_east_asian_custom_consts,
> +                         cast_type='u16',
> +                         spec="https://drafts.csswg.org/css-fonts/#propdef-font-variant-east-asian",
> +                         animation_type="none")}

Also this isn't quite right either, since the property is defined to parse as:

normal | [ <east-asian-variant-values> || <east-asian-width-values> || ruby ]

which means there can be up to three keywords specified in the value, but single_keyword defines a property that only accepts a single keyword.

::: servo/components/style/properties/longhand/font.mako.rs:885
(Diff revision 1)
> +${helpers.single_keyword("font-variant-ligatures",
> +                         "normal none common-ligatures no-common-ligatures discretionary-ligatures\
> +                          no-discretionary-ligatures historical-ligatures no-historical-ligatures \
> +                          contextual no-contextual",
> +                         products="gecko",
> +                         gecko_ffi_name="mFont.variantLigatures",
> +                         gecko_constant_prefix="NS_FONT_VARIANT_LIGATURES",
> +                         custom_consts=font_variant_ligatures_custom_consts,
> +                         cast_type='u16',
> +                         spec="https://drafts.csswg.org/css-fonts/#propdef-font-variant-ligatures",
> +                         animation_type="none")}

Same here too; we can't just use single_keyword since the property is defined to parse as:

normal | none | [ <common-lig-values> || <discretionary-lig-values> || <historical-lig-values> || <contextual-alt-values> ]

::: servo/components/style/properties/longhand/font.mako.rs:901
(Diff revision 1)
> +${helpers.single_keyword("font-variant-numeric",
> +                         "normal lining-nums oldstyle-nums proportional-nums tabular-nums \
> +                         diagonal-fractions stacked-fractions slashzero ordinal",
> +                         products="gecko",
> +                         gecko_ffi_name="mFont.variantNumeric",
> +                         gecko_constant_prefix="NS_FONT_VARIANT_NUMERIC",
> +                         custom_consts=font_variant_numeric_custom_consts,
> +                         spec="https://drafts.csswg.org/css-fonts/#propdef-font-variant-numeric",
> +                         animation_type="none")}

And here:

normal | [ <numeric-figure-values> || <numeric-spacing-values> || <numeric-fraction-values> || ordinal || slashed-zero ]
Attachment #8857265 - Flags: review?(cam) → review-
Comment on attachment 8856321 [details]
Bug 1354876 - Implement font-variant shorthand.

https://reviewboard.mozilla.org/r/128218/#review131824

r=me if your intention is to handle the parsing and setting of the other longhand components of font-variant in a followup bug (and please file that bug!), or r- if you meant to handle them in this bug. :-)

::: servo/components/style/properties/shorthand/font.mako.rs:166
(Diff revision 2)
> +            if caps.is_none() {
> +                if let Ok(value) = input.try(|input| font_variant_caps::parse(context, input)) {
> +                    caps = Some(value);
> +                    continue
> +                }
> +            }

We need to try to parse all the rest of the longhand component values too, inside a |% if product == "gecko"|, right?

::: servo/components/style/properties/shorthand/font.mako.rs:184
(Diff revision 2)
> +            % if product == "gecko" or data.testing:
> +                % for name in gecko_sub_properties:
> +                    font_variant_${name}: font_variant_${name}::get_initial_specified_value(),
> +                % endfor
> +            % endif

Oh, are you leaving the parsing of these other values until a later bug?  If so, please add a comment here and above that we need to parse those values.
Attachment #8856321 - Flags: review?(cam) → review+
Comment on attachment 8856322 [details]
Bug 1354876 - Update tests expectations for font-variant.

https://reviewboard.mozilla.org/r/128220/#review131828
Attachment #8856322 - Flags: review?(cam) → review+
See Also: → 1356134
Comment on attachment 8856321 [details]
Bug 1354876 - Implement font-variant shorthand.

https://reviewboard.mozilla.org/r/128218/#review131824

> Oh, are you leaving the parsing of these other values until a later bug?  If so, please add a comment here and above that we need to parse those values.

Filed 1356134.
Comment on attachment 8857263 [details]
Bug 1354876 - Use literal numbers to shift bit for macros of font-variant-{alternates, east-asian, numeric, ligatures}.

https://reviewboard.mozilla.org/r/129194/#review132396
Attachment #8857263 - Flags: review?(cam) → review+
Comment on attachment 8857265 [details]
Bug 1354876 - Add font-variant-{alternates,east-asian,ligutures,numeric}.

https://reviewboard.mozilla.org/r/129198/#review132398

Nice work!

::: servo/components/style/properties/gecko.mako.rs:289
(Diff revision 2)
> +    pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
> +        % for gecko_bit in bit_map.values():
> +        use gecko_bindings::structs::${gecko_bit_prefix}${gecko_bit};
> +        % endfor
> +
> +        let mut bits: ${cast_type} = 0;

Maybe add a comment in here saying that we could make this faster if we ensured that the Servo bitflags storage is the same as Gecko's storage.

::: servo/components/style/properties/gecko.mako.rs:296
(Diff revision 2)
> +        if v.contains(longhands::${ident}::${servo_bit}) {
> +            bits |= ${gecko_bit_prefix}${gecko_bit} as ${cast_type};
> +        }
> +        % endfor
> +
> +        self.gecko.${gecko_ffi_name}= bits as ${cast_type};

Nit: space before "=".

::: servo/components/style/properties/gecko.mako.rs:1435
(Diff revision 2)
> +        self.gecko.mFont.variantAlternates = other.gecko.mFont.variantAlternates;
> +        // FIXME: Copy alternateValues as well.
> +        // self.gecko.mFont.alternateValues = other.gecko.mFont.alternateValues;
> +    }
> +
> +    ///                                servo_bit: gecko_bit

Use "//" rather than "///", since "///" means a doc comment in Rust, and it will apply to the function generated by impl_bitflags_setter below.
Attachment #8857265 - Flags: review?(cam) → review+
Attachment #8857265 - Attachment is obsolete: true
Attachment #8856321 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b72eb1ddad0
Use literal numbers to shift bit for macros of font-variant-{alternates, east-asian, numeric, ligatures}. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d29c3abdf9ff
Add variables for represents 'normal' value for font-variant-{alternates, east-asian, numeric, ligatures}. r=heycam
https://hg.mozilla.org/integration/autoland/rev/89af05c8331c
Update tests expectations for font-variant. r=heycam
Thank you Cameron for landing patches!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: