Closed Bug 1364286 Opened 7 years ago Closed 7 years ago

stylo: serialize system font correctly when some subprops have value specified

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: xidorn, Assigned: manishearth)

References

Details

Attachments

(1 file)

Bug 1364286 - Fix serialization of system fonts;
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
For example, "font: menu; font-family: inherit;" should be serialized to itself, rather than
> font-style: ; font-variant-caps: ; ... font-family: inherit; font-size: ;
which is invalid.

This should probably be a special case in serialization of declaration.

In addition, if we decide not to have -moz-use-system-font, we need to adjust layout/style/test/test_system_font_serialization.html to skip some of its checks.
Manish, I guess you may be interested in taking this?
Depends on: 1349417
Flags: needinfo?(manishearth)
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Flags: needinfo?(manishearth)
I need to add some comments but I might not get the chance till monday. Till then, this is a working patch.
Comment on attachment 8867420 [details]
Bug 1364286 - Fix serialization of system fonts;

https://reviewboard.mozilla.org/r/138956/#review142352

::: servo/components/style/properties/declaration_block.rs:435
(Diff revision 1)
> +                    if found_system.is_some() {
> +                        current_longhands = current_longhands.into_iter()
> +                                                             .filter(|l| l.get_system().is_some() ||
> +                                                                         l.is_default_line_height())
> +                                                             .collect();
> +                    }

You probably need to change `important_count` as well, otherwise you may still get wrong serialization when e.g.
```css
font: menu;
font-size: 10px !important;
```

::: servo/components/style/properties/shorthand/font.mako.rs:6
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  <%namespace name="helpers" file="/helpers.mako.rs" />
> -<% from data import SYSTEM_FONT_LONGHANDS %>
> +<% from data import SYSTEM_FONT_LONGHANDS, to_camel_case %>

It doesn't seem to me this change is necessary here.
Attachment #8867420 - Flags: review?(xidorn+moz)
We fail one mochitest which is `font: inherit; font-family: Helvetica;`; we just serialize these in a different order. Gecko does (all font longhands except family) and then family, we have the family within the longhands. I think this is okay (and we can perhaps make that test more robust).
Comment on attachment 8867420 [details]
Bug 1364286 - Fix serialization of system fonts;

https://reviewboard.mozilla.org/r/138956/#review142866

::: servo/components/style/properties/declaration_block.rs:437
(Diff revision 3)
> -                    }
> +                        }
> +                    }
> +
> +                    if found_system.is_some() {
> +                        current_longhands = current_longhands.into_iter()
> +

Seems to be a pointless empty line.

::: servo/components/style/properties/declaration_block.rs:441
(Diff revision 3)
> +                        important_count = longhands.iter().filter(|longhand| {
> +                           longhand.1.important() && longhand.0.id().is_longhand_of(shorthand)
> +                        }).count();

This still doesn't handle the case of
```css
font: menu;
font-size: 10px !important;
```
does it?

It counts all longhands of font, but `current_longhands` may only contain part of them.

I have a feeling that, code may be clearer and more efficient if you just handle the two cases separately rather than doing a re-filtering, something like:
```rust
if shorthand == ShorthandId::Font && longhands.iter().any(|&&(ref l, _)| l.get_system().is_some()) {
    for &&(ref longhand, longhand_importance) in longhands.iter() {
        if longhand.get_system().is_some() || longhand.is_default_line_height() {
            current_longhands.push(longhand);
            if longhand_importance.important() {
                important_count += 1;
            }
        }
    }
} else {
    for &&(ref longhand, longhand_importance) in longhands.iter() {
        if longhand.id().is_longhand_of(shorthand) {
            current_longhands.push(longhand);
            if longhand_importance.important() {
                important_count += 1;
            }
        }
    }
    // Substep 1: ...
    if current_longhands.len() != properties.len() {
        continue;
    }
}
```
Attachment #8867420 - Flags: review?(xidorn+moz)
Comment on attachment 8867420 [details]
Bug 1364286 - Fix serialization of system fonts;

https://reviewboard.mozilla.org/r/138956/#review148642

::: servo/components/style/properties/declaration_block.rs:508
(Diff revision 4)
>  
> +                    if shorthand == ShorthandId::Font && longhands.iter().any(|&&(ref l, _)| l.get_system().is_some()) {
> +                        for &&(ref longhand, longhand_importance) in longhands.iter() {
> +                            if longhand.get_system().is_some() || longhand.is_default_line_height() {
> +                                current_longhands.push(longhand);
> +                                found_system = longhand.get_system();

If the last eligible longhand is `line-height`, wouldn't this become `None`?

::: servo/components/style/properties/declaration_block.rs:528
(Diff revision 4)
> -                    // Substep 1:
> +                        // Substep 1:
> -                    //
> +                        //
> -                    // Assuming that the PropertyDeclarationBlock contains no
> +                         // Assuming that the PropertyDeclarationBlock contains no
> -                    // duplicate entries, if the current_longhands length is
> +                         // duplicate entries, if the current_longhands length is
> -                    // equal to the properties length, it means that the
> +                         // equal to the properties length, it means that the
> -                    // properties that map to shorthand are present in longhands
> +                         // properties that map to shorthand are present in longhands    

Unnecessary trailing whitespaces.
Attachment #8867420 - Flags: review?(xidorn+moz) → review+
https://hg.mozilla.org/mozilla-central/rev/8c962337cfa6
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: