From 05b5aabc698a4d852e2736109812e448b7922bbc Mon Sep 17 00:00:00 2001 From: Matthias Devlamynck Date: Wed, 8 Feb 2017 20:17:44 +0100 Subject: [PATCH 1/3] Shorthand with variable reference should not have extra whitespace after colon for serialization --- .../style/properties/declaration_block.rs | 6 ++-- tests/wpt/metadata/MANIFEST.json | 10 ++++++ .../cssom/serialize-variable-reference.html | 34 +++++++++++++++++++ 3 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 9d76adf1383..e7adda198a8 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -561,9 +561,6 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W, // for normal parsed values, add a space between key: and value match &appendable_value { - &AppendableValue::Css(_) => { - try!(write!(dest, " ")) - }, &AppendableValue::Declaration(decl) => { if !decl.value_is_unparsed() { // for normal parsed values, add a space between key: and value @@ -572,7 +569,8 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W, }, // Currently append_serialization is only called with a Css or // a Declaration AppendableValue. - &AppendableValue::DeclarationsForShorthand(..) => unreachable!() + &AppendableValue::DeclarationsForShorthand(..) => unreachable!(), + &AppendableValue::Css(_) => {} } try!(append_declaration_value(dest, appendable_value)); diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json index 139d679d65b..672ef7efbbb 100644 --- a/tests/wpt/metadata/MANIFEST.json +++ b/tests/wpt/metadata/MANIFEST.json @@ -83640,6 +83640,12 @@ {} ] ], + "cssom/serialize-variable-reference.html": [ + [ + "/cssom/serialize-variable-reference.html", + {} + ] + ], "cssom/shorthand-serialization.html": [ [ "/cssom/shorthand-serialization.html", @@ -159941,6 +159947,10 @@ "329fe02cb9e54b1a24a8f9dedcfcf5c0f61c7f24", "testharness" ], + "cssom/serialize-variable-reference.html": [ + "5e83f084efc82184c3052a40bb4a061fd4a1336f", + "testharness" + ], "cssom/shorthand-serialization.html": [ "bd514834dbd48c267c16a4329af6fec7f6cbc081", "testharness" diff --git a/tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html b/tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html new file mode 100644 index 00000000000..d714b81518b --- /dev/null +++ b/tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html @@ -0,0 +1,34 @@ + + +CSSOM - Serialization with variable preserves original serialization. + + +
+
+
+
+ From 835d95eb41da12501cfb13a18d3aec48534bbfff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 26 Mar 2017 17:45:53 +0200 Subject: [PATCH 2/3] style: Correctly track whether a longhand contains any variable. --- .../style/properties/declaration_block.rs | 81 ++++++++++++------- .../style/properties/properties.mako.rs | 18 +++-- 2 files changed, 63 insertions(+), 36 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index e7adda198a8..00dd7cc01bb 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -333,7 +333,9 @@ impl PropertyDeclarationBlock { } let iter = self.declarations.iter().map(get_declaration as fn(_) -> _); match shorthand.get_shorthand_appendable_value(iter) { - Some(AppendableValue::Css(css)) => dest.write_str(css), + Some(AppendableValue::Css { css, .. }) => { + dest.write_str(css) + }, Some(AppendableValue::DeclarationsForShorthand(_, decls)) => { shorthand.longhands_to_css(decls, dest) } @@ -428,15 +430,25 @@ impl ToCss for PropertyDeclarationBlock { Importance::Normal }; - // Substep 5 - Let value be the result of invoking serialize a CSS - // value of current longhands. + // Substep 5 - Let value be the result of invoking serialize + // a CSS value of current longhands. let mut value = String::new(); - match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { - None => continue, - Some(appendable_value) => { - try!(append_declaration_value(&mut value, appendable_value)); - } - } + let has_variables = { + let appendable_value = + match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { + None => continue, + Some(appendable_value) => appendable_value, + }; + + let has_variables = match appendable_value { + AppendableValue::Css { with_variables, .. } => with_variables, + _ => false, + }; + + append_declaration_value(&mut value, appendable_value)?; + + has_variables + }; // Substep 6 if value.is_empty() { @@ -444,9 +456,12 @@ impl ToCss for PropertyDeclarationBlock { } // Substeps 7 and 8 - try!(append_serialization::>, _>( - dest, &shorthand, AppendableValue::Css(&value), importance, - &mut is_first_serialization)); + append_serialization::<_, Cloned>, _>( + dest, + &shorthand, + AppendableValue::Css { css: &value, with_variables: has_variables }, + importance, + &mut is_first_serialization)?; for current_longhand in current_longhands { // Substep 9 @@ -456,8 +471,8 @@ impl ToCss for PropertyDeclarationBlock { // Substep 10 longhands.remove(index); } - } - } + } + } } // Step 3.3.4 @@ -503,7 +518,12 @@ pub enum AppendableValue<'a, I> DeclarationsForShorthand(ShorthandId, I), /// A raw CSS string, coming for example from a property with CSS variables, /// or when storing a serialized shorthand value before appending directly. - Css(&'a str) + Css { + /// The raw CSS string. + css: &'a str, + /// Whether the original serialization contained variables or not. + with_variables: bool, + } } /// Potentially appends whitespace after the first (property: value;) pair. @@ -513,12 +533,11 @@ fn handle_first_serialization(dest: &mut W, where W: fmt::Write, { if !*is_first_serialization { - try!(write!(dest, " ")); + dest.write_str(" ") } else { *is_first_serialization = false; + Ok(()) } - - Ok(()) } /// Append a given kind of appendable value to a serialization. @@ -529,18 +548,16 @@ pub fn append_declaration_value<'a, W, I>(dest: &mut W, I: Iterator, { match appendable_value { - AppendableValue::Css(css) => { - try!(write!(dest, "{}", css)) + AppendableValue::Css { css, .. } => { + dest.write_str(css) }, AppendableValue::Declaration(decl) => { - try!(decl.to_css(dest)); + decl.to_css(dest) }, AppendableValue::DeclarationsForShorthand(shorthand, decls) => { - try!(shorthand.longhands_to_css(decls, dest)); + shorthand.longhands_to_css(decls, dest) } } - - Ok(()) } /// Append a given property and value pair to a serialization. @@ -560,17 +577,21 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W, try!(dest.write_char(':')); // for normal parsed values, add a space between key: and value - match &appendable_value { - &AppendableValue::Declaration(decl) => { + match appendable_value { + AppendableValue::Declaration(decl) => { if !decl.value_is_unparsed() { - // for normal parsed values, add a space between key: and value - try!(write!(dest, " ")); + // For normal parsed values, add a space between key: and value. + dest.write_str(" ")? } }, + AppendableValue::Css { with_variables, .. } => { + if !with_variables { + dest.write_str(" ")? + } + } // Currently append_serialization is only called with a Css or // a Declaration AppendableValue. - &AppendableValue::DeclarationsForShorthand(..) => unreachable!(), - &AppendableValue::Css(_) => {} + AppendableValue::DeclarationsForShorthand(..) => unreachable!(), } try!(append_declaration_value(dest, appendable_value)); diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 49f88fd31aa..5c482fb5f7e 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -561,8 +561,8 @@ impl ShorthandId { /// /// Returns the optional appendable value. pub fn get_shorthand_appendable_value<'a, I>(self, - declarations: I) - -> Option> + declarations: I) + -> Option> where I: IntoIterator, I::IntoIter: Clone, { @@ -580,15 +580,21 @@ impl ShorthandId { // https://drafts.csswg.org/css-variables/#variables-in-shorthands if let Some(css) = first_declaration.with_variables_from_shorthand(self) { if declarations2.all(|d| d.with_variables_from_shorthand(self) == Some(css)) { - return Some(AppendableValue::Css(css)); - } - return None; + return Some(AppendableValue::Css { + css: css, + with_variables: true, + }); + } + return None; } // 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 Some(AppendableValue::Css { + css: keyword.to_str(), + with_variables: false, + }); } return None; } From 66f00c5566bf7359e399fdc7d64c99b9d6d78859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 26 Mar 2017 17:47:29 +0200 Subject: [PATCH 3/3] style: Multiple style nits and cleanups. --- .../style/properties/declaration_block.rs | 71 +++++++++++-------- .../style/properties/properties.mako.rs | 4 +- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 00dd7cc01bb..4d3da86906c 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -411,11 +411,13 @@ impl ToCss for PropertyDeclarationBlock { } } - // Substep 1 - /* Assuming that the PropertyDeclarationBlock contains no duplicate entries, - if the current_longhands length is equal to the properties length, it means - that the properties that map to shorthand are present in longhands */ - if current_longhands.is_empty() || current_longhands.len() != properties.len() { + // Substep 1: + // + // Assuming that the PropertyDeclarationBlock contains no + // duplicate entries, if the current_longhands length is + // equal to the properties length, it means that the + // properties that map to shorthand are present in longhands + if current_longhands.len() != properties.len() { continue; } @@ -432,41 +434,50 @@ impl ToCss for PropertyDeclarationBlock { // Substep 5 - Let value be the result of invoking serialize // a CSS value of current longhands. - let mut value = String::new(); - let has_variables = { - let appendable_value = - match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { - None => continue, - Some(appendable_value) => appendable_value, - }; - - let has_variables = match appendable_value { - AppendableValue::Css { with_variables, .. } => with_variables, - _ => false, + let appendable_value = + match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { + None => continue, + Some(appendable_value) => appendable_value, }; - append_declaration_value(&mut value, appendable_value)?; + // We avoid re-serializing if we're already an + // AppendableValue::Css. + let mut value = String::new(); + let value = match appendable_value { + AppendableValue::Css { css, with_variables } => { + debug_assert!(!css.is_empty()); + AppendableValue::Css { + css: css, + with_variables: with_variables, + } + } + other @ _ => { + append_declaration_value(&mut value, other)?; - has_variables + // Substep 6 + if value.is_empty() { + continue; + } + + AppendableValue::Css { + css: &value, + with_variables: false, + } + } }; - // Substep 6 - if value.is_empty() { - continue - } - // Substeps 7 and 8 append_serialization::<_, Cloned>, _>( dest, &shorthand, - AppendableValue::Css { css: &value, with_variables: has_variables }, + value, importance, &mut is_first_serialization)?; - for current_longhand in current_longhands { + for current_longhand in ¤t_longhands { // Substep 9 already_serialized.push(current_longhand.id()); - let index_to_remove = longhands.iter().position(|l| l.0 == *current_longhand); + let index_to_remove = longhands.iter().position(|l| l.0 == **current_longhand); if let Some(index) = index_to_remove { // Substep 10 longhands.remove(index); @@ -488,12 +499,12 @@ impl ToCss for PropertyDeclarationBlock { // "error: unable to infer enough type information about `_`; // type annotations or generic parameter binding required [E0282]" // Use the same type as earlier call to reuse generated code. - try!(append_serialization::>, _>( + append_serialization::<_, Cloned>, _>( dest, &property, AppendableValue::Declaration(declaration), importance, - &mut is_first_serialization)); + &mut is_first_serialization)?; // Step 3.3.8 already_serialized.push(property); @@ -597,10 +608,10 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W, try!(append_declaration_value(dest, appendable_value)); if importance.important() { - try!(write!(dest, " !important")); + try!(dest.write_str(" !important")); } - write!(dest, ";") + dest.write_char(';') } /// A helper to parse the style attribute of an element, in order for this to be diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 5c482fb5f7e..af392618f5e 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1197,10 +1197,10 @@ impl PropertyDeclaration { /// the longhand declarations. pub fn may_serialize_as_part_of_shorthand(&self) -> bool { match *self { - PropertyDeclaration::CSSWideKeyword(..) => false, + PropertyDeclaration::CSSWideKeyword(..) | PropertyDeclaration::WithVariables(..) => false, PropertyDeclaration::Custom(..) => - unreachable!("Serialize a custom property as part of shorthand?"), + unreachable!("Serializing a custom property as part of shorthand?"), _ => true, } }