Bug 1330172 part 2 - Fix serialization of property declaration with variable reference. r=heycam draft
authorXidorn Quan <me@upsuper.org>
Fri, 20 Jan 2017 22:35:12 +1100
changeset 464831 0cf558db29680bcfe76beda2ed097bb515f91f0f
parent 464830 4aaf85d874311b7db7281def0e2c16b17c4455c0
child 543002 64dca35b2091b8ff01507d58b6803060ce902260
push id42442
push userxquan@mozilla.com
push dateMon, 23 Jan 2017 00:26:10 +0000
reviewersheycam
bugs1330172
milestone53.0a1
Bug 1330172 part 2 - Fix serialization of property declaration with variable reference. r=heycam MozReview-Commit-ID: ATDj4lHrtR1
layout/style/Declaration.cpp
layout/style/Declaration.h
layout/style/test/test_value_storage.html
layout/style/test/test_variables.html
--- a/layout/style/Declaration.cpp
+++ b/layout/style/Declaration.cpp
@@ -246,28 +246,32 @@ Declaration::HasProperty(nsCSSPropertyID
                                       ? mImportantData : mData;
   const nsCSSValue *val = data->ValueFor(aProperty);
   return !!val;
 }
 
 bool
 Declaration::AppendValueToString(nsCSSPropertyID aProperty,
                                  nsAString& aResult,
-                                 nsCSSValue::Serialization aSerialization) const
+                                 nsCSSValue::Serialization aSerialization,
+                                 bool* aIsTokenStream) const
 {
   MOZ_ASSERT(0 <= aProperty && aProperty < eCSSProperty_COUNT_no_shorthands,
              "property ID out of range");
 
   nsCSSCompressedDataBlock *data = GetPropertyIsImportantByID(aProperty)
                                       ? mImportantData : mData;
   const nsCSSValue *val = data->ValueFor(aProperty);
   if (!val) {
     return false;
   }
 
+  if (aIsTokenStream) {
+    *aIsTokenStream = val->GetUnit() == eCSSUnit_TokenStream;
+  }
   val->AppendToString(aProperty, aResult, aSerialization);
   return true;
 }
 
 static void
 AppendSingleImageLayerPositionValue(const nsCSSValue& aPositionX,
                                     const nsCSSValue& aPositionY,
                                     const nsCSSPropertyID aTable[],
@@ -531,23 +535,26 @@ Declaration::GetImageLayerPositionValue(
     aValue.Append(char16_t(','));
     aValue.Append(char16_t(' '));
   }
 }
 
 void
 Declaration::GetPropertyValueInternal(
     nsCSSPropertyID aProperty, nsAString& aValue,
-    nsCSSValue::Serialization aSerialization) const
+    nsCSSValue::Serialization aSerialization, bool* aIsTokenStream) const
 {
   aValue.Truncate(0);
+  if (aIsTokenStream) {
+    *aIsTokenStream = false;
+  }
 
   // simple properties are easy.
   if (!nsCSSProps::IsShorthand(aProperty)) {
-    AppendValueToString(aProperty, aValue, aSerialization);
+    AppendValueToString(aProperty, aValue, aSerialization, aIsTokenStream);
     return;
   }
 
   // DOM Level 2 Style says (when describing CSS2Properties, although
   // not CSSStyleDeclaration.getPropertyValue):
   //   However, if there is no shorthand declaration that could be added
   //   to the ruleset without changing in any way the rules already
   //   declared in the ruleset (i.e., by adding longhand rules that were
@@ -635,16 +642,19 @@ Declaration::GetPropertyValueInternal(
       unsetCount != 0 || nonMatchingTokenStreamCount != 0) {
     // Case (2): partially initial, inherit, unset or token stream.
     return;
   }
   if (tokenStream) {
     if (matchingTokenStreamCount == totalCount) {
       // Shorthand was specified using variable references and all of its
       // longhand components were set by the shorthand.
+      if (aIsTokenStream) {
+        *aIsTokenStream = true;
+      }
       aValue.Append(tokenStream->GetTokenStreamValue()->mTokenStream);
     } else {
       // In all other cases, serialize to the empty string.
     }
     return;
   }
 
   nsCSSCompressedDataBlock *data = importantCount ? mImportantData : mData;
@@ -1600,31 +1610,39 @@ Declaration::GetPropertyIsImportantByID(
       return false;
     }
   }
   return true;
 }
 
 void
 Declaration::AppendPropertyAndValueToString(nsCSSPropertyID aProperty,
+                                            nsAString& aResult,
                                             nsAutoString& aValue,
-                                            nsAString& aResult) const
+                                            bool aValueIsTokenStream) const
 {
   MOZ_ASSERT(0 <= aProperty && aProperty < eCSSProperty_COUNT,
              "property enum out of range");
   MOZ_ASSERT((aProperty < eCSSProperty_COUNT_no_shorthands) == aValue.IsEmpty(),
              "aValue should be given for shorthands but not longhands");
   AppendASCIItoUTF16(nsCSSProps::GetStringValue(aProperty), aResult);
-  aResult.AppendLiteral(": ");
-  if (aValue.IsEmpty())
-    AppendValueToString(aProperty, aResult, nsCSSValue::eNormalized);
-  else
-    aResult.Append(aValue);
+  if (aValue.IsEmpty()) {
+    AppendValueToString(aProperty, aValue,
+                        nsCSSValue::eNormalized, &aValueIsTokenStream);
+  }
+  aResult.Append(':');
+  if (!aValueIsTokenStream) {
+    aResult.Append(' ');
+  }
+  aResult.Append(aValue);
   if (GetPropertyIsImportantByID(aProperty)) {
-    aResult.AppendLiteral(" !important");
+    if (!aValueIsTokenStream) {
+      aResult.Append(' ');
+    }
+    aResult.AppendLiteral("!important");
   }
   aResult.AppendLiteral("; ");
 }
 
 void
 Declaration::AppendVariableAndValueToString(const nsAString& aName,
                                             nsAString& aResult) const
 {
@@ -1733,42 +1751,47 @@ Declaration::ToString(nsAString& aString
     for (const nsCSSPropertyID *shorthands =
            nsCSSProps::ShorthandsContaining(property);
          *shorthands != eCSSProperty_UNKNOWN; ++shorthands) {
       // ShorthandsContaining returns the shorthands in order from those
       // that contain the most subproperties to those that contain the
       // least, which is exactly the order we want to test them.
       nsCSSPropertyID shorthand = *shorthands;
 
-      GetPropertyValueByID(shorthand, value);
+      bool isTokenStream;
+      GetPropertyValueInternal(shorthand, value,
+                               nsCSSValue::eNormalized, &isTokenStream);
 
       // in the system font case, skip over font-variant shorthand, since all
       // subproperties are already dealt with via the font shorthand
       if (shorthand == eCSSProperty_font_variant &&
           value.EqualsLiteral("-moz-use-system-font")) {
         continue;
       }
 
-      // If GetPropertyValueByID gives us a non-empty string back, we can
+      // If GetPropertyValueInternal gives us a non-empty string back, we can
       // use that value; otherwise it's not possible to use this shorthand.
       if (!value.IsEmpty()) {
-        AppendPropertyAndValueToString(shorthand, value, aString);
+        AppendPropertyAndValueToString(shorthand, aString,
+                                       value, isTokenStream);
         shorthandsUsed.AppendElement(shorthand);
         doneProperty = true;
         break;
       }
 
       if (shorthand == eCSSProperty_font) {
         if (haveSystemFont && !didSystemFont) {
           // Output the shorthand font declaration that we will
           // partially override later.  But don't add it to
           // |shorthandsUsed|, since we will have to override it.
           systemFont->AppendToString(eCSSProperty__x_system_font, value,
                                      nsCSSValue::eNormalized);
-          AppendPropertyAndValueToString(eCSSProperty_font, value, aString);
+          isTokenStream = systemFont->GetUnit() == eCSSUnit_TokenStream;
+          AppendPropertyAndValueToString(eCSSProperty_font, aString,
+                                         value, isTokenStream);
           value.Truncate();
           didSystemFont = true;
         }
 
         // That we output the system font is enough for this property if:
         //   (1) it's the hidden system font subproperty (which either
         //       means we output it or we don't have it), or
         //   (2) its value is the hidden system font value and it matches
@@ -1781,17 +1804,17 @@ Declaration::ToString(nsAString& aString
           break;
         }
       }
     }
     if (doneProperty)
       continue;
 
     MOZ_ASSERT(value.IsEmpty(), "value should be empty now");
-    AppendPropertyAndValueToString(property, value, aString);
+    AppendPropertyAndValueToString(property, aString, value, false);
   }
   if (! aString.IsEmpty()) {
     // if the string is not empty, we have trailing whitespace we
     // should remove
     aString.Truncate(aString.Length() - 1);
   }
 }
 
