style: Make word-spacing, letter-spacing, and line-height use Rust lengths.

emilio · aditj · commit f6de533a89b3 · 2019-03-15T14:07:06.000+05:30
This also adopts the resolution from [1] while at it, making letter-spacing compute to a length, serializing 0 to normal rather than keeping normal in the computed value, which matches every other engine. This removes the SMIL tests for percentages from letter-spacing since letter-spacing does in fact not support percentages, so they were passing just by chance. [1]: w3c/csswg-drafts#1484 Differential Revision: https://phabricator.services.mozilla.com/D21850
diff --git a/components/style/cbindgen.toml b/components/style/cbindgen.toml
@@ -78,8 +78,10 @@ include = [
   "Resize",
   "Overflow",
   "LengthPercentage",
+  "LetterSpacing",
   "NonNegativeLengthPercentage",
   "LengthPercentageOrAuto",
+  "LineHeight",
   "NonNegativeLengthPercentageOrAuto",
   "Rect",
   "IntersectionObserverRootMargin",
@@ -102,6 +104,7 @@ item_types = ["enums", "structs", "typedefs"]
 [export.body]
 "CSSPixelLength" = """
   inline nscoord ToAppUnits() const;
+  inline bool IsZero() const;
 """
 
 "LengthPercentage" = """
@@ -118,6 +121,7 @@ item_types = ["enums", "structs", "typedefs"]
   inline bool ConvertsToPercentage() const;
   inline bool HasLengthAndPercentage() const;
   inline float ToPercentage() const;
+  inline bool IsDefinitelyZero() const;
   inline CSSCoord ResolveToCSSPixels(CSSCoord aPercentageBasisInCSSPixels) const;
   template<typename T> inline CSSCoord ResolveToCSSPixelsWith(T aPercentageGetter) const;
   template<typename T, typename U>
diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs
@@ -1227,56 +1227,23 @@ impl Clone for ${style_struct.gecko_struct_name} {
 
     # Types used with predefined_type()-defined properties that we can auto-generate.
     predefined_types = {
-        "Appearance": impl_simple,
-        "OverscrollBehavior": impl_simple,
-        "OverflowClipBox": impl_simple,
-        "ScrollSnapAlign": impl_simple,
-        "ScrollSnapType": impl_simple,
-        "Float": impl_simple,
-        "Overflow": impl_simple,
-        "BreakBetween": impl_simple,
-        "BreakWithin": impl_simple,
-        "Resize": impl_simple,
         "Color": impl_color,
         "ColorOrAuto": impl_color,
-        "GreaterThanOrEqualToOneNumber": impl_simple,
-        "Integer": impl_simple,
         "length::LengthOrAuto": impl_style_coord,
         "length::LengthOrNormal": impl_style_coord,
         "length::NonNegativeLengthOrAuto": impl_style_coord,
         "length::NonNegativeLengthPercentageOrNormal": impl_style_coord,
-        "FillRule": impl_simple,
-        "FlexBasis": impl_simple,
         "Length": impl_absolute_length,
         "LengthOrNormal": impl_style_coord,
-        "LengthPercentage": impl_simple,
         "LengthPercentageOrAuto": impl_style_coord,
-        "MaxSize": impl_simple,
-        "Size": impl_simple,
         "MozScriptMinSize": impl_absolute_length,
-        "MozScriptSizeMultiplier": impl_simple,
-        "NonNegativeLengthPercentage": impl_simple,
-        "NonNegativeLengthOrNumber": impl_simple,
-        "NonNegativeLengthOrNumberRect": impl_simple,
-        "BorderImageSlice": impl_simple,
-        "NonNegativeNumber": impl_simple,
-        "Number": impl_simple,
-        "Opacity": impl_simple,
-        "OverflowWrap": impl_simple,
-        "OverflowAnchor": impl_simple,
-        "Perspective": impl_simple,
-        "Position": impl_simple,
         "RGBAColor": impl_rgba_color,
         "SVGLength": impl_svg_length,
         "SVGOpacity": impl_svg_opacity,
         "SVGPaint": impl_svg_paint,
         "SVGWidth": impl_svg_length,
         "Transform": impl_transform,
-        "TransformOrigin": impl_simple,
-        "UserSelect": impl_simple,
         "url::UrlOrNone": impl_css_url,
-        "WordBreak": impl_simple,
-        "ZIndex": impl_simple,
     }
 
     def longhand_method(longhand):
@@ -1291,15 +1258,12 @@ impl Clone for ${style_struct.gecko_struct_name} {
             args.update(keyword=longhand.keyword)
             if "font" in longhand.ident:
                 args.update(cast_type=longhand.cast_type)
-        else:
+        elif longhand.predefined_type in predefined_types:
             method = predefined_types[longhand.predefined_type]
+        else:
+            method = impl_simple
 
         method(**args)
-
-    picked_longhands = []
-    for x in longhands:
-        if x.keyword or x.predefined_type in predefined_types or x.logical:
-            picked_longhands.append(x)
 %>
 impl ${style_struct.gecko_struct_name} {
     /*
@@ -1311,7 +1275,7 @@ impl ${style_struct.gecko_struct_name} {
      * Auto-Generated Methods.
      */
     <%
-    for longhand in picked_longhands:
+    for longhand in longhands:
         longhand_method(longhand)
     %>
 }
@@ -1992,6 +1956,7 @@ fn static_assert() {
 
 <%
     skip_font_longhands = """font-family font-size font-size-adjust font-weight
+                             font-style font-stretch -moz-script-level
                              font-synthesis -x-lang font-variant-alternates
                              font-variant-east-asian font-variant-ligatures
                              font-variant-numeric font-language-override
@@ -2783,6 +2748,7 @@ fn static_assert() {
                           animation-iteration-count animation-timing-function
                           clear transition-duration transition-delay
                           transition-timing-function transition-property
+                          transform-style
                           rotate scroll-snap-points-x scroll-snap-points-y
                           scroll-snap-coordinate -moz-binding will-change
                           offset-path shape-outside contain touch-action
@@ -4158,7 +4124,7 @@ fn static_assert() {
 
 
 <%self:impl_trait style_struct_name="InheritedText"
-                  skip_longhands="text-align text-emphasis-style text-shadow line-height letter-spacing word-spacing
+                  skip_longhands="text-align text-emphasis-style text-shadow
                                   -webkit-text-stroke-width text-emphasis-position">
 
     <% text_align_keyword = Keyword("text-align",
@@ -4190,78 +4156,6 @@ fn static_assert() {
         longhands::text_shadow::computed_value::List(buf)
     }
 
-    pub fn set_line_height(&mut self, v: longhands::line_height::computed_value::T) {
-        use crate::values::generics::text::LineHeight;
-        // FIXME: Align binary representations and ditch |match| for cast + static_asserts
-        let en = match v {
-            LineHeight::Normal => CoordDataValue::Normal,
-            LineHeight::Length(val) => CoordDataValue::Coord(val.0.to_i32_au()),
-            LineHeight::Number(val) => CoordDataValue::Factor(val.0),
-            LineHeight::MozBlockHeight =>
-                    CoordDataValue::Enumerated(structs::NS_STYLE_LINE_HEIGHT_BLOCK_HEIGHT),
-        };
-        self.gecko.mLineHeight.set_value(en);
-    }
-
-    pub fn clone_line_height(&self) -> longhands::line_height::computed_value::T {
-        use crate::values::generics::text::LineHeight;
-        return match self.gecko.mLineHeight.as_value() {
-            CoordDataValue::Normal => LineHeight::Normal,
-            CoordDataValue::Coord(coord) => LineHeight::Length(Au(coord).into()),
-            CoordDataValue::Factor(n) => LineHeight::Number(n.into()),
-            CoordDataValue::Enumerated(val) if val == structs::NS_STYLE_LINE_HEIGHT_BLOCK_HEIGHT =>
-                LineHeight::MozBlockHeight,
-            _ => panic!("this should not happen"),
-        }
-    }
-
-    <%call expr="impl_coord_copy('line_height', 'mLineHeight')"></%call>
-
-    pub fn set_letter_spacing(&mut self, v: longhands::letter_spacing::computed_value::T) {
-        use crate::values::generics::text::Spacing;
-        match v {
-            Spacing::Value(value) => self.gecko.mLetterSpacing.set(value),
-            Spacing::Normal => self.gecko.mLetterSpacing.set_value(CoordDataValue::Normal)
-        }
-    }
-
-    pub fn clone_letter_spacing(&self) -> longhands::letter_spacing::computed_value::T {
-        use crate::values::computed::Length;
-        use crate::values::generics::text::Spacing;
-        debug_assert!(
-            matches!(self.gecko.mLetterSpacing.as_value(),
-                     CoordDataValue::Normal |
-                     CoordDataValue::Coord(_)),
-            "Unexpected computed value for letter-spacing");
-        Length::from_gecko_style_coord(&self.gecko.mLetterSpacing).map_or(Spacing::Normal, Spacing::Value)
-    }
-
-    <%call expr="impl_coord_copy('letter_spacing', 'mLetterSpacing')"></%call>
-
-    pub fn set_word_spacing(&mut self, v: longhands::word_spacing::computed_value::T) {
-        use crate::values::generics::text::Spacing;
-        match v {
-            Spacing::Value(lp) => self.gecko.mWordSpacing.set(lp),
-            // https://drafts.csswg.org/css-text-3/#valdef-word-spacing-normal
-            Spacing::Normal => self.gecko.mWordSpacing.set_value(CoordDataValue::Coord(0)),
-        }
-    }
-
-    pub fn clone_word_spacing(&self) -> longhands::word_spacing::computed_value::T {
-        use crate::values::computed::LengthPercentage;
-        use crate::values::generics::text::Spacing;
-        debug_assert!(
-            matches!(self.gecko.mWordSpacing.as_value(),
-                     CoordDataValue::Normal |
-                     CoordDataValue::Coord(_) |
-                     CoordDataValue::Percent(_) |
-                     CoordDataValue::Calc(_)),
-            "Unexpected computed value for word-spacing");
-        LengthPercentage::from_gecko_style_coord(&self.gecko.mWordSpacing).map_or(Spacing::Normal, Spacing::Value)
-    }
-
-    <%call expr="impl_coord_copy('word_spacing', 'mWordSpacing')"></%call>
-
     fn clear_text_emphasis_style_if_string(&mut self) {
         if self.gecko.mTextEmphasisStyle == structs::NS_STYLE_TEXT_EMPHASIS_STYLE_STRING as u8 {
             self.gecko.mTextEmphasisStyleString.truncate();
diff --git a/components/style/properties/longhands/inherited_text.mako.rs b/components/style/properties/longhands/inherited_text.mako.rs
@@ -157,7 +157,7 @@ ${helpers.predefined_type(
 ${helpers.predefined_type(
     "word-spacing",
     "WordSpacing",
-    "computed::WordSpacing::normal()",
+    "computed::WordSpacing::zero()",
     animation_value_type="ComputedValue",
     flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER",
     spec="https://drafts.csswg.org/css-text/#propdef-word-spacing",
diff --git a/components/style/values/computed/text.rs b/components/style/values/computed/text.rs
@@ -7,13 +7,14 @@
 #[cfg(feature = "servo")]
 use crate::properties::StyleBuilder;
 use crate::values::computed::length::{Length, LengthPercentage};
-use crate::values::computed::{NonNegativeLength, NonNegativeNumber};
+use crate::values::computed::{Context, NonNegativeLength, NonNegativeNumber, ToComputedValue};
 use crate::values::generics::text::InitialLetter as GenericInitialLetter;
 use crate::values::generics::text::LineHeight as GenericLineHeight;
 use crate::values::generics::text::Spacing;
-use crate::values::specified::text::TextOverflowSide;
+use crate::values::specified::text::{self as specified, TextOverflowSide};
 use crate::values::specified::text::{TextEmphasisFillMode, TextEmphasisShapeKeyword};
 use crate::values::{CSSFloat, CSSInteger};
+use crate::Zero;
 use std::fmt::{self, Write};
 use style_traits::{CssWriter, ToCss};
 
@@ -25,10 +26,78 @@ pub use crate::values::specified::TextEmphasisPosition;
 pub type InitialLetter = GenericInitialLetter<CSSFloat, CSSInteger>;
 
 /// A computed value for the `letter-spacing` property.
-pub type LetterSpacing = Spacing<Length>;
+#[repr(transparent)]
+#[derive(
+    Animate,
+    Clone,
+    ComputeSquaredDistance,
+    Copy,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    ToAnimatedValue,
+    ToAnimatedZero,
+)]
+pub struct LetterSpacing(Length);
+
+impl LetterSpacing {
+    /// Return the `normal` computed value, which is just zero.
+    #[inline]
+    pub fn normal() -> Self {
+        LetterSpacing(Length::zero())
+    }
+}
+
+impl ToCss for LetterSpacing {
+    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
+    where
+        W: Write,
+    {
+        // https://drafts.csswg.org/css-text/#propdef-letter-spacing
+        //
+        // For legacy reasons, a computed letter-spacing of zero yields a
+        // resolved value (getComputedStyle() return value) of normal.
+        if self.0.is_zero() {
+            return dest.write_str("normal");
+        }
+        self.0.to_css(dest)
+    }
+}
+
+impl ToComputedValue for specified::LetterSpacing {
+    type ComputedValue = LetterSpacing;
+    fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
+        match *self {
+            Spacing::Normal => LetterSpacing(Length::zero()),
+            Spacing::Value(ref v) => LetterSpacing(v.to_computed_value(context)),
+        }
+    }
+
+    fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+        if computed.0.is_zero() {
+            return Spacing::Normal;
+        }
+        Spacing::Value(ToComputedValue::from_computed_value(&computed.0))
+    }
+}
 
 /// A computed value for the `word-spacing` property.
-pub type WordSpacing = Spacing<LengthPercentage>;
+pub type WordSpacing = LengthPercentage;
+
+impl ToComputedValue for specified::WordSpacing {
+    type ComputedValue = WordSpacing;
+
+    fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
+        match *self {
+            Spacing::Normal => LengthPercentage::zero(),
+            Spacing::Value(ref v) => v.to_computed_value(context),
+        }
+    }
+
+    fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+        Spacing::Value(ToComputedValue::from_computed_value(computed))
+    }
+}
 
 /// A computed value for the `line-height` property.
 pub type LineHeight = GenericLineHeight<NonNegativeNumber, NonNegativeLength>;
diff --git a/components/style/values/generics/text.rs b/components/style/values/generics/text.rs
