Handle serialization of inherit and -moz-initial for shorthand properties correctly, and refuse to serialize most unspecifiable cases of shorthands. (Bug 160403) r+sr=bzbarsky
authorL. David Baron <dbaron@dbaron.org>
Tue, 07 Oct 2008 15:10:20 -0700
changeset 20117 7356c512e9e157f269c4f7b1871983c6541104f1
parent 20116 08d7f38b8ee3f66250955f7420f2a1fd0e3388f5
child 20118 4c7ccb9f3e70e57dc8b9c122c55ca46ffd378ca5
push id2647
push userdbaron@mozilla.com
push dateTue, 07 Oct 2008 23:20:21 +0000
treeherdermozilla-central@7356c512e9e1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs160403
milestone1.9.1b2pre
Handle serialization of inherit and -moz-initial for shorthand properties correctly, and refuse to serialize most unspecifiable cases of shorthands. (Bug 160403) r+sr=bzbarsky
layout/style/nsCSSDeclaration.cpp
layout/style/nsCSSDeclaration.h
layout/style/nsCSSParser.cpp
layout/style/test/Makefile.in
layout/style/test/test_bug160403.html
layout/style/test/test_inherit_storage.html
layout/style/test/test_initial_storage.html
--- a/layout/style/nsCSSDeclaration.cpp
+++ b/layout/style/nsCSSDeclaration.cpp
@@ -174,24 +174,16 @@ nsCSSDeclaration::GetValueOrImportantVal
                                      ? mImportantData : mData;
   const void *storage = data->StorageFor(aProperty);
   if (!storage)
     return NS_OK;
   aValue = *static_cast<const nsCSSValue*>(storage);
   return NS_OK;
 }
 
