Auto merge of #16135 - emilio:shorthand-reference-variable, r=upsuper

Shorthand with variable reference should not have extra whitespace after colon for serialization.

This is https://github.com/servo/servo/pull/15422 + a manifest update. Fixes  #15295.

<!-- 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/16135)
<!-- Reviewable:end -->

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)
                     }
@@ -409,11 +411,13 @@ impl ToCss for PropertyDeclarationBlock {
                         }
                     }
 
-                    // Substep 1
+                    // Substep 1:
-                    /* Assuming that the PropertyDeclarationBlock contains no duplicate entries,
+                    //
-                    if the current_longhands length is equal to the properties length, it means
+                    // Assuming that the PropertyDeclarationBlock contains no
-                    that the properties that map to shorthand are present in longhands */
+                    // duplicate entries, if the current_longhands length is
-                    if current_longhands.is_empty() || current_longhands.len() != properties.len() {
+                    // 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;
                     }
 
@@ -428,36 +432,58 @@ impl ToCss for PropertyDeclarationBlock {
                         Importance::Normal
                     };
 
-                    // Substep 5 - Let value be the result of invoking serialize a CSS
+                    // Substep 5 - Let value be the result of invoking serialize
-                    // value of current longhands.
+                    // a CSS value of current longhands.
-                    let mut value = String::new();
+                    let appendable_value =
-                    match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) {
+                        match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) {
-                        None => continue,
+                            None => continue,
-                        Some(appendable_value) => {
+                            Some(appendable_value) => appendable_value,
-                            try!(append_declaration_value(&mut value, appendable_value));
+                        };
-                        }
-                    }
 
-                    // Substep 6
+                    // We avoid re-serializing if we're already an
-                    if value.is_empty() {
+                    // AppendableValue::Css.
-                        continue
+                    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)?;
+
+                            // Substep 6
+                            if value.is_empty() {
+                                continue;
+                            }
+
+                            AppendableValue::Css {
+                                css: &value,
+                                with_variables: false,
+                            }
+                        }
+                    };
 
                     // Substeps 7 and 8
-                    try!(append_serialization::<W, Cloned<slice::Iter< _>>, _>(
+                    append_serialization::<_, Cloned<slice::Iter< _>>, _>(
-                             dest, &shorthand, AppendableValue::Css(&value), importance,
+                         dest,
-                             &mut is_first_serialization));
+                         &shorthand,
+                         value,
+                         importance,
+                         &mut is_first_serialization)?;
 
-                    for current_longhand in current_longhands {
+                    for current_longhand in &current_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);
                         }
-                     }
+                    }
-                 }
+                }
             }
 
             // Step 3.3.4
@@ -473,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::<W, Cloned<slice::Iter< &PropertyDeclaration>>, _>(
+            append_serialization::<_, Cloned<slice::Iter<_>>, _>(
                 dest,
                 &property,
                 AppendableValue::Declaration(declaration),
                 importance,
-                &mut is_first_serialization));
+                &mut is_first_serialization)?;
 
             // Step 3.3.8
             already_serialized.push(property);
@@ -503,7 +529,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 +544,11 @@ fn handle_first_serialization<W>(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 +559,16 @@ pub fn append_declaration_value<'a, W, I>(dest: &mut W,
           I: Iterator<Item=&'a PropertyDeclaration>,
 {
     match appendable_value {
-        AppendableValue::Css(css) => {
+        AppendableValue::Css { css, .. } => {
-            try!(write!(dest, "{}", 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,28 +588,30 @@ 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 {
+    match appendable_value {
-        &AppendableValue::Css(_) => {
+        AppendableValue::Declaration(decl) => {
-            try!(write!(dest, " "))
-        },
-        &AppendableValue::Declaration(decl) => {
             if !decl.value_is_unparsed() {
-                // for normal parsed values, add a space between key: and value
+                // For normal parsed values, add a space between key: and value.
-                try!(write!(dest, " "));
+                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::DeclarationsForShorthand(..) => unreachable!(),
     }
 
     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

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)
+                                                 declarations: I)
-                                             -> Option<AppendableValue<'a, I::IntoIter>>
+                                                 -> Option<AppendableValue<'a, I::IntoIter>>
         where I: IntoIterator<Item=&'a PropertyDeclaration>,
               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 Some(AppendableValue::Css {
-           }
+                   css: css,
-           return None;
+                   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;
         }
@@ -1191,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,
         }
     }

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"

tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html

@@ -0,0 +1,34 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSSOM - Serialization with variable preserves original serialization.</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div id="longhand-whitespace" style="font-size: var(--a);"></div>
+<div id="shorthand-whitespace" style="font: var(--a);"></div>
+<div id="longhand" style="font-size:var(--a);"></div>
+<div id="shorthand" style="font:var(--a);"></div>
+<script>
+    test(function() {
+        var elem = document.getElementById('longhand-whitespace');
+
+        assert_equals(elem.style.cssText, 'font-size: var(--a);');
+    }, 'Longhand with variable preserves original serialization: with withespace')
+
+    test(function() {
+        var elem = document.getElementById('shorthand-whitespace');
+
+        assert_equals(elem.style.cssText, 'font: var(--a);');
+    }, 'Shorthand with variable preserves original serialization: with withespace')
+
+    test(function() {
+        var elem = document.getElementById('longhand');
+
+        assert_equals(elem.style.cssText, 'font-size:var(--a);');
+    }, 'Longhand with variable preserves original serialization: without withespace')
+
+    test(function() {
+        var elem = document.getElementById('shorthand');
+
+        assert_equals(elem.style.cssText, 'font:var(--a);');
+    }, 'Shorthand with variable preserves original serialization: without withespace')
+</script>