--- a/layout/style/Declaration.h
+++ b/layout/style/Declaration.h
@@ -305,29 +305,31 @@ public:
     return nullptr;
   }
 
 private:
   Declaration& operator=(const Declaration& aCopy) = delete;
   bool operator==(const Declaration& aCopy) const = delete;
 
   void GetPropertyValueInternal(nsCSSPropertyID aProperty, nsAString& aValue,
-                                nsCSSValue::Serialization aValueSerialization)
-    const;
+                                nsCSSValue::Serialization aValueSerialization,
+                                bool* aIsTokenStream = nullptr) const;
   bool GetPropertyIsImportantByID(nsCSSPropertyID aProperty) const;
 
   static void AppendImportanceToString(bool aIsImportant, nsAString& aString);
   // return whether there was a value in |aValue| (i.e., it had a non-null unit)
   bool AppendValueToString(nsCSSPropertyID aProperty, nsAString& aResult) const;
   bool AppendValueToString(nsCSSPropertyID aProperty, nsAString& aResult,
-                           nsCSSValue::Serialization aValueSerialization) const;
+                           nsCSSValue::Serialization aValueSerialization,
+                           bool* aIsTokenStream = nullptr) const;
   // Helper for ToString with strange semantics regarding aValue.
   void AppendPropertyAndValueToString(nsCSSPropertyID aProperty,
+                                      nsAString& aResult,
                                       nsAutoString& aValue,
-                                      nsAString& aResult) const;
+                                      bool aValueIsTokenStream) const;
   // helper for ToString that serializes a custom property declaration for
   // a variable with the specified name
   void AppendVariableAndValueToString(const nsAString& aName,
                                       nsAString& aResult) const;
 
   void GetImageLayerValue(nsCSSCompressedDataBlock *data,
                           nsAString& aValue,
                           nsCSSValue::Serialization aSerialization,
--- a/layout/style/test/test_value_storage.html
+++ b/layout/style/test/test_value_storage.html
@@ -150,16 +150,17 @@ function test_property(property)
       is(gDeclaration.getPropertyValue(sh), "",
          "setting '" + value + "' on '" + property + "' (for shorthand '" + sh + "')");
     }
   }
 
   function test_value(value, resolved_value) {
     var value_has_variable_reference = resolved_value != null;
 
+    var colon = value == "var(--a)" ? ":" : ": ";
     gDeclaration.setProperty(property, value, "");
 
     var idx;
 
     var step1val = gDeclaration.getPropertyValue(property);
     var step1vals = [];
     var step1ser = gDeclaration.cssText;
     if ("subproperties" in info)
@@ -188,30 +189,30 @@ function test_property(property)
         }
       }
 
     // We don't care particularly about the whitespace or the placement of
     // semicolons, but for simplicity we'll test the current behavior.
     var expected_serialization = "";
     if (step1val != "") {
       if ("alias_for" in info) {
-        expected_serialization = info.alias_for + ": " + step1val + ";";
+        expected_serialization = info.alias_for + colon + step1val + ";";
       } else {
-        expected_serialization = property + ": " + step1val + ";";
+        expected_serialization = property + colon + step1val + ";";
       }
     }
     is(step1ser, expected_serialization,
        "serialization should match property value");
 
     gDeclaration.removeProperty(property);
     gDeclaration.setProperty(property, step1val, "");
 
     is(gDeclaration.getPropertyValue(property), step1val,
        "parse+serialize should be idempotent for '" +
-         property + ": " + value + "'");
+         property + colon + value + "'");
     if (info.type != CSS_TYPE_TRUE_SHORTHAND) {
       is(gComputedStyle.getPropertyValue(property), step1comp,
          "serialize+parse should be identity transform for '" +
          property + ": " + value + "'");
     }
 
     if ("subproperties" in info &&
         // Using setProperty over subproperties is not sufficient for
@@ -232,17 +233,17 @@ function test_property(property)
       for (idx in info.subproperties) {
         var subprop = info.subproperties[idx];
         is(gComputedStyle.getPropertyValue(subprop), step1comps[idx],
            "serialize(" + subprop + ")+parse should be the identity " +
            "transform for '" + property + ": " + value + "'");
       }
       is(gDeclaration.getPropertyValue(property), step1val,
          "parse+split+serialize should be idempotent for '" +
-         property + ": " + value + "'");
+         property + colon + value + "'");
     }
 
     if (info.type != CSS_TYPE_TRUE_SHORTHAND &&
           property != "mask") {
       gDeclaration.removeProperty(property);
       gDeclaration.setProperty(property, step1comp, "");
       var func = xfail_compute(property, value) ? todo_is : is;
       func(gComputedStyle.getPropertyValue(property), step1comp,
--- a/layout/style/test/test_variables.html
+++ b/layout/style/test/test_variables.html
@@ -97,19 +97,19 @@ var tests = [
     test6.style.color = "white";
     is(declaration.getPropertyValue("-x-system-font"), " var(--var6) hangul mongolian");
   },
 
   function() {
     // https://bugzilla.mozilla.org/show_bug.cgi?id=1154356
     var test7 = document.getElementById("test7");
     test7.textContent = "p { --weird\\;name: green; }";
-    is(test7.sheet.cssRules[0].style.cssText, "--weird\\;name:  green;");
+    is(test7.sheet.cssRules[0].style.cssText, "--weird\\;name: green;");
     test7.textContent = "p { --0: green; }";
-    is(test7.sheet.cssRules[0].style.cssText, "--0:  green;");
+    is(test7.sheet.cssRules[0].style.cssText, "--0: green;");
   },
 
   function() {
     // https://bugzilla.mozilla.org/show_bug.cgi?id=1330172
     var test8 = document.getElementById("test8");
     test8.textContent = "p { --a:inHerit; }";
     is(test8.sheet.cssRules[0].style.cssText, "--a: inherit;");
     test8.textContent = "p { --b: initial!important; }";