-nsresult
-nsCSSDeclaration::GetValue(const nsAString& aProperty,
-                           nsAString& aValue) const
-{
-  nsCSSProperty propID = nsCSSProps::LookupProperty(aProperty);
-  return GetValue(propID, aValue);
-}
-
 PRBool nsCSSDeclaration::AppendValueToString(nsCSSProperty aProperty, nsAString& aResult) const
 {
   nsCSSCompressedDataBlock *data = GetValueIsImportant(aProperty)
                                       ? mImportantData : mData;
   const void *storage = data->StorageFor(aProperty);
   if (storage) {
     switch (nsCSSProps::kTypeTable[aProperty]) {
       case eCSSType_Value: {
@@ -530,23 +522,115 @@ nsCSSDeclaration::GetValue(nsCSSProperty
   aValue.Truncate(0);
 
   // simple properties are easy.
   if (!nsCSSProps::IsShorthand(aProperty)) {
     AppendValueToString(aProperty, aValue);
     return NS_OK;
   }
 
-  // shorthands
+  // 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
+  //   previously not declared in the ruleset), then the empty string
+  //   should be returned for the shorthand property.
+  // This means we need to check a number of cases:
+  //   (1) Since a shorthand sets all sub-properties, if some of its
+  //       subproperties were not specified, we must return the empty
+  //       string.
+  //   (2) Since 'inherit' and 'initial' can only be specified as the
+  //       values for entire properties, we need to return the empty
+  //       string if some but not all of the subproperties have one of
+  //       those values.
+  //   (3) Since a single value only makes sense with or without
+  //       !important, we return the empty string if some values are
+  //       !important and some are not.
+  // Since we're doing this check for 'inherit' and 'initial' up front,
+  // we can also simplify the property serialization code by serializing
+  // those values up front as well.
+  PRUint32 totalCount = 0, importantCount = 0,
+           initialCount = 0, inheritCount = 0;
   CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aProperty) {
-    if (!mData->StorageFor(*p) &&
-        (!mImportantData || !mImportantData->StorageFor(*p)))
-      // We don't have all the properties in the shorthand.
-      if (*p != eCSSProperty__x_system_font)
-        return NS_OK;
+    if (*p == eCSSProperty__x_system_font ||
+         nsCSSProps::kFlagsTable[*p] & CSS_PROPERTY_DIRECTIONAL_SOURCE) {
+      // The system-font subproperty and the *-source properties don't count.
+      continue;
+    }
+    ++totalCount;
+    const void *storage = mData->StorageFor(*p);
+    NS_ASSERTION(!storage || !mImportantData || !mImportantData->StorageFor(*p),
+                 "can't be in both blocks");
+    if (!storage && mImportantData) {
+      ++importantCount;
+      storage = mImportantData->StorageFor(*p);
+    }
+    if (!storage) {
+      // Case (1) above: some subproperties not specified.
+      return NS_OK;
+    }
+    nsCSSUnit unit;
+    switch (nsCSSProps::kTypeTable[*p]) {
+      case eCSSType_Value: {
+        const nsCSSValue *val = static_cast<const nsCSSValue*>(storage);
+        unit = val->GetUnit();
+      } break;
+      case eCSSType_Rect: {
+        const nsCSSRect *rect = static_cast<const nsCSSRect*>(storage);
+        unit = rect->mTop.GetUnit();
+      } break;
+      case eCSSType_ValuePair: {
+        const nsCSSValuePair *pair = static_cast<const nsCSSValuePair*>(storage);
+        unit = pair->mXValue.GetUnit();
+      } break;
+      case eCSSType_ValueList: {
+        const nsCSSValueList* item =
+            *static_cast<nsCSSValueList*const*>(storage);
+        if (item) {
+          unit = item->mValue.GetUnit();
+        } else {
+          unit = eCSSUnit_Null;
+        }
+      } break;
+      case eCSSType_ValuePairList: {
+        const nsCSSValuePairList* item =
+            *static_cast<nsCSSValuePairList*const*>(storage);
+        if (item) {
+          unit = item->mXValue.GetUnit();
+        } else {
+          unit = eCSSUnit_Null;
+        }
+      } break;
+    }
+    if (unit == eCSSUnit_Inherit) {
+      ++inheritCount;
+    } else if (unit == eCSSUnit_Initial) {
+      ++initialCount;
+    }
+  }
+  if (importantCount != 0 && importantCount != totalCount) {
+    // Case (3), no consistent importance.
+    return NS_OK;
+  }
+  if (initialCount == totalCount) {
+    // Simplify serialization below by serializing initial up-front.
+    AppendCSSValueToString(eCSSProperty_UNKNOWN, nsCSSValue(eCSSUnit_Initial),
+                           aValue);
+    return NS_OK;
+  }
+  if (inheritCount == totalCount) {
+    // Simplify serialization below by serializing inherit up-front.
+    AppendCSSValueToString(eCSSProperty_UNKNOWN, nsCSSValue(eCSSUnit_Inherit),
+                           aValue);
+    return NS_OK;
+  }
+  if (initialCount != 0 || inheritCount != 0) {
+    // Case (2): partially initial or inherit.
+    return NS_OK;
   }
 
 
   // XXX What about checking the consistency of '!important'?
   // XXX What about checking that we don't serialize inherit,
   // -moz-initial, or other illegal values?
   // XXXldb Can we share shorthand logic with ToString?
   switch (aProperty) {
@@ -789,17 +873,32 @@ nsCSSDeclaration::GetValueIsImportant(co
 }
 
 PRBool
 nsCSSDeclaration::GetValueIsImportant(nsCSSProperty aProperty) const
 {
   if (!mImportantData)
     return PR_FALSE;
 
-  // Inefficient, but we can assume '!important' is rare.
+  // Calling StorageFor is inefficient, but we can assume '!important'
+  // is rare.
+
+  if (nsCSSProps::IsShorthand(aProperty)) {
+    CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aProperty) {
+      if (*p == eCSSProperty__x_system_font) {
+        // The system_font subproperty doesn't count.
+        continue;
+      }
+      if (!mImportantData->StorageFor(*p)) {
+        return PR_FALSE;
+      }
+    }
+    return PR_TRUE;
+  }
+
   return mImportantData->StorageFor(aProperty) != nsnull;
 }
 
 // XXXldb Bug 376075 All callers of AllPropertiesSameImportance also
 // need to check for 'inherit' and 'initial' values, since you can't
 // output a mix of either mixed with other values in the same shorthand!
 PRBool
 nsCSSDeclaration::AllPropertiesSameImportance(PRInt32 aFirst, PRInt32 aSecond,
--- a/layout/style/nsCSSDeclaration.h
+++ b/layout/style/nsCSSDeclaration.h
@@ -72,17 +72,16 @@ public:
    * for this declaration.
    */
   nsresult ValueAppended(nsCSSProperty aProperty);
 
   nsresult AppendComment(const nsAString& aComment);
   nsresult RemoveProperty(nsCSSProperty aProperty);
 
   nsresult GetValue(nsCSSProperty aProperty, nsAString& aValue) const;
-  nsresult GetValue(const nsAString& aProperty, nsAString& aValue) const;
 
   PRBool HasImportantData() const { return mImportantData != nsnull; }
   PRBool GetValueIsImportant(nsCSSProperty aProperty) const;
   PRBool GetValueIsImportant(const nsAString& aProperty) const;
 
   PRUint32 Count() const {
     return mOrder.Length(); 
   }
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -5780,21 +5780,24 @@ CSSParserImpl::ParseBackground()
   mTempData.SetPropertyBit(eCSSProperty_background_repeat);
   mTempData.mColor.mBackAttachment.SetIntValue(NS_STYLE_BG_ATTACHMENT_SCROLL,
                                                eCSSUnit_Enumerated);
   mTempData.SetPropertyBit(eCSSProperty_background_attachment);
   mTempData.mColor.mBackPosition.mXValue.SetPercentValue(0.0f);
   mTempData.mColor.mBackPosition.mYValue.SetPercentValue(0.0f);
   mTempData.SetPropertyBit(eCSSProperty_background_position);
   // including the ones that we can't set from the shorthand.
-  mTempData.mColor.mBackClip.SetInitialValue();
+  mTempData.mColor.mBackClip.SetIntValue(NS_STYLE_BG_CLIP_BORDER,
+                                         eCSSUnit_Enumerated);
   mTempData.SetPropertyBit(eCSSProperty__moz_background_clip);
-  mTempData.mColor.mBackOrigin.SetInitialValue();
+  mTempData.mColor.mBackOrigin.SetIntValue(NS_STYLE_BG_ORIGIN_PADDING,
+                                           eCSSUnit_Enumerated);
   mTempData.SetPropertyBit(eCSSProperty__moz_background_origin);
-  mTempData.mColor.mBackInlinePolicy.SetInitialValue();
+  mTempData.mColor.mBackInlinePolicy.SetIntValue(
+    NS_STYLE_BG_INLINE_POLICY_CONTINUOUS, eCSSUnit_Enumerated);
   mTempData.SetPropertyBit(eCSSProperty__moz_background_inline_policy);
 
   // XXX If ParseSingleValueProperty were table-driven (bug 376079) and
   // automatically filled in the right field of mTempData, we could move
   // ParseBackgroundPosition to it (as a special case) and switch back
   // to using ParseChoice here.
 
   PRBool haveColor = PR_FALSE,
--- a/layout/style/test/Makefile.in
+++ b/layout/style/test/Makefile.in
@@ -67,16 +67,17 @@ css_properties.js: host_ListCSSPropertie
 	cat $(srcdir)/css_properties_like_longhand.js >> $@
 
 GARBAGE += css_properties.js
 
 _TEST_FILES =	test_acid3_test46.html \
 		test_bug73586.html \
 		test_bug74880.html \
 		test_bug98997.html \
+		test_bug160403.html \
 		test_bug221428.html \
 		test_bug229915.html \
 		test_bug302186.html \
 		test_bug319381.html \
 		test_bug357614.html \
 		test_bug363146.html \
 		test_bug365932.html \
 		test_bug372770.html \
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_bug160403.html
@@ -0,0 +1,62 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=160403
+-->
+<head>
+  <title>Test for Bug 160403</title>
+  <script type="application/javascript" src="/MochiKit/MochiKit.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=160403">Mozilla Bug 160403</a>
+<div id="content" style="display: none">
+  
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 160403 **/
+
+var element = document.getElementById("content");
+var style = element.style;
+
+element.setAttribute("style", "border-top-style: dotted");
+is(style.getPropertyValue("border-top-style"), "dotted");
+is(style.getPropertyPriority("border-top-style"), "");
+is(style.getPropertyValue("border-style"), "");
+is(style.getPropertyPriority("border-style"), "");
+
+element.setAttribute("style", "border-top-style: dotted ! important");
+is(style.getPropertyValue("border-top-style"), "dotted");
+is(style.getPropertyPriority("border-top-style"), "important");
+is(style.getPropertyValue("border-style"), "");
+is(style.getPropertyPriority("border-style"), "");
+
+element.setAttribute("style", "border-top-style: dotted ! important; border-bottom-style: dotted ! important; border-left-style: dotted ! important");
+is(style.getPropertyValue("border-top-style"), "dotted");
+is(style.getPropertyPriority("border-top-style"), "important");
+is(style.getPropertyValue("border-style"), "");
+is(style.getPropertyPriority("border-style"), "");
+
+element.setAttribute("style", "border-top-style: dotted ! important; border-right-style: dotted; border-bottom-style: dotted ! important; border-left-style: dotted ! important");
+is(style.getPropertyValue("border-top-style"), "dotted");
+is(style.getPropertyPriority("border-top-style"), "important");
+is(style.getPropertyValue("border-right-style"), "dotted");
+is(style.getPropertyPriority("border-right-style"), "");
+is(style.getPropertyValue("border-style"), "");
+is(style.getPropertyPriority("border-style"), "");
+
+element.setAttribute("style", "border-top-style: dotted ! important; border-right-style: dotted ! important; border-bottom-style: dotted ! important; border-left-style: dotted ! important");
+is(style.getPropertyValue("border-top-style"), "dotted");
+is(style.getPropertyPriority("border-top-style"), "important");
+is(style.getPropertyValue("border-right-style"), "dotted");
+is(style.getPropertyPriority("border-right-style"), "important");
+isnot(style.getPropertyValue("border-style"), "");
+is(style.getPropertyPriority("border-style"), "important");
+
+</script>
+</pre>
+</body>
+</html>
--- a/layout/style/test/test_inherit_storage.html
+++ b/layout/style/test/test_inherit_storage.html
@@ -20,41 +20,16 @@ https://bugzilla.mozilla.org/show_bug.cg
 </div>
 <pre id="test">
 <script class="testbody" type="text/javascript">
 
 /** Test for parsing, storage, and serialization of CSS 'inherit' **/
 
 var gDeclaration = document.getElementById("testnode").style;
 
-var gKnownFails = {
-  /* bug 377519: */
-  "-moz-border-end": true,
-  "-moz-border-radius": true,
-  "-moz-border-start": true,
-  "-moz-column-rule": true,
-  "-moz-outline-radius": true,
-  "background": true,
-  "border": true,
-  "border-bottom": true,
-  "border-color": true,
-  "border-left": true,
-  "border-right": true,
-  "border-style": true,
-  "border-top": true,
-  "border-width": true,
-  "cue": true,
-  "font": true,
-  "list-style": true,
-  "margin": true,
-  "outline": true,
-  "padding": true,
-  "pause": true
-};
-
 var gKnownFails2 = {
   "-moz-border-end": true,
   "-moz-border-radius": true,
   "-moz-border-start": true,
   "-moz-column-rule": true,
   "-moz-outline-radius": true,
   "background": true,
   "border": true,
@@ -87,21 +62,18 @@ function test_property(property)
     for (var idx in info.subproperties)
       check_initial(info.subproperties[idx]);
 
   gDeclaration.setProperty(property, "inherit", "");
 
   function check_set(sproperty) {
     var sinfo = gCSSProperties[sproperty];
     val = gDeclaration.getPropertyValue(sproperty);
-    var func = is;
-    if (sproperty == property && property in gKnownFails)
-      func = todo_is;
-    func(val, "inherit",
-         "inherit reported back for property '" + sproperty + "'");
+    is(val, "inherit",
+       "inherit reported back for property '" + sproperty + "'");
     if (sinfo.domProp) {
       is(val, gDeclaration[sinfo.domProp],
          "consistency between decl.getPropertyValue('" + sproperty +
          "') and decl." + sinfo.domProp);
     }
   }
   check_set(property);
   if ("subproperties" in info)
--- a/layout/style/test/test_initial_storage.html
+++ b/layout/style/test/test_initial_storage.html
@@ -20,41 +20,16 @@ https://bugzilla.mozilla.org/show_bug.cg
 </div>
 <pre id="test">
 <script class="testbody" type="text/javascript">
 
 /** Test for parsing, storage, and serialization of CSS '-moz-initial' **/
 
 var gDeclaration = document.getElementById("testnode").style;
 
-var gKnownFails = {
-  /* bug 377519: */
-  "-moz-border-end": true,
-  "-moz-border-radius": true,
-  "-moz-border-start": true,
-  "-moz-column-rule": true,
-  "-moz-outline-radius": true,
-  "background": true,
-  "border": true,
-  "border-bottom": true,
-  "border-color": true,
-  "border-left": true,
-  "border-right": true,
-  "border-style": true,
-  "border-top": true,
-  "border-width": true,
-  "cue": true,
-  "font": true,
-  "list-style": true,
-  "margin": true,
-  "outline": true,
-  "padding": true,
-  "pause": true
-};
-
 var gKnownFails2 = {
   "-moz-border-end": true,
   "-moz-border-radius": true,
   "-moz-border-start": true,
   "-moz-column-rule": true,
   "-moz-outline-radius": true,
   "background": true,
   "border": true,
@@ -87,21 +62,18 @@ function test_property(property)
     for (var idx in info.subproperties)
       check_initial(info.subproperties[idx]);
 
   gDeclaration.setProperty(property, "-moz-initial", "");
 
   function check_set(sproperty) {
     var sinfo = gCSSProperties[sproperty];
     val = gDeclaration.getPropertyValue(sproperty);
-    var func = is;
-    if (sproperty == property && property in gKnownFails)
-      func = todo_is;
-    func(val, "-moz-initial",
-         "-moz-initial reported back for property '" + sproperty + "'");
+    is(val, "-moz-initial",
+       "-moz-initial reported back for property '" + sproperty + "'");
     if (sinfo.domProp) {
       is(val, gDeclaration[sinfo.domProp],
          "consistency between decl.getPropertyValue('" + sproperty +
          "') and decl." + sinfo.domProp);
     }
   }
   check_set(property);
   if ("subproperties" in info)