Closed Bug 1356124 Opened 7 years ago Closed 7 years ago

stylo: implement font-variant-alternates properly

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox55 --- disabled
firefox56 --- fixed

People

(Reporter: hiro, Assigned: canova)

References

Details

Attachments

(2 files)

Bug 1356124 - Part 1: Fix font-variant-alternates property
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1356124 - Part 2: Update test expectation for font-variant-alternates property
59 bytes, text/x-review-board-request
emilio
: review+
Details 
In bug 1354876, I did give up implementing font-variant-alternates properly.
We need to parse feature-value-name for each value and set the values to nsFont::alternateValues.
I'm working on this.
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
I still need to see the results of the try run but it seems correct to me :)
Comment on attachment 8881914 [details]
Bug 1356124 - Part 1: Fix font-variant-alternates property

https://reviewboard.mozilla.org/r/152974/#review158298

So this looks fine, but it seems like a pretty big struct  for a list of variants that would usually be small... Perhaps this could be something like:

```
enum VariantAlternate {
    HistoricalForms,
    // ...
    Stylistic(CustomIdent),
    // ...
    StyleSet(Box<[CustomIdent]>),
}
```

and make the specified value `Box<[VariantAlternate]>`?

I wouldn't care a lot if this was because of the longhand, but we'll get a PropertyDeclarationBlock with this each time anyone sets the "font" longhand, WDYT?

::: servo/components/style/properties/longhand/font.mako.rs:1296
(Diff revision 2)
> -            const CHARACTER_VARIANT = 0x08,
> -            const SWASH = 0x10,
> -            const ORNAMENTS = 0x20,
> -            const ANNOTATION = 0x40,
> +        pub ornaments: Option<CustomIdent>,
> +        pub annotation: Option<CustomIdent>,
> +        pub historical_forms: bool,
> +    }
> +
> +    impl Default for VariantAlternates {

`#[derive(Default)]`.

::: servo/components/style/properties/longhand/font.mako.rs:1426
(Diff revision 2)
> -                return Err(StyleParseError::UnspecifiedError.into())
> +                    return Err(StyleParseError::UnspecifiedError.into())
> -            }
> +                }
> -            result.insert(flag);
> +                empty = false;
> +            )
> +        );
> +        while let Ok(_) = input.try(|input| {

I think this would be nicer matching on the token:

```
match input.next()? {
    Token::Ident(..) => { ... }
    Token::Function(..) => { ... }
};
```
Comment on attachment 8881994 [details]
Bug 1356124 - Part 2: Update test expectation for font-variant-alternates property

https://reviewboard.mozilla.org/r/153066/#review158310

::: layout/style/test/stylo-failures.md:77
(Diff revision 1)
>    * place-{content,items,self} shorthands bug 1363971
>      * test_align_shorthand_serialization.html [6]
>    * system font serialization with subprop specified bug 1364286
>      * test_system_font_serialization.html [3]
>    * serialize subprops to -moz-use-system-font when using system font bug 1364289
> -    * test_value_storage.html `'font'` [224]
> +    * test_value_storage.html `'font'` [240]

Why the new failures?
Comment on attachment 8881994 [details]
Bug 1356124 - Part 2: Update test expectation for font-variant-alternates property

https://reviewboard.mozilla.org/r/153066/#review158322
Attachment #8881994 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8881914 [details]
Bug 1356124 - Part 1: Fix font-variant-alternates property

https://reviewboard.mozilla.org/r/152974/#review158562

r=me with those.

::: layout/style/ServoBindings.cpp:1246
(Diff revision 3)
> +}
> +
> +void
> +Gecko_AppendAlternateValues(nsFont* aFont, uint32_t aAlternateName, nsIAtom* aAtom)
> +{
> +  gfxAlternateValue v;

Let's do:

```
aFont->alternateValues.AppendElement(gfxAlternateValue {
  aAlternateName,
  nsDependentAtomString(aAtom)
})
```

so that if someone adds more members to that struct we'll notice.

::: layout/style/ServoBindings.cpp:1253
(Diff revision 3)
> +  v.value = nsDependentAtomString(aAtom);
> +  aFont->alternateValues.AppendElement(v);
> +}
> +
> +void
> +Gecko_CopyAlternateValuesFrom(nsFont* aDest, nsFont* aSrc)

Let's make `aSrc` a `const nsFont*`.

::: servo/components/style/properties/gecko.mako.rs:1892
(Diff revision 3)
>      pub fn copy_font_variant_alternates_from(&mut self, other: &Self) {
> +        use gecko_bindings::bindings::Gecko_CopyAlternateValuesFrom;
> +
>          self.gecko.mFont.variantAlternates = other.gecko.mFont.variantAlternates;
> -        // FIXME: Copy alternateValues as well.
> -        // self.gecko.mFont.alternateValues = other.gecko.mFont.alternateValues;
> +        unsafe {
> +            Gecko_CopyAlternateValuesFrom(& mut self.gecko.mFont, &other.gecko.mFont as *const _ as *mut _);

Then you don't need this nasty cast.
Attachment #8881914 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8881914 [details]
Bug 1356124 - Part 1: Fix font-variant-alternates property

https://reviewboard.mozilla.org/r/152974/#review158568

::: servo/components/style/properties/gecko.mako.rs:1892
(Diff revision 3)
>      pub fn copy_font_variant_alternates_from(&mut self, other: &Self) {
> +        use gecko_bindings::bindings::Gecko_CopyAlternateValuesFrom;
> +
>          self.gecko.mFont.variantAlternates = other.gecko.mFont.variantAlternates;
> -        // FIXME: Copy alternateValues as well.
> -        // self.gecko.mFont.alternateValues = other.gecko.mFont.alternateValues;
> +        unsafe {
> +            Gecko_CopyAlternateValuesFrom(& mut self.gecko.mFont, &other.gecko.mFont as *const _ as *mut _);

Also, tere's an extra space in `& mut`.
Thanks for the review. I'll trigger one more try run before landing this.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9895f7930785
Part 1: Fix font-variant-alternates property r=emilio
https://hg.mozilla.org/integration/autoland/rev/43856b83fe21
Part 2: Update test expectation for font-variant-alternates property r=emilio
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fb39e4135884
Add AlternateValues bindings to hazard whitelist r=me
https://hg.mozilla.org/mozilla-central/rev/9895f7930785
https://hg.mozilla.org/mozilla-central/rev/43856b83fe21
https://hg.mozilla.org/mozilla-central/rev/fb39e4135884
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: