Fix bugs in handling of inherit, initial, and none for -moz-border-*-colors. (Bug 389404) r+sr=bzbarsky

layout/style/nsCSSParser.cpp

@@ -6363,37 +6363,36 @@ PRBool
 CSSParserImpl::ParseBorderColors(nsCSSValueList** aResult,
                                  nsCSSProperty aProperty)
 {
-  nsCSSValue value;
-  if (ParseVariant(value, VARIANT_HCK|VARIANT_NONE, nsCSSProps::kBorderColorKTable)) {
-    nsCSSValueList* listHead = new nsCSSValueList();
-    nsCSSValueList* list = listHead;
-    if (!list) {
+  nsCSSValueList *list = nsnull;
+  for (nsCSSValueList **curp = &list, *cur; ; curp = &cur->mNext) {
+    cur = *curp = new nsCSSValueList();
+    if (!cur) {
       mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY);
-      return PR_FALSE;
+      break;
     }
-    list->mValue = value;
-
-    while (list) {
-      if (ExpectEndProperty()) {
-        mTempData.SetPropertyBit(aProperty);
-        *aResult = listHead;
-        return PR_TRUE;
-      }
-      // FIXME Bug 389404: We should not accept inherit, -moz-initial,
-      // or none as anything other than the first value.
-      if (ParseVariant(value, VARIANT_HCK|VARIANT_NONE, nsCSSProps::kBorderColorKTable)) {
-        list->mNext = new nsCSSValueList();
-        list = list->mNext;
-        if (list)
-          list->mValue = value;
-        else
-          mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY);
-      }
-      else
-        break;
+    if (!ParseVariant(cur->mValue,
+                      (cur == list)
+                        ? (VARIANT_HCK | VARIANT_NONE)
+                        : (VARIANT_COLOR | VARIANT_KEYWORD),
+                      nsCSSProps::kBorderColorKTable)) {
+      break;
+    }
+    if (ExpectEndProperty()) {
+      // Only success case here, since having the failure case at the
+      // end allows more sharing of code.
+      mTempData.SetPropertyBit(aProperty);
+      *aResult = list;
+      return PR_TRUE;
+    }
+    if (cur->mValue.GetUnit() == eCSSUnit_Inherit ||
+        cur->mValue.GetUnit() == eCSSUnit_Initial ||
+        cur->mValue.GetUnit() == eCSSUnit_None) {
+      // 'inherit', 'initial', and 'none' are only allowed on their own
+      break;
     }
-    delete listHead;
   }
+  // Have failure case at the end so we can |break| to get to it.
+  delete list;
   return PR_FALSE;
 }
 

layout/style/nsRuleNode.cpp

@@ -3819,17 +3819,19 @@ nsRuleNode::ComputeBorderData(void* aStartStruct,
                       Margin, marginData)
 
   // -moz-box-shadow: none, list, inherit, initial
-  nsCSSValueList* list = marginData.mBoxShadow;
-  if (list) {
-    // This handles 'none' and 'initial'
-    border->mBoxShadow = nsnull;
+  {
+    nsCSSValueList* list = marginData.mBoxShadow;
+    if (list) {
+      // This handles 'none' and 'initial'
+      border->mBoxShadow = nsnull;
 
-    if (eCSSUnit_Inherit == list->mValue.GetUnit()) {
-      inherited = PR_TRUE;
-      border->mBoxShadow = parentBorder->mBoxShadow;
-    } else if (eCSSUnit_Array == list->mValue.GetUnit()) {
-      // List of arrays
-      border->mBoxShadow = GetShadowData(list, aContext, PR_TRUE, inherited);
+      if (eCSSUnit_Inherit == list->mValue.GetUnit()) {
+        inherited = PR_TRUE;
+        border->mBoxShadow = parentBorder->mBoxShadow;
+      } else if (eCSSUnit_Array == list->mValue.GetUnit()) {
+        // List of arrays
+        border->mBoxShadow = GetShadowData(list, aContext, PR_TRUE, inherited);
+      }
     }
   }
 
@@ -3918,7 +3920,7 @@ nsRuleNode::ComputeBorderData(void* aStartStruct,
     }
   }
 
-  // -moz-border-*-colors: color, string, enum
+  // -moz-border-*-colors: color, string, enum, none, inherit/initial
   nscolor borderColor;
   nscolor unused = NS_RGB(0,0,0);
   
@@ -3926,17 +3928,38 @@ nsRuleNode::ComputeBorderData(void* aStartStruct,
     NS_FOR_CSS_SIDES(side) {
       nsCSSValueList* list =
           marginData.mBorderColors.*(nsCSSValueListRect::sides[side]);
-      // FIXME Bug 389404: Implement inherit and -moz-initial.
       if (list) {
-        // Some composite border color information has been specified for this
-        // border side.
-        border->EnsureBorderColors();
-        border->ClearBorderColors(side);
-        while (list) {
-          if (SetColor(list->mValue, unused, mPresContext,
-                       aContext, borderColor, inherited))
-            border->AppendBorderColor(side, borderColor);
-          list = list->mNext;
+        if (eCSSUnit_Initial == list->mValue.GetUnit() ||
+            eCSSUnit_None == list->mValue.GetUnit()) {
+          NS_ASSERTION(!list->mNext, "should have only one item");
+          border->ClearBorderColors(side);
+        }
+        else if (eCSSUnit_Inherit == list->mValue.GetUnit()) {
+          NS_ASSERTION(!list->mNext, "should have only one item");
+          nsBorderColors *parentColors;
+          parentBorder->GetCompositeColors(side, &parentColors);
+          if (parentColors) {
+            border->EnsureBorderColors();
+            border->ClearBorderColors(side);
+            border->mBorderColors[side] = parentColors->Clone();
+          } else {
+            border->ClearBorderColors(side);
+          }
+        }
+        else {
+          // Some composite border color information has been specified for this
+          // border side.
+          border->EnsureBorderColors();
+          border->ClearBorderColors(side);
+          while (list) {
+            if (SetColor(list->mValue, unused, mPresContext,
+                         aContext, borderColor, inherited))
+              border->AppendBorderColor(side, borderColor);
+            else {
+              NS_NOTREACHED("unexpected item in -moz-border-*-colors list");
+            }
+            list = list->mNext;
+          }
         }
       }
     }

layout/style/nsStyleStruct.h

@@ -431,7 +431,7 @@ struct nsStyleBorder {
   }
 
   void ClearBorderColors(PRUint8 aSide) {
-    if (mBorderColors[aSide]) {
+    if (mBorderColors && mBorderColors[aSide]) {
       delete mBorderColors[aSide];
       mBorderColors[aSide] = nsnull;
     }

layout/style/test/property_database.js

@@ -109,7 +109,7 @@ var gCSSProperties = {
 		type: CSS_TYPE_LONGHAND,
 		initial_values: [ "none" ],
 		other_values: [ "red green", "red #fc3", "#ff00cc", "currentColor", "blue currentColor orange currentColor" ],
-		invalid_values: [ "red none", "red inherit", "red, green" ]
+		invalid_values: [ "red none", "red inherit", "red, green", "none red", "inherit red" ]
 	},
 	"-moz-border-end": {
 		domProp: "MozBorderEnd",
@@ -172,7 +172,7 @@ var gCSSProperties = {
 		type: CSS_TYPE_LONGHAND,
 		initial_values: [ "none" ],
 		other_values: [ "red green", "red #fc3", "#ff00cc", "currentColor", "blue currentColor orange currentColor" ],
-		invalid_values: [ "red none", "red inherit", "red, green" ]
+		invalid_values: [ "red none", "red inherit", "red, green", "none red", "inherit red" ]
 	},
 	"-moz-border-radius": {
 		domProp: "MozBorderRadius",
@@ -231,7 +231,7 @@ var gCSSProperties = {
 		type: CSS_TYPE_LONGHAND,
 		initial_values: [ "none" ],
 		other_values: [ "red green", "red #fc3", "#ff00cc", "currentColor", "blue currentColor orange currentColor" ],
-		invalid_values: [ "red none", "red inherit", "red, green" ]
+		invalid_values: [ "red none", "red inherit", "red, green", "none red", "inherit red" ]
 	},
 	"-moz-border-start": {
 		domProp: "MozBorderStart",
@@ -277,7 +277,7 @@ var gCSSProperties = {
 		type: CSS_TYPE_LONGHAND,
 		initial_values: [ "none" ],
 		other_values: [ "red green", "red #fc3", "#ff00cc", "currentColor", "blue currentColor orange currentColor" ],
-		invalid_values: [ "red none", "red inherit", "red, green" ]
+		invalid_values: [ "red none", "red inherit", "red, green", "none red", "inherit red" ]
 	},
 	"-moz-box-align": {
 		domProp: "MozBoxAlign",

layout/style/test/test_inherit_computation.html

@@ -22,18 +22,6 @@
 
 /** Test for computation of CSS 'inherit' **/
 
-var gBrokenInherit = {
-  // Not implemented in nsRuleNode
-  "-moz-border-bottom-colors": true,
-  "-moz-border-left-colors": true,
-  "-moz-border-right-colors": true,
-  "-moz-border-top-colors": true,
-};
-
-function xfail_inherit(property, matching_initial) {
-  return property in gBrokenInherit;
-}
-
 // elements without a frame
 var gNParent = document.getElementById("nparent");
 var gNChild = document.getElementById("nchild");
@@ -86,23 +74,19 @@ function test_property(property)
     gNChild.className="allother";
     var inherit_initial_computed_n = get_computed_value_node(gNChild, property);
     var inherit_initial_computed_f = get_computed_value_node(gFChild, property);
-    (xfail_inherit(property, true) ? todo_is : is)(
-       inherit_initial_computed_n, initial_computed_n,
+    is(inherit_initial_computed_n, initial_computed_n,
        "inherit should cause inheritance of initial value for '" +
        property + "'");
-    (xfail_inherit(property, true) ? todo_is : is)(
-       inherit_initial_computed_f, initial_computed_f,
+    is(inherit_initial_computed_f, initial_computed_f,
        "inherit should cause inheritance of initial value for '" +
        property + "'");
     gParentRuleTop.style.setProperty(property, info.other_values[0], "");
     var inherit_other_computed_n = get_computed_value_node(gNChild, property);
     var inherit_other_computed_f = get_computed_value_node(gFChild, property);
-    (xfail_inherit(property, false) ? todo_is : is)(
-       inherit_other_computed_n, other_computed_n,
+    is(inherit_other_computed_n, other_computed_n,
        "inherit should cause inheritance of other value for '" +
        property + "'");
-    (xfail_inherit(property, false) ? todo_is : is)(
-       inherit_other_computed_f, other_computed_f,
+    is(inherit_other_computed_f, other_computed_f,
        "inherit should cause inheritance of other value for '" +
        property + "'");
     gParentRuleTop.style.removeProperty(property);
@@ -125,24 +109,20 @@ function test_property(property)
     gChildRule2.style.setProperty(property, "inherit", "");
     var inherit_other_computed_n = get_computed_value_node(gNChild, property);
     var inherit_other_computed_f = get_computed_value_node(gFChild, property);
-    (xfail_inherit(property, false) ? todo_is : is)(
-       inherit_other_computed_n, other_computed_n,
+    is(inherit_other_computed_n, other_computed_n,
        "inherit should cause inheritance of other value for '" +
        property + "'");
-    (xfail_inherit(property, false) ? todo_is : is)(
-       inherit_other_computed_f, other_computed_f,
+    is(inherit_other_computed_f, other_computed_f,
        "inherit should cause inheritance of other value for '" +
        property + "'");
     gParentRuleTop.style.removeProperty(property);
     gChildRule1.style.setProperty(property, info.other_values[0], "");
     var inherit_initial_computed_n = get_computed_value_node(gNChild, property);
     var inherit_initial_computed_f = get_computed_value_node(gFChild, property);
-    (xfail_inherit(property, true) ? todo_is : is)(
-       inherit_initial_computed_n, initial_computed_n,
+    is(inherit_initial_computed_n, initial_computed_n,
        "inherit should cause inheritance of initial value for '" +
        property + "'");
-    (xfail_inherit(property, true) ? todo_is : is)(
-       inherit_initial_computed_f, initial_computed_f,
+    is(inherit_initial_computed_f, initial_computed_f,
        "inherit should cause inheritance of initial value for '" +
        property + "'");
     gParentRuleTop.style.removeProperty(property);

layout/style/test/test_property_syntax_errors.html

@@ -22,10 +22,6 @@
 var gDeclaration = document.getElementById("testnode").style;
 
 var gKnownFails = {
-  "-moz-border-bottom-colors": [ "red none", "red inherit" ],
-  "-moz-border-left-colors": [ "red none", "red inherit" ],
-  "-moz-border-right-colors": [ "red none", "red inherit" ],
-  "-moz-border-top-colors": [ "red none", "red inherit" ],
   "pitch-range": [ " -0.01", "100.2", "108", "-3" ],
   "richness": [ " -0.01", "100.2", "108", "-3" ],
   "stress": [ " -0.01", "100.2", "108", "-3" ],