Bug 444377 - preserve string form of color attributes; r=bzbarsky

content/base/src/nsAttrValue.cpp

@@ -384,23 +384,14 @@ nsAttrValue::ToString(nsAString& aResult) const
 
       break;
     }
+#ifdef DEBUG
     case eColor:
     {
-      nscolor v;
+      NS_NOTREACHED("color attribute without string data");
-      GetColorValue(v);
+      aResult.Truncate();
-      if (NS_GET_A(v) == 255) {
-        char buf[10];
-        PR_snprintf(buf, sizeof(buf), "#%02x%02x%02x",
-                    NS_GET_R(v), NS_GET_G(v), NS_GET_B(v));
-        CopyASCIItoUTF16(buf, aResult);
-      } else if (v == NS_RGBA(0,0,0,0)) {
-        aResult.AssignLiteral("transparent");
-      } else {
-        NS_NOTREACHED("translucent color attribute cannot be stringified");
-        aResult.Truncate();
-      }
       break;
     }
+#endif
     case eEnum:
     {
       PRInt16 val = GetEnumValue();
@@ -485,26 +476,13 @@ nsAttrValue::GetStringValue() const
 PRBool
 nsAttrValue::GetColorValue(nscolor& aColor) const
 {
-  NS_PRECONDITION(Type() == eColor || Type() == eString, "wrong type");
+  if (Type() != eColor) {
-  switch (Type()) {
+    // Unparseable value, treat as unset.
-    case eString:
+    NS_ASSERTION(Type() == eString, "unexpected type for color-valued attr");
-    {
+    return PR_FALSE;
-      return GetPtr() && NS_ColorNameToRGB(GetStringValue(), &aColor);
-    }
-    case eColor:
-    {
-      aColor = GetMiscContainer()->mColor;
-      
-      break;
-    }
-    default:
-    {
-      NS_NOTREACHED("unexpected basetype");
-      
-      break;
-    }
   }
 
+  aColor = GetMiscContainer()->mColor;
   return PR_TRUE;
 }
 
@@ -663,7 +641,7 @@ nsAttrValue::Equals(const nsAttrValue& aOther) const
   }
 
   PRBool needsStringComparison = PR_FALSE;
-  
+
   switch (thisCont->mType) {
     case eInteger:
     {
@@ -688,7 +666,10 @@ nsAttrValue::Equals(const nsAttrValue& aOther) const
     }
     case eColor:
     {
-      return thisCont->mColor == otherCont->mColor;
+      if (thisCont->mColor == otherCont->mColor) {
+        needsStringComparison = PR_TRUE;
+      }
+      break;
     }
     case eCSSStyleRule:
     {
@@ -1109,46 +1090,62 @@ nsAttrValue::ParseIntWithBounds(const nsAString& aString,
   return PR_TRUE;
 }
 
+void
+nsAttrValue::SetColorValue(nscolor aColor, const nsAString& aString)
+{
+  nsStringBuffer* buf = GetStringBuffer(aString);
+  if (!buf) {
+    return;
+  }
+
+  if (!EnsureEmptyMiscContainer()) {
+    buf->Release();
+    return;
+  }
+
+  MiscContainer* cont = GetMiscContainer();
+  cont->mColor = aColor;
+  cont->mType = eColor;
+
+  // Save the literal string we were passed for round-tripping.
+  cont->mStringBits = reinterpret_cast<PtrBits>(buf) | eStringBase;
+}
+
 PRBool
 nsAttrValue::ParseColor(const nsAString& aString, nsIDocument* aDocument)
 {
+  ResetIfSet();
+
   nsAutoString colorStr(aString);
   colorStr.CompressWhitespace(PR_TRUE, PR_TRUE);
   if (colorStr.IsEmpty()) {
-    Reset();
     return PR_FALSE;
   }
 
   nscolor color;
-  // No color names begin with a '#', but numerical colors do so
+  // No color names begin with a '#'; in standards mode, all acceptable
-  // it is a very common first char
+  // numeric colors do.
-  if ((colorStr.CharAt(0) != '#') && NS_ColorNameToRGB(colorStr, &color)) {
+  if (colorStr.First() == '#') {
-    SetTo(colorStr);
-    return PR_TRUE;
-  }
-
-  // Check if we are in compatibility mode
-  if (aDocument->GetCompatibilityMode() == eCompatibility_NavQuirks) {
-    NS_LooseHexToRGB(colorStr, &color);
-  }
-  else {
-    if (colorStr.First() != '#') {
-      Reset();
-      return PR_FALSE;
-    }
     colorStr.Cut(0, 1);
-    if (!NS_HexToRGB(colorStr, &color)) {
+    if (NS_HexToRGB(colorStr, &color)) {
-      Reset();
+      SetColorValue(color, aString);
-      return PR_FALSE;
+      return PR_TRUE;
+    }
+  } else {
+    if (NS_ColorNameToRGB(colorStr, &color)) {
+      SetColorValue(color, aString);
+      return PR_TRUE;
     }
   }
 
-  if (EnsureEmptyMiscContainer()) {
+  if (aDocument->GetCompatibilityMode() != eCompatibility_NavQuirks) {
-    MiscContainer* cont = GetMiscContainer();
+    return PR_FALSE;
-    cont->mColor = color;
-    cont->mType = eColor;
   }
 
+  // In compatibility mode, try LooseHexToRGB as a fallback for either
+  // of the above two possibilities.
+  NS_LooseHexToRGB(colorStr, &color);
+  SetColorValue(color, aString);
   return PR_TRUE;
 }
 

content/base/src/nsAttrValue.h

@@ -306,6 +306,7 @@ private:
   inline void SetPtrValueAndType(void* aValue, ValueBaseType aType);
   void SetIntValueAndType(PRInt32 aValue, ValueType aType,
                           const nsAString* aStringValue);
+  void SetColorValue(nscolor aColor, const nsAString& aString);
   void SetMiscAtomOrString(const nsAString* aValue);
   void ResetMiscAtomOrString();
   inline void ResetIfSet();

content/base/test/test_bug433533.html

@@ -214,16 +214,24 @@ is(bd.getAttribute("bgcolor"), "red", "Bgcolor attribute didn't store the origin
 is(bd.bgColor, "red", ".bgColor didn't return the right value!");
 
 bd.setAttribute("bgcolor", "  red  ");
-todo_is(bd.getAttribute("bgcolor"), "  red  ", "Bgcolor attribute didn't store the original value");
+is(bd.getAttribute("bgcolor"), "  red  ", "Bgcolor attribute didn't store the original value");
-todo_is(bd.bgColor, "  red  ", ".bgColor didn't return the right value!");
+is(bd.bgColor, "  red  ", ".bgColor didn't return the right value!");
 
 bd.setAttribute("bgcolor", "#ff0000");
 is(bd.getAttribute("bgcolor"), "#ff0000", "Bgcolor attribute didn't store the original value");
 is(bd.bgColor, "#ff0000", ".bgColor didn't return the right value!");
 
+bd.setAttribute("bgcolor", "#f00");
+is(bd.getAttribute("bgcolor"), "#f00", "Bgcolor attribute didn't store the original value");
+is(bd.bgColor, "#f00", ".bgColor didn't return the right value!");
+
 bd.setAttribute("bgcolor", "  #ff0000  ");
-todo_is(bd.getAttribute("bgcolor"), "  #ff0000  ", "Bgcolor attribute didn't store the original value");
+is(bd.getAttribute("bgcolor"), "  #ff0000  ", "Bgcolor attribute didn't store the original value");
-todo_is(bd.bgColor, "  #ff0000  ", ".bgColor didn't return the right value!");
+is(bd.bgColor, "  #ff0000  ", ".bgColor didn't return the right value!");
+
+bd.setAttribute("bgcolor", "nonsense(complete)");
+is(bd.getAttribute("bgcolor"), "nonsense(complete)", "Bgcolor attribute didn't store the original value");
+is(bd.bgColor, "nonsense(complete)", ".bgColor didn't return the right value!");
 
 // same test again setting the prop
 bd.bgColor = "red";
@@ -231,16 +239,42 @@ is(bd.getAttribute("bgcolor"), "red", "Bgcolor attribute didn't store the origin
 is(bd.bgColor, "red", ".bgColor didn't return the right value!");
 
 bd.bgColor = "  red  ";
-todo_is(bd.getAttribute("bgcolor"), "  red  ", "Bgcolor attribute didn't store the original value");
+is(bd.getAttribute("bgcolor"), "  red  ", "Bgcolor attribute didn't store the original value");
-todo_is(bd.bgColor, "  red  ", ".bgColor didn't return the right value!");
+is(bd.bgColor, "  red  ", ".bgColor didn't return the right value!");
 
 bd.bgColor = "#ff0000";
 is(bd.getAttribute("bgcolor"), "#ff0000", "Bgcolor attribute didn't store the original value");
 is(bd.bgColor, "#ff0000", ".bgColor didn't return the right value!");
 
+bd.bgColor = "#f00";
+is(bd.getAttribute("bgcolor"), "#f00", "Bgcolor attribute didn't store the original value");
+is(bd.bgColor, "#f00", ".bgColor didn't return the right value!");
+
 bd.bgColor = "  #ff0000  ";
-todo_is(bd.getAttribute("bgcolor"), "  #ff0000  ", "Bgcolor attribute didn't store the original value");
+is(bd.getAttribute("bgcolor"), "  #ff0000  ", "Bgcolor attribute didn't store the original value");
-todo_is(bd.bgColor, "  #ff0000  ", ".bgColor didn't return the right value!");
+is(bd.bgColor, "  #ff0000  ", ".bgColor didn't return the right value!");
+
+bd.bgColor = "nonsense(complete)";
+is(bd.getAttribute("bgcolor"), "nonsense(complete)", "Bgcolor attribute didn't store the original value");
+is(bd.bgColor, "nonsense(complete)", ".bgColor didn't return the right value!");
+
+// equal color, unequal string
+var f1 = document.createElement("font");
+var f2 = document.createElement("font");
+var f3 = document.createElement("font");
+f1.color = "#f00";
+f2.color = "#ff0000";
+f3.color = "red";
+isnot(f1.color, f2.color, "#f00 and #ff0000 should not compare equal");
+isnot(f1.color, f3.color, "#f00 and red should not compare equal");
+isnot(f2.color, f3.color, "#ff0000 and red should not compare equal");
+
+isnot(f1.getAttribute("color"), f2.getAttribute("color"),
+      "#f00 and #ff0000 should not compare equal [attr]");
+isnot(f1.getAttribute("color"), f3.getAttribute("color"),
+      "#f00 and red should not compare equal [attr]");
+isnot(f2.getAttribute("color"), f3.getAttribute("color"),
+      "#ff0000 and red should not compare equal [attr]");
 
 var video = document.createElement("video");
 video.setAttribute("playbackrate", "1");