Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties. r?emilio draft
authorXidorn Quan <me@upsuper.org>
Wed, 25 Apr 2018 14:33:24 +1000
changeset 788767 d2fc92ed32f497ff6826228181b19787c1b48cc4
parent 788766 6be27f2f044e727f1a153894713ab7c2194869f0
child 788768 e9e5d72857746710ead3cd42481b805efc771389
push id108088
push userxquan@mozilla.com
push dateFri, 27 Apr 2018 00:40:56 +0000
reviewersemilio
bugs1434130
milestone61.0a1
Bug 1434130 part 1 - Skip system font variant for ToCss in font subproperties. r?emilio System font keywords are not a valid value for those properties. The newly-added #[css(skip)] would be reused by deriving algorithm of SpecifiedValueInfo to skip them as well. MozReview-Commit-ID: EmnhkaA9RR5
layout/style/test/test_value_storage.html
servo/components/style/properties/helpers.mako.rs
servo/components/style/properties/longhand/font.mako.rs
servo/components/style/values/specified/font.rs
servo/components/style_derive/to_css.rs
--- a/layout/style/test/test_value_storage.html
+++ b/layout/style/test/test_value_storage.html
@@ -149,16 +149,17 @@ function test_property(property)
       }
       is(gDeclaration.getPropertyValue(sh), "",
          "setting '" + value + "' on '" + property + "' (for shorthand '" + sh + "')");
     }
   }
 
   function test_value(value, resolved_value) {
     var value_has_variable_reference = resolved_value != null;
+    var is_system_font = property == "font" && value in gSystemFont;
 
     var colon = value == "var(--a)" ? ":" : ": ";
     gDeclaration.setProperty(property, value, "");
 
     var idx;
 
     var step1val = gDeclaration.getPropertyValue(property);
     var step1vals = [];
@@ -170,17 +171,19 @@ function test_property(property)
     var step1comps = [];
     if (info.type != CSS_TYPE_TRUE_SHORTHAND)
       step1comp = gComputedStyle.getPropertyValue(property);
     if ("subproperties" in info)
       for (idx in info.subproperties)
         step1comps.push(gComputedStyle.getPropertyValue(info.subproperties[idx]));
 
     SimpleTest.isnot(step1val, "", "setting '" + value + "' on '" + property + "'");
-    if ("subproperties" in info)
+    if ("subproperties" in info &&
+        // System font doesn't produce meaningful value for subproperties.
+        !is_system_font)
       for (idx in info.subproperties) {
         var subprop = info.subproperties[idx];
         if (value_has_variable_reference &&
             (!info.alias_for || info.type == CSS_TYPE_TRUE_SHORTHAND)) {
           is(gDeclaration.getPropertyValue(subprop), "",
              "setting '" + value + "' on '" + property + "' (for '" + subprop + "')");
           test_other_shorthands_empty(value, subprop);
         } else {
@@ -212,17 +215,17 @@ function test_property(property)
       is(gComputedStyle.getPropertyValue(property), step1comp,
          "serialize+parse should be identity transform for '" +
          property + ": " + value + "'");
     }
 
     if ("subproperties" in info &&
         // Using setProperty over subproperties is not sufficient for
         // system fonts, since the shorthand does more than its parts.
-        (property != "font" || !(value in gSystemFont)) &&
+        !is_system_font &&
         // Likewise for special compatibility values of transform
         (property != "-moz-transform" || !value.match(/^matrix.*(px|em|%)/)) &&
         !value_has_variable_reference) {
       gDeclaration.removeProperty(property);
       for (idx in info.subproperties) {
         var subprop = info.subproperties[idx];
         gDeclaration.setProperty(subprop, step1vals[idx], "");
       }
@@ -245,17 +248,17 @@ function test_property(property)
           property != "mask") {
       gDeclaration.removeProperty(property);
       gDeclaration.setProperty(property, step1comp, "");
       var func = xfail_compute(property, value) ? todo_is : is;
       func(gComputedStyle.getPropertyValue(property), step1comp,
            "parse+compute+serialize should be idempotent for '" +
            property + ": " + value + "'");
     }
-    if ("subproperties" in info) {
+    if ("subproperties" in info && !is_system_font) {
       gDeclaration.removeProperty(property);
       for (idx in info.subproperties) {
         var subprop = info.subproperties[idx];
         gDeclaration.setProperty(subprop, step1comps[idx], "");
       }
 
       // Now that all the subprops are set, check their values.  Note that we
       // need this in a separate loop, in case parts of the shorthand affect
--- a/servo/components/style/properties/helpers.mako.rs
+++ b/servo/components/style/properties/helpers.mako.rs
@@ -406,16 +406,17 @@
 
             ${gecko_keyword_conversion(keyword, keyword.values_for(product), type="T", cast_to="i32")}
         }
 
         #[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
         #[derive(Clone, Copy, Debug, Eq, PartialEq, SpecifiedValueInfo, ToCss)]
         pub enum SpecifiedValue {
             Keyword(computed_value::T),
+            #[css(skip)]
             System(SystemFont),
         }
 
         pub fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result<SpecifiedValue, ParseError<'i>> {
             Ok(SpecifiedValue::Keyword(computed_value::T::parse(input)?))
         }
 
         impl ToComputedValue for SpecifiedValue {
--- a/servo/components/style/properties/longhand/font.mako.rs
+++ b/servo/components/style/properties/longhand/font.mako.rs
@@ -269,16 +269,21 @@ https://drafts.csswg.org/css-fonts-4/#lo
         //! While Gecko handles these as a separate property and keyword
         //! values on each property indicating that the font should be picked
         //! from the -x-system-font property, we avoid this. Instead,
         //! each font longhand has a special SystemFont variant which contains
         //! the specified system font. When the cascade function (in helpers)
         //! detects that a value has a system font, it will resolve it, and
         //! cache it on the ComputedValues. After this, it can be just fetched
         //! whenever a font longhand on the same element needs the system font.
+        //!
+        //! When a longhand property is holding a SystemFont, it's serialized
+        //! to an empty string as if its value comes from a shorthand with
+        //! variable reference. We may want to improve this behavior at some
+        //! point. See also https://github.com/w3c/csswg-drafts/issues/1586.
 
         use app_units::Au;
         use cssparser::{Parser, ToCss};
         use gecko_bindings::structs::FontFamilyType;
         use properties::longhands;
         use std::fmt;
         use std::hash::{Hash, Hasher};
         use style_traits::ParseError;
--- a/servo/components/style/values/specified/font.rs
+++ b/servo/components/style/values/specified/font.rs
@@ -81,16 +81,17 @@ pub const MAX_FONT_WEIGHT: f32 = 1000.;
 pub enum FontWeight {
     /// `<font-weight-absolute>`
     Absolute(AbsoluteFontWeight),
     /// Bolder variant
     Bolder,
     /// Lighter variant
     Lighter,
     /// System font variant.
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontWeight {
     system_font_methods!(FontWeight, font_weight);
 
     /// `normal`
     #[inline]
@@ -333,16 +334,17 @@ impl SpecifiedFontStyle {
 }
 
 /// The specified value of the `font-style` property.
 #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo,
          ToCss)]
 #[allow(missing_docs)]
 pub enum FontStyle {
     Specified(SpecifiedFontStyle),
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontStyle {
     /// Return the `normal` value.
     #[inline]
     pub fn normal() -> Self {
         FontStyle::Specified(generics::FontStyle::Normal)
@@ -379,16 +381,17 @@ impl Parse for FontStyle {
 ///
 /// https://drafts.csswg.org/css-fonts-4/#font-stretch-prop
 #[allow(missing_docs)]
 #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo,
          ToCss)]
 pub enum FontStretch {
     Stretch(Percentage),
     Keyword(FontStretchKeyword),
+    #[css(skip)]
     System(SystemFont),
 }
 
 /// A keyword value for `font-stretch`.
 #[derive(Clone, Copy, Debug, MallocSizeOf, Parse, PartialEq, SpecifiedValueInfo,
          ToCss)]
 #[allow(missing_docs)]
 pub enum FontStretchKeyword {
@@ -520,32 +523,34 @@ pub enum FontSize {
     /// will go into the offset.
     /// See bug 1355707.
     Keyword(KeywordInfo),
     /// font-size: smaller
     Smaller,
     /// font-size: larger
     Larger,
     /// Derived from a specified system font.
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl From<LengthOrPercentage> for FontSize {
     fn from(other: LengthOrPercentage) -> Self {
         FontSize::Length(other)
     }
 }
 
 /// Specifies a prioritized list of font family names or generic family names.
 #[derive(Clone, Debug, Eq, Hash, PartialEq, ToCss)]
 pub enum FontFamily {
     /// List of `font-family`
     #[css(comma)]
     Values(#[css(iterable)] FontFamilyList),
     /// System font
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontFamily {
     system_font_methods!(FontFamily, font_family);
 
     /// Parse a specified font-family value
     pub fn parse_specified<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
@@ -627,16 +632,17 @@ impl Parse for FamilyName {
 #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToCss)]
 /// Preserve the readability of text when font fallback occurs
 pub enum FontSizeAdjust {
     /// None variant
     None,
     /// Number variant
     Number(Number),
     /// system font
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontSizeAdjust {
     #[inline]
     /// Default value of font-size-adjust
     pub fn none() -> Self {
         FontSizeAdjust::None
@@ -1131,16 +1137,17 @@ impl VariantAlternatesList {
 }
 
 #[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToCss)]
 /// Control over the selection of these alternate glyphs
 pub enum FontVariantAlternates {
     /// Use alternative glyph from value
     Value(VariantAlternatesList),
     /// Use system font glyph
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontVariantAlternates {
     #[inline]
     /// Get initial specified value with VariantAlternatesList
     pub fn get_initial_specified_value() -> Self {
         FontVariantAlternates::Value(VariantAlternatesList(vec![].into_boxed_slice()))
@@ -1384,16 +1391,17 @@ pub fn assert_variant_east_asian_matches
 
 #[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
 #[derive(Clone, Debug, PartialEq, SpecifiedValueInfo, ToCss)]
 /// Allows control of glyph substitution and sizing in East Asian text.
 pub enum FontVariantEastAsian {
     /// Value variant with `variant-east-asian`
     Value(VariantEastAsian),
     /// System font variant
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontVariantEastAsian {
     #[inline]
     /// Get default `font-variant-east-asian` with `empty` variant
     pub fn empty() -> Self {
         FontVariantEastAsian::Value(VariantEastAsian::empty())
@@ -1612,16 +1620,17 @@ pub fn assert_variant_ligatures_matches(
 #[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
 #[derive(Clone, Debug, PartialEq, SpecifiedValueInfo, ToCss)]
 /// Ligatures and contextual forms are ways of combining glyphs
 /// to produce more harmonized forms
 pub enum FontVariantLigatures {
     /// Value variant with `variant-ligatures`
     Value(VariantLigatures),
     /// System font variant
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontVariantLigatures {
     system_font_methods!(FontVariantLigatures, font_variant_ligatures);
 
     /// Default value of `font-variant-ligatures` as `empty`
     #[inline]
@@ -1842,16 +1851,17 @@ pub fn assert_variant_numeric_matches() 
 
 #[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
 #[derive(Clone, Debug, PartialEq, SpecifiedValueInfo, ToCss)]
 /// Specifies control over numerical forms.
 pub enum FontVariantNumeric {
     /// Value variant with `variant-numeric`
     Value(VariantNumeric),
     /// System font
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontVariantNumeric {
     #[inline]
     /// Default value of `font-variant-numeric` as `empty`
     pub fn empty() -> FontVariantNumeric {
         FontVariantNumeric::Value(VariantNumeric::empty())
@@ -1949,16 +1959,17 @@ pub type SpecifiedFontFeatureSettings = 
 
 /// Define initial settings that apply when the font defined by an @font-face
 /// rule is rendered.
 #[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToCss)]
 pub enum FontFeatureSettings {
     /// Value of `FontSettings`
     Value(SpecifiedFontFeatureSettings),
     /// System font
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontFeatureSettings {
     #[inline]
     /// Get default value of `font-feature-settings` as normal
     pub fn normal() -> FontFeatureSettings {
         FontFeatureSettings::Value(FontSettings::normal())
@@ -2099,16 +2110,17 @@ pub enum FontLanguageOverride {
     /// the content language of the element is
     /// used to infer the OpenType language system
     Normal,
     /// Single three-letter case-sensitive OpenType language system tag,
     /// specifies the OpenType language system to be used instead of
     /// the language system implied by the language of the element
     Override(Box<str>),
     /// Use system font
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontLanguageOverride {
     #[inline]
     /// Get default value with `normal`
     pub fn normal() -> FontLanguageOverride {
         FontLanguageOverride::Normal
@@ -2181,16 +2193,17 @@ pub type SpecifiedFontVariationSettings 
 
 /// Define initial settings that apply when the font defined by an @font-face
 /// rule is rendered.
 #[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToCss)]
 pub enum FontVariationSettings {
     /// Value of `FontSettings`
     Value(SpecifiedFontVariationSettings),
     /// System font
+    #[css(skip)]
     System(SystemFont),
 }
 
 impl FontVariationSettings {
     #[inline]
     /// Get default value of `font-variation-settings` as normal
     pub fn normal() -> FontVariationSettings {
         FontVariationSettings::Value(FontSettings::normal())
--- a/servo/components/style_derive/to_css.rs
+++ b/servo/components/style_derive/to_css.rs
@@ -73,16 +73,19 @@ fn derive_variant_arm(
     generics: &mut Option<WhereClause>,
 ) -> Tokens {
     let bindings = variant.bindings();
     let identifier = cg::to_css_identifier(variant.ast().ident.as_ref());
     let ast = variant.ast();
     let variant_attrs = cg::parse_variant_attrs_from_ast::<CssVariantAttrs>(&ast);
     let separator = if variant_attrs.comma { ", " } else { " " };
 
+    if variant_attrs.skip {
+        return quote!(Ok(()));
+    }
     if variant_attrs.dimension {
         assert_eq!(bindings.len(), 1);
         assert!(
             variant_attrs.function.is_none() && variant_attrs.keyword.is_none(),
             "That makes no sense"
         );
     }
 
@@ -218,16 +221,17 @@ pub struct CssInputAttrs {
 #[darling(attributes(css), default)]
 #[derive(Default, FromVariant)]
 pub struct CssVariantAttrs {
     pub function: Option<Override<String>>,
     pub comma: bool,
     pub dimension: bool,
     pub keyword: Option<String>,
     pub aliases: Option<String>,
+    pub skip: bool,
 }
 
 #[darling(attributes(css), default)]
 #[derive(Default, FromField)]
 pub struct CssFieldAttrs {
     pub if_empty: Option<String>,
     pub field_bound: bool,
     pub iterable: bool,