From 2cc6593fa1f1ee3cd15314023d0922fdc76a579f Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Wed, 1 Mar 2017 11:20:40 +1100 Subject: [PATCH] Don't serialize shorthand when some but not all of its longhands are CSS-wide keyword Shorthand cannot represent CSS-wide keyword in individual longhands, and thus we cannot serialize longhand with CSS-wide keyword as part of a shorthand. --- .../style/properties/properties.mako.rs | 63 ++++++-- tests/unit/style/properties/serialization.rs | 142 +++--------------- 2 files changed, 76 insertions(+), 129 deletions(-) diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index d841b7e555a..990c83e1713 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -390,13 +390,19 @@ pub enum CSSWideKeyword { Unset, } -impl ToCss for CSSWideKeyword { - fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { - dest.write_str(match *self { +impl CSSWideKeyword { + fn to_str(&self) -> &'static str { + match *self { CSSWideKeyword::Initial => "initial", CSSWideKeyword::Inherit => "inherit", CSSWideKeyword::Unset => "unset", - }) + } + } +} + +impl ToCss for CSSWideKeyword { + fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { + dest.write_str(self.to_str()) } } @@ -545,7 +551,16 @@ impl ShorthandId { return None; } - if !declarations3.any(|d| d.with_variables()) { + // Check whether they are all the same CSS-wide keyword. + if let Some(keyword) = first_declaration.get_css_wide_keyword() { + if declarations2.all(|d| d.get_css_wide_keyword() == Some(keyword)) { + return Some(AppendableValue::Css(keyword.to_str())); + } + return None; + } + + // Check whether all declarations can be serialized as part of shorthand. + if declarations3.all(|d| d.may_serialize_as_part_of_shorthand()) { return Some(AppendableValue::DeclarationsForShorthand(self, declarations)); } @@ -927,23 +942,47 @@ impl PropertyDeclaration { } } - /// Return whether this is a pending-substitution value. - /// https://drafts.csswg.org/css-variables/#variables-in-shorthands - pub fn with_variables(&self) -> bool { + /// Returns a CSS-wide keyword if the declaration's value is one. + pub fn get_css_wide_keyword(&self) -> Option { match *self { % for property in data.longhands: PropertyDeclaration::${property.camel_case}(ref value) => match *value { - DeclaredValue::WithVariables(_) => true, - _ => false, + DeclaredValue::CSSWideKeyword(keyword) => Some(keyword), + _ => None, }, % endfor PropertyDeclaration::Custom(_, ref value) => match *value { - DeclaredValue::WithVariables(_) => true, - _ => false, + DeclaredValue::CSSWideKeyword(keyword) => Some(keyword), + _ => None, } } } + /// Returns whether the declaration may be serialized as part of a shorthand. + /// + /// This method returns false if this declaration contains variable or has a + /// CSS-wide keyword value, since these values cannot be serialized as part + /// of a shorthand. + /// + /// Caller should check `with_variables_from_shorthand()` and whether all + /// needed declarations has the same CSS-wide keyword first. + /// + /// Note that, serialization of a shorthand may still fail because of other + /// property-specific requirement even when this method returns true for all + /// the longhand declarations. + pub fn may_serialize_as_part_of_shorthand(&self) -> bool { + match *self { + % for property in data.longhands: + PropertyDeclaration::${property.camel_case}(ref value) => match *value { + DeclaredValue::Value(_) => true, + _ => false, + }, + % endfor + PropertyDeclaration::Custom(..) => + unreachable!("Serialize a custom property as part of shorthand?"), + } + } + /// Return whether the value is stored as it was in the CSS source, /// preserving whitespace (as opposed to being parsed into a more abstract /// data structure). diff --git a/tests/unit/style/properties/serialization.rs b/tests/unit/style/properties/serialization.rs index 262a1a5ea29..9e99169c54d 100644 --- a/tests/unit/style/properties/serialization.rs +++ b/tests/unit/style/properties/serialization.rs @@ -7,7 +7,7 @@ use media_queries::CSSErrorReporterTest; use servo_url::ServoUrl; use style::computed_values::display::T::inline_block; use style::parser::ParserContext; -use style::properties::{CSSWideKeyword, DeclaredValue, PropertyDeclaration}; +use style::properties::{DeclaredValue, PropertyDeclaration}; use style::properties::{PropertyDeclarationBlock, Importance, PropertyId}; use style::properties::longhands::outline_color::computed_value::T as ComputedColor; use style::properties::parse_property_declaration_list; @@ -372,33 +372,18 @@ mod shorthand_serialization { assert_eq!(serialization, "border-top: 4px solid rgb(255, 0, 0);"); } - #[test] - fn directional_border_with_no_specified_style_will_show_style_as_none() { - let mut properties = Vec::new(); - - let width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let style = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - let color = DeclaredValue::Value(CSSColor { - parsed: ComputedColor::RGBA(RGBA::new(255, 0, 0, 255)), - authored: None - }); - - properties.push(PropertyDeclaration::BorderTopWidth(width)); - properties.push(PropertyDeclaration::BorderTopStyle(style)); - properties.push(PropertyDeclaration::BorderTopColor(color)); - - let serialization = shorthand_properties_to_string(properties); - assert_eq!(serialization, "border-top: 4px none rgb(255, 0, 0);"); + fn get_border_property_values() -> (DeclaredValue, + DeclaredValue, + DeclaredValue) { + (DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))), + DeclaredValue::Value(BorderStyle::solid), + DeclaredValue::Value(CSSColor::currentcolor())) } #[test] - fn directional_border_with_no_specified_color_will_not_show_color() { + fn border_top_should_serialize_correctly() { let mut properties = Vec::new(); - - let width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let style = DeclaredValue::Value(BorderStyle::solid); - let color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - + let (width, style, color) = get_border_property_values(); properties.push(PropertyDeclaration::BorderTopWidth(width)); properties.push(PropertyDeclaration::BorderTopStyle(style)); properties.push(PropertyDeclaration::BorderTopColor(color)); @@ -410,11 +395,7 @@ mod shorthand_serialization { #[test] fn border_right_should_serialize_correctly() { let mut properties = Vec::new(); - - let width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let style = DeclaredValue::Value(BorderStyle::solid); - let color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - + let (width, style, color) = get_border_property_values(); properties.push(PropertyDeclaration::BorderRightWidth(width)); properties.push(PropertyDeclaration::BorderRightStyle(style)); properties.push(PropertyDeclaration::BorderRightColor(color)); @@ -426,11 +407,7 @@ mod shorthand_serialization { #[test] fn border_bottom_should_serialize_correctly() { let mut properties = Vec::new(); - - let width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let style = DeclaredValue::Value(BorderStyle::solid); - let color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - + let (width, style, color) = get_border_property_values(); properties.push(PropertyDeclaration::BorderBottomWidth(width)); properties.push(PropertyDeclaration::BorderBottomStyle(style)); properties.push(PropertyDeclaration::BorderBottomColor(color)); @@ -442,11 +419,7 @@ mod shorthand_serialization { #[test] fn border_left_should_serialize_correctly() { let mut properties = Vec::new(); - - let width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let style = DeclaredValue::Value(BorderStyle::solid); - let color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - + let (width, style, color) = get_border_property_values(); properties.push(PropertyDeclaration::BorderLeftWidth(width)); properties.push(PropertyDeclaration::BorderLeftStyle(style)); properties.push(PropertyDeclaration::BorderLeftColor(color)); @@ -458,38 +431,23 @@ mod shorthand_serialization { #[test] fn border_should_serialize_correctly() { let mut properties = Vec::new(); + let (width, style, color) = get_border_property_values(); - let top_width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let top_style = DeclaredValue::Value(BorderStyle::solid); - let top_color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); + properties.push(PropertyDeclaration::BorderTopWidth(width.clone())); + properties.push(PropertyDeclaration::BorderTopStyle(style.clone())); + properties.push(PropertyDeclaration::BorderTopColor(color.clone())); - properties.push(PropertyDeclaration::BorderTopWidth(top_width)); - properties.push(PropertyDeclaration::BorderTopStyle(top_style)); - properties.push(PropertyDeclaration::BorderTopColor(top_color)); + properties.push(PropertyDeclaration::BorderRightWidth(width.clone())); + properties.push(PropertyDeclaration::BorderRightStyle(style.clone())); + properties.push(PropertyDeclaration::BorderRightColor(color.clone())); - let right_width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let right_style = DeclaredValue::Value(BorderStyle::solid); - let right_color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); + properties.push(PropertyDeclaration::BorderBottomWidth(width.clone())); + properties.push(PropertyDeclaration::BorderBottomStyle(style.clone())); + properties.push(PropertyDeclaration::BorderBottomColor(color.clone())); - properties.push(PropertyDeclaration::BorderRightWidth(right_width)); - properties.push(PropertyDeclaration::BorderRightStyle(right_style)); - properties.push(PropertyDeclaration::BorderRightColor(right_color)); - - let bottom_width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let bottom_style = DeclaredValue::Value(BorderStyle::solid); - let bottom_color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - - properties.push(PropertyDeclaration::BorderBottomWidth(bottom_width)); - properties.push(PropertyDeclaration::BorderBottomStyle(bottom_style)); - properties.push(PropertyDeclaration::BorderBottomColor(bottom_color)); - - let left_width = DeclaredValue::Value(BorderWidth::from_length(Length::from_px(4f32))); - let left_style = DeclaredValue::Value(BorderStyle::solid); - let left_color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - - properties.push(PropertyDeclaration::BorderLeftWidth(left_width)); - properties.push(PropertyDeclaration::BorderLeftStyle(left_style)); - properties.push(PropertyDeclaration::BorderLeftColor(left_color)); + properties.push(PropertyDeclaration::BorderLeftWidth(width.clone())); + properties.push(PropertyDeclaration::BorderLeftStyle(style.clone())); + properties.push(PropertyDeclaration::BorderLeftColor(color.clone())); let serialization = shorthand_properties_to_string(properties); assert_eq!(serialization, "border: 4px solid;"); @@ -518,22 +476,6 @@ mod shorthand_serialization { let serialization = shorthand_properties_to_string(properties); assert_eq!(serialization, "list-style: inside url(\"http://servo/test.png\") disc;"); } - - #[test] - fn list_style_should_show_all_properties_even_if_only_one_is_set() { - let mut properties = Vec::new(); - - let position = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - let image = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - let style_type = DeclaredValue::Value(ListStyleType::disc); - - properties.push(PropertyDeclaration::ListStylePosition(position)); - properties.push(PropertyDeclaration::ListStyleImage(image)); - properties.push(PropertyDeclaration::ListStyleType(style_type)); - - let serialization = shorthand_properties_to_string(properties); - assert_eq!(serialization, "list-style: outside none disc;"); - } } mod outline { @@ -560,40 +502,6 @@ mod shorthand_serialization { assert_eq!(serialization, "outline: 4px solid rgb(255, 0, 0);"); } - #[test] - fn outline_should_not_show_color_if_not_set() { - let mut properties = Vec::new(); - - let width = DeclaredValue::Value(WidthContainer(Length::from_px(4f32))); - let style = DeclaredValue::Value(Either::Second(BorderStyle::solid)); - let color = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - - properties.push(PropertyDeclaration::OutlineWidth(width)); - properties.push(PropertyDeclaration::OutlineStyle(style)); - properties.push(PropertyDeclaration::OutlineColor(color)); - - let serialization = shorthand_properties_to_string(properties); - assert_eq!(serialization, "outline: 4px solid;"); - } - - #[test] - fn outline_should_serialize_correctly_when_style_is_not_set() { - let mut properties = Vec::new(); - - let width = DeclaredValue::Value(WidthContainer(Length::from_px(4f32))); - let style = DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial); - let color = DeclaredValue::Value(CSSColor { - parsed: ComputedColor::RGBA(RGBA::new(255, 0, 0, 255)), - authored: None - }); - properties.push(PropertyDeclaration::OutlineWidth(width)); - properties.push(PropertyDeclaration::OutlineStyle(style)); - properties.push(PropertyDeclaration::OutlineColor(color)); - - let serialization = shorthand_properties_to_string(properties); - assert_eq!(serialization, "outline: 4px none rgb(255, 0, 0);"); - } - #[test] fn outline_should_serialize_correctly_when_style_is_auto() { let mut properties = Vec::new();