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?

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.

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; } } ```

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.

Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8c962337cfa6 Update test expectations; r=xidorn