Auto merge of #16016 - Manishearth:graft-font-size, r=upsuper

Add separate specified value for keyword font sizes

In Gecko, these keywords compute to different values depending on the
font.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1341775

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16016)
<!-- Reviewable:end -->

components/script/dom/element.rs

@@ -478,7 +478,10 @@ impl LayoutElementHelpers for LayoutJS<Element> {
         if let Some(font_size) = font_size {
             hints.push(from_declaration(
                 shared_lock,
-                PropertyDeclaration::FontSize(font_size::SpecifiedValue(font_size.into()))))
+                PropertyDeclaration::FontSize(
+                    font_size::SpecifiedValue::from_html_size(font_size as u8)
+                )
+            ))
         }
 
         let cellspacing = if let Some(this) = self.downcast::<HTMLTableElement>() {

components/script/dom/htmlfontelement.rs

@@ -18,7 +18,6 @@ use html5ever_atoms::LocalName;
 use servo_atoms::Atom;
 use style::attr::AttrValue;
 use style::str::{HTML_SPACE_CHARACTERS, read_numbers};
-use style::values::specified;
 
 #[dom_struct]
 pub struct HTMLFontElement {
@@ -62,8 +61,7 @@ impl HTMLFontElementMethods for HTMLFontElement {
     // https://html.spec.whatwg.org/multipage/#dom-font-size
     fn SetSize(&self, value: DOMString) {
         let element = self.upcast::<Element>();
-        let length = parse_length(&value);
+        element.set_attribute(&local_name!("size"), parse_size(&value));
-        element.set_attribute(&local_name!("size"), AttrValue::Length(value.into(), length));
     }
 }
 
@@ -76,10 +74,7 @@ impl VirtualMethods for HTMLFontElement {
         match name {
             &local_name!("face") => AttrValue::from_atomic(value.into()),
             &local_name!("color") => AttrValue::from_legacy_color(value.into()),
-            &local_name!("size") => {
+            &local_name!("size") => parse_size(&value),
-                let length = parse_length(&value);
-                AttrValue::Length(value.into(), length)
-            },
             _ => self.super_type().unwrap().parse_plain_attribute(name, value),
         }
     }
@@ -88,7 +83,7 @@ impl VirtualMethods for HTMLFontElement {
 pub trait HTMLFontElementLayoutHelpers {
     fn get_color(&self) -> Option<RGBA>;
     fn get_face(&self) -> Option<Atom>;
-    fn get_size(&self) -> Option<specified::Length>;
+    fn get_size(&self) -> Option<u32>;
 }
 
 impl HTMLFontElementLayoutHelpers for LayoutJS<HTMLFontElement> {
@@ -113,18 +108,21 @@ impl HTMLFontElementLayoutHelpers for LayoutJS<HTMLFontElement> {
     }
 
     #[allow(unsafe_code)]
-    fn get_size(&self) -> Option<specified::Length> {
+    fn get_size(&self) -> Option<u32> {
-        unsafe {
+        let size = unsafe {
             (*self.upcast::<Element>().unsafe_get())
                 .get_attr_for_layout(&ns!(), &local_name!("size"))
-                .and_then(AttrValue::as_length)
+        };
-                .cloned()
+        match size {
+            Some(&AttrValue::UInt(_, s)) => Some(s),
+            _ => None,
         }
     }
 }
 
 /// https://html.spec.whatwg.org/multipage/#rules-for-parsing-a-legacy-font-size
-fn parse_length(mut input: &str) -> Option<specified::Length> {
+fn parse_size(mut input: &str) -> AttrValue {
+    let original_input = input;
     // Steps 1 & 2 are not relevant
 
     // Step 3
@@ -138,7 +136,7 @@ fn parse_length(mut input: &str) -> Option<specified::Length> {
     let mut input_chars = input.chars().peekable();
     let parse_mode = match input_chars.peek() {
         // Step 4
-        None => return None,
+        None => return AttrValue::String(original_input.into()),
 
         // Step 5
         Some(&'+') => {
@@ -155,7 +153,7 @@ fn parse_length(mut input: &str) -> Option<specified::Length> {
     // Steps 6, 7, 8
     let mut value = match read_numbers(input_chars) {
         (Some(v), _) if v >= 0 => v,
-        _ => return None,
+        _ => return AttrValue::String(original_input.into()),
     };
 
     // Step 9
@@ -166,5 +164,5 @@ fn parse_length(mut input: &str) -> Option<specified::Length> {
     }
 
     // Steps 10, 11, 12
-    Some(specified::Length::from_font_size_int(value as u8))
+    AttrValue::UInt(original_input.into(), value as u32)
 }

components/style/properties/longhand/font.mako.rs

@@ -400,24 +400,129 @@ ${helpers.single_keyword("font-variant-caps",
 
     impl ToCss for SpecifiedValue {
         fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-            self.0.to_css(dest)
+            match *self {
+                SpecifiedValue::Length(ref lop) => lop.to_css(dest),
+                SpecifiedValue::Keyword(kw) => kw.to_css(dest),
+                SpecifiedValue::Smaller => dest.write_str("smaller"),
+                SpecifiedValue::Larger => dest.write_str("larger"),
+            }
         }
     }
 
     impl HasViewportPercentage for SpecifiedValue {
         fn has_viewport_percentage(&self) -> bool {
-            return self.0.has_viewport_percentage()
+            match *self {
+                SpecifiedValue::Length(ref lop) => lop.has_viewport_percentage(),
+                _ => false
+            }
         }
     }
 
     #[derive(Debug, Clone, PartialEq)]
     #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
-    pub struct SpecifiedValue(pub specified::LengthOrPercentage);
+    pub enum SpecifiedValue {
+        Length(specified::LengthOrPercentage),
+        Keyword(KeywordSize),
+        Smaller,
+        Larger,
+    }
+
     pub mod computed_value {
         use app_units::Au;
         pub type T = Au;
     }
 
+    /// CSS font keywords
+    #[derive(Debug, Copy, Clone, PartialEq)]
+    #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+    pub enum KeywordSize {
+        XXSmall = 0,
+        XSmall = 1,
+        Small = 2,
+        Medium = 3,
+        Large = 4,
+        XLarge = 5,
+        XXLarge = 6,
+        // This is not a real font keyword and will not parse
+        // HTML font-size 7 corresponds to this value
+        XXXLarge = 7,
+    }
+
+    pub use self::KeywordSize::*;
+
+    impl KeywordSize {
+        pub fn parse(input: &mut Parser) -> Result<Self, ()> {
+            Ok(match_ignore_ascii_case! {&*input.expect_ident()?,
+                "xx-small" => XXSmall,
+                "x-small" => XSmall,
+                "small" => Small,
+                "medium" => Medium,
+                "large" => Large,
+                "x-large" => XLarge,
+                "xx-large" => XXLarge,
+                _ => return Err(())
+            })
+        }
+    }
+
+    impl ToCss for KeywordSize {
+        fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+            dest.write_str(match *self {
+                XXSmall => "xx-small",
+                XSmall => "x-small",
+                Small => "small",
+                Medium => "medium",
+                Large => "large",
+                XLarge => "x-large",
+                XXLarge => "xx-large",
+                XXXLarge => unreachable!("We should never serialize \
+                                          specified values set via
+                                          HTML presentation attributes"),
+            })
+        }
+    }
+
+    impl ToComputedValue for KeywordSize {
+        type ComputedValue = Au;
+        #[inline]
+        fn to_computed_value(&self, _: &Context) -> computed_value::T {
+            // https://drafts.csswg.org/css-fonts-3/#font-size-prop
+            use values::FONT_MEDIUM_PX;
+            match *self {
+                XXSmall => Au::from_px(FONT_MEDIUM_PX) * 3 / 5,
+                XSmall => Au::from_px(FONT_MEDIUM_PX) * 3 / 4,
+                Small => Au::from_px(FONT_MEDIUM_PX) * 8 / 9,
+                Medium => Au::from_px(FONT_MEDIUM_PX),
+                Large => Au::from_px(FONT_MEDIUM_PX) * 6 / 5,
+                XLarge => Au::from_px(FONT_MEDIUM_PX) * 3 / 2,
+                XXLarge => Au::from_px(FONT_MEDIUM_PX) * 2,
+                XXXLarge => Au::from_px(FONT_MEDIUM_PX) * 3,
+            }
+        }
+
+        #[inline]
+        fn from_computed_value(_: &computed_value::T) -> Self {
+            unreachable!()
+        }
+    }
+
+    impl SpecifiedValue {
+        /// https://html.spec.whatwg.org/multipage/#rules-for-parsing-a-legacy-font-size
+        pub fn from_html_size(size: u8) -> Self {
+            SpecifiedValue::Keyword(match size {
+                // If value is less than 1, let it be 1.
+                0 | 1 => XSmall,
+                2 => Small,
+                3 => Medium,
+                4 => Large,
+                5 => XLarge,
+                6 => XXLarge,
+                // If value is greater than 7, let it be 7.
+                _ => XXXLarge,
+            })
+        }
+    }
+
     #[inline]
     #[allow(missing_docs)]
     pub fn get_initial_value() -> computed_value::T {
@@ -426,7 +531,7 @@ ${helpers.single_keyword("font-variant-caps",
 
     #[inline]
     pub fn get_initial_specified_value() -> SpecifiedValue {
-        SpecifiedValue(specified::LengthOrPercentage::Length(NoCalcLength::medium()))
+        SpecifiedValue::Keyword(Medium)
     }
 
     impl ToComputedValue for SpecifiedValue {
@@ -434,45 +539,61 @@ ${helpers.single_keyword("font-variant-caps",
 
         #[inline]
         fn to_computed_value(&self, context: &Context) -> computed_value::T {
-            match self.0 {
+            use values::specified::length::FontRelativeLength;
-                LengthOrPercentage::Length(NoCalcLength::FontRelative(value)) => {
+            match *self {
+                SpecifiedValue::Length(LengthOrPercentage::Length(
+                        NoCalcLength::FontRelative(value))) => {
                     value.to_computed_value(context, /* use inherited */ true)
                 }
-                LengthOrPercentage::Length(NoCalcLength::ServoCharacterWidth(value)) => {
+                SpecifiedValue::Length(LengthOrPercentage::Length(
+                        NoCalcLength::ServoCharacterWidth(value))) => {
                     value.to_computed_value(context.inherited_style().get_font().clone_font_size())
                 }
-                LengthOrPercentage::Length(ref l) => {
+                SpecifiedValue::Length(LengthOrPercentage::Length(ref l)) => {
                     l.to_computed_value(context)
                 }
-                LengthOrPercentage::Percentage(Percentage(value)) => {
+                SpecifiedValue::Length(LengthOrPercentage::Percentage(Percentage(value))) => {
                     context.inherited_style().get_font().clone_font_size().scale_by(value)
                 }
-                LengthOrPercentage::Calc(ref calc) => {
+                SpecifiedValue::Length(LengthOrPercentage::Calc(ref calc)) => {
                     let calc = calc.to_computed_value(context);
                     calc.length() + context.inherited_style().get_font().clone_font_size()
                                            .scale_by(calc.percentage())
                 }
+                SpecifiedValue::Keyword(ref key) => {
+                    key.to_computed_value(context)
+                }
+                SpecifiedValue::Smaller => {
+                    FontRelativeLength::Em(0.85).to_computed_value(context,
+                                                                   /* use_inherited */ true)
+                }
+                SpecifiedValue::Larger => {
+                    FontRelativeLength::Em(1.2).to_computed_value(context,
+                                                                   /* use_inherited */ true)
+                }
             }
         }
 
         #[inline]
         fn from_computed_value(computed: &computed_value::T) -> Self {
-                SpecifiedValue(LengthOrPercentage::Length(
+                SpecifiedValue::Length(LengthOrPercentage::Length(
                         ToComputedValue::from_computed_value(computed)
                 ))
         }
     }
     /// <length> | <percentage> | <absolute-size> | <relative-size>
-    pub fn parse(_context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
+    pub fn parse(_: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
-        use values::specified::{Length, LengthOrPercentage};
+        if let Ok(lop) = input.try(specified::LengthOrPercentage::parse_non_negative) {
-
+            Ok(SpecifiedValue::Length(lop))
-        input.try(specified::LengthOrPercentage::parse_non_negative)
+        } else if let Ok(kw) = input.try(KeywordSize::parse) {
-        .or_else(|()| {
+            Ok(SpecifiedValue::Keyword(kw))
-            let ident = try!(input.expect_ident());
+        } else {
-            NoCalcLength::from_str(&ident as &str)
+            match_ignore_ascii_case! {&*input.expect_ident()?,
-                         .ok_or(())
+                "smaller" => Ok(SpecifiedValue::Smaller),
-                         .map(specified::LengthOrPercentage::Length)
+                "larger" => Ok(SpecifiedValue::Larger),
-        }).map(SpecifiedValue)
+                _ => Err(())
+            }
+        }
     }
 </%helpers:longhand>
 

components/style/values/specified/length.rs

@@ -276,38 +276,6 @@ impl Mul<CSSFloat> for NoCalcLength {
 }
 
 impl NoCalcLength {
-    /// https://drafts.csswg.org/css-fonts-3/#font-size-prop
-    pub fn from_str(s: &str) -> Option<NoCalcLength> {
-        Some(match_ignore_ascii_case! { s,
-            "xx-small" => NoCalcLength::Absolute(Au::from_px(FONT_MEDIUM_PX) * 3 / 5),
-            "x-small" => NoCalcLength::Absolute(Au::from_px(FONT_MEDIUM_PX) * 3 / 4),
-            "small" => NoCalcLength::Absolute(Au::from_px(FONT_MEDIUM_PX) * 8 / 9),
-            "medium" => NoCalcLength::Absolute(Au::from_px(FONT_MEDIUM_PX)),
-            "large" => NoCalcLength::Absolute(Au::from_px(FONT_MEDIUM_PX) * 6 / 5),
-            "x-large" => NoCalcLength::Absolute(Au::from_px(FONT_MEDIUM_PX) * 3 / 2),
-            "xx-large" => NoCalcLength::Absolute(Au::from_px(FONT_MEDIUM_PX) * 2),
-
-            // https://github.com/servo/servo/issues/3423#issuecomment-56321664
-            "smaller" => NoCalcLength::FontRelative(FontRelativeLength::Em(0.85)),
-            "larger" => NoCalcLength::FontRelative(FontRelativeLength::Em(1.2)),
-            _ => return None
-        })
-    }
-
-    /// https://drafts.csswg.org/css-fonts-3/#font-size-prop
-    pub fn from_font_size_int(i: u8) -> Self {
-        let au = match i {
-            0 | 1 => Au::from_px(FONT_MEDIUM_PX) * 3 / 4,
-            2 => Au::from_px(FONT_MEDIUM_PX) * 8 / 9,
-            3 => Au::from_px(FONT_MEDIUM_PX),
-            4 => Au::from_px(FONT_MEDIUM_PX) * 6 / 5,
-            5 => Au::from_px(FONT_MEDIUM_PX) * 3 / 2,
-            6 => Au::from_px(FONT_MEDIUM_PX) * 2,
-            _ => Au::from_px(FONT_MEDIUM_PX) * 3,
-        };
-        NoCalcLength::Absolute(au)
-    }
-
     /// Parse a given absolute or relative dimension.
     pub fn parse_dimension(value: CSSFloat, unit: &str) -> Result<NoCalcLength, ()> {
         match_ignore_ascii_case! { unit,
@@ -444,21 +412,11 @@ impl Length {
         Length::NoCalc(NoCalcLength::zero())
     }
 
-    /// https://drafts.csswg.org/css-fonts-3/#font-size-prop
-    pub fn from_str(s: &str) -> Option<Length> {
-        NoCalcLength::from_str(s).map(Length::NoCalc)
-    }
-
     /// Parse a given absolute or relative dimension.
     pub fn parse_dimension(value: CSSFloat, unit: &str) -> Result<Length, ()> {
         NoCalcLength::parse_dimension(value, unit).map(Length::NoCalc)
     }
 
-    /// https://drafts.csswg.org/css-fonts-3/#font-size-prop
-    pub fn from_font_size_int(i: u8) -> Self {
-        Length::NoCalc(NoCalcLength::from_font_size_int(i))
-    }
-
     #[inline]
     fn parse_internal(input: &mut Parser, context: AllowedNumericType) -> Result<Length, ()> {
         match try!(input.next()) {

ports/geckolib/glue.rs

@@ -1111,7 +1111,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetKeywordValue(declarations:
                                                          value: i32) {
     use style::properties::{PropertyDeclaration, LonghandId};
     use style::properties::longhands;
-    use style::values::specified::{BorderStyle, NoCalcLength};
+    use style::values::specified::BorderStyle;
 
     let declarations = Locked::<PropertyDeclarationBlock>::as_arc(&declarations);
     let long = get_longhand_from_id!(property);
@@ -1127,7 +1127,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetKeywordValue(declarations:
         Clear => longhands::clear::SpecifiedValue::from_gecko_keyword(value),
         FontSize => {
             // We rely on Gecko passing in font-size values (0...7) here.
-            longhands::font_size::SpecifiedValue(NoCalcLength::from_font_size_int(value as u8).into())
+            longhands::font_size::SpecifiedValue::from_html_size(value as u8)
         },
         ListStyleType => longhands::list_style_type::SpecifiedValue::from_gecko_keyword(value),
         WhiteSpace => longhands::white_space::SpecifiedValue::from_gecko_keyword(value),

tests/wpt/metadata/cssom/serialize-values.html.ini

@@ -27,33 +27,6 @@
   [font-family: Arial]
     expected: FAIL
 
-  [font-size: xx-small]
-    expected: FAIL
-
-  [font-size: x-small]
-    expected: FAIL
-
-  [font-size: small]
-    expected: FAIL
-
-  [font-size: medium]
-    expected: FAIL
-
-  [font-size: large]
-    expected: FAIL
-
-  [font-size: x-large]
-    expected: FAIL
-
-  [font-size: xx-large]
-    expected: FAIL
-
-  [font-size: larger]
-    expected: FAIL
-
-  [font-size: smaller]
-    expected: FAIL
-
   [list-style-type: decimal-leading-zero]
     expected: FAIL