Bug 488649: Unify document.body.{bgColor,text,link,vLink,aLink} with the <body> attributes of the same names. Do not default to prescontext/CSS for these. Remove the unsafe function NS_RGBToHex().
authorZack Weinberg <zweinberg@mozilla.com>
Tue, 19 May 2009 22:11:01 -0400
changeset 28611 821405837446610ade88a2bdfc70b71677e58a05
parent 28610 401c215ae146e7612e7ff1df266f135b0d155ed2
child 28612 a71886cfb2e41e668ad2f9f495aed63bc380fa9e
push idunknown
push userunknown
push dateunknown
bugs488649
milestone1.9.2a1pre
Bug 488649: Unify document.body.{bgColor,text,link,vLink,aLink} with the <body> attributes of the same names. Do not default to prescontext/CSS for these. Remove the unsafe function NS_RGBToHex().
content/base/src/nsAttrValue.cpp
content/base/test/test_bug433533.html
content/html/content/src/nsHTMLBodyElement.cpp
content/html/document/src/nsHTMLDocument.cpp
gfx/public/nsColor.h
gfx/src/nsColor.cpp
layout/reftests/bugs/488649-1-ref.html
layout/reftests/bugs/488649-1.html
layout/reftests/bugs/reftest.list
--- a/content/base/src/nsAttrValue.cpp
+++ b/content/base/src/nsAttrValue.cpp
@@ -382,18 +382,25 @@ nsAttrValue::ToString(nsAString& aResult
       aResult = intStr;
 
       break;
     }
     case eColor:
     {
       nscolor v;
       GetColorValue(v);
-      NS_RGBToHex(v, aResult);
-
+      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 {
+        NS_NOTREACHED("non-opaque color attribute cannot be stringified");
+        aResult.Truncate();
+      }
       break;
     }
     case eEnum:
     {
       PRInt16 val = GetEnumValue();
       PRUint32 allEnumBits =
         cont ? cont->mEnumValue : static_cast<PRUint32>(GetIntInternal());
       const EnumTable* table = sEnumTableArray->
--- a/content/base/test/test_bug433533.html
+++ b/content/base/test/test_bug433533.html
@@ -206,26 +206,46 @@ is(iframe.getAttribute("marginwidth"), "
    "Marginwidth attribute didn't store the original value");
 iframe.setAttribute("marginwidth", "foobar");
 is(iframe.getAttribute("marginwidth"), "foobar",
    "Marginwidth attribute didn't store the original value");
 
 var bd = document.createElement("body");
 bd.setAttribute("bgcolor", "red");
 is(bd.getAttribute("bgcolor"), "red", "Bgcolor attribute didn't store the original value");
+is(bd.bgColor, "red", ".bgColor didn't return the right value!");
+
 bd.setAttribute("bgcolor", "  red  ");
-todo(bd.getAttribute("bgcolor") == "  red  ", "Bgcolor attribute didn't store the original value");
-td.setAttribute("colspan", "100k");
-is(td.getAttribute("colspan"), "100k", "Colspan attribute didn't store the original value");
-bd.setAttribute("bgcolor", "red");
+todo_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!");
+
+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", "  red  ");
+
+bd.setAttribute("bgcolor", "  #ff0000  ");
+todo_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!");
+
+// same test again setting the prop
+bd.bgColor = "red";
+is(bd.getAttribute("bgcolor"), "red", "Bgcolor attribute didn't store the original value");
+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");
+todo_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.setAttribute("bgcolor", "#ff0000");
-is(bd.bgColor, "#ff0000", ".bgColor didn't return the right value!");
+
+bd.bgColor = "  #ff0000  ";
+todo_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!");
 
 var video = document.createElement("video");
 video.setAttribute("playbackrate", "1");
 is(video.getAttribute('playbackrate'), "1",
    "Playbackrate attribute didn't store the original value");
 video.setAttribute("playbackrate", "1.5");
 is(video.getAttribute('playbackrate'), "1.5",
    "Playbackrate attribute didn't store the original value");
--- a/content/html/content/src/nsHTMLBodyElement.cpp
+++ b/content/html/content/src/nsHTMLBodyElement.cpp
@@ -305,142 +305,21 @@ NS_INTERFACE_TABLE_HEAD(nsHTMLBodyElemen
                                                nsGenericHTMLElement)
 NS_HTML_CONTENT_INTERFACE_TABLE_TAIL_CLASSINFO(HTMLBodyElement)
 
 NS_IMPL_ELEMENT_CLONE(nsHTMLBodyElement)
 
 
 NS_IMPL_URI_ATTR(nsHTMLBodyElement, Background, background)
 
-static nscolor
-GetDefaultColor(nsPresContext* aContext, nsIAtom* aAtom)
-{
-  if (aAtom == nsGkAtoms::vlink) {
-    return aContext->DefaultVisitedLinkColor();
-  } else if (aAtom == nsGkAtoms::alink) {
-    return aContext->DefaultActiveLinkColor();
-  } else if (aAtom == nsGkAtoms::link) {
-    return aContext->DefaultLinkColor();
-  } else if (aAtom == nsGkAtoms::text) {
-    return aContext->DefaultColor();
-  } 
-  NS_ERROR("Unhandled nsGkAtoms::attribute");
-  return NS_RGBA(0,0,0,0);
-}
-
-nsresult
-nsHTMLBodyElement::GetColorHelper(nsIAtom* aAtom, nsAString& aColor)
-{
-  aColor.Truncate();
-  nsAutoString color;
-  nscolor attrColor; 
-  if (!GetAttr(kNameSpaceID_None, aAtom, color)) {
-    nsPresContext *presContext = GetPresContext();
-    if (presContext) {
-      NS_RGBToHex(GetDefaultColor(presContext, aAtom), aColor);
-    }
-  } else if (NS_ColorNameToRGB(color, &attrColor)) {
-    NS_RGBToHex(attrColor, aColor);
-  } else {
-    aColor.Assign(color);
-  }
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsHTMLBodyElement::GetVLink(nsAString& aColor)
-{
-  return GetColorHelper(nsGkAtoms::vlink, aColor);
-}
-
-NS_IMETHODIMP
-nsHTMLBodyElement::SetVLink(const nsAString& aColor)
-{
-  return SetAttr(kNameSpaceID_None, nsGkAtoms::vlink, aColor,
-                 PR_TRUE);
-}
-
-NS_IMETHODIMP
-nsHTMLBodyElement::GetALink(nsAString& aColor)
-{
-  return GetColorHelper(nsGkAtoms::alink, aColor);
-}
-
-NS_IMETHODIMP
-nsHTMLBodyElement::SetALink(const nsAString& aColor)
-{
-  return SetAttr(kNameSpaceID_None, nsGkAtoms::alink, aColor,
-                 PR_TRUE);
-}
-
-NS_IMETHODIMP
-nsHTMLBodyElement::GetLink(nsAString& aColor)
-{
-  return GetColorHelper(nsGkAtoms::link, aColor);
-}
-
-NS_IMETHODIMP
-nsHTMLBodyElement::SetLink(const nsAString& aColor)
-{
-  return SetAttr(kNameSpaceID_None, nsGkAtoms::link, aColor,
-                 PR_TRUE);
-}
-
-// XXX Should text check the body frame's style struct for color,
-// like we do for bgColor?
-NS_IMETHODIMP
-nsHTMLBodyElement::GetText(nsAString& aColor)
-{
-  return GetColorHelper(nsGkAtoms::text, aColor);
-}
-
-NS_IMETHODIMP
-nsHTMLBodyElement::SetText(const nsAString& aColor)
-{
-  return SetAttr(kNameSpaceID_None, nsGkAtoms::text, aColor,
-                 PR_TRUE);
-}
-
-NS_IMETHODIMP 
-nsHTMLBodyElement::GetBgColor(nsAString& aBgColor)
-{
-  aBgColor.Truncate();
-
-  nsAutoString attr;
-  nscolor bgcolor;
-
-  // If we don't have an attribute, find the actual color used for
-  // (generally from the user agent style sheet) for compatibility
-  if (!GetAttr(kNameSpaceID_None, nsGkAtoms::bgcolor, attr)) {
-    // Make sure the style is up-to-date, since we need it
-    nsIFrame* frame = GetPrimaryFrame(Flush_Style);
-    
-    if (frame) {
-      bgcolor = frame->GetStyleBackground()->mBackgroundColor;
-      NS_RGBToHex(bgcolor, aBgColor);
-    }
-  }
-  else if (NS_ColorNameToRGB(attr, &bgcolor)) {
-    // If we have a color name which we can convert to an nscolor,
-    // then we should use the hex value instead of the color name.
-    NS_RGBToHex(bgcolor, aBgColor);
-  }
-  else {
-    // Otherwise, just assign whatever the attribute value is.
-    aBgColor.Assign(attr);
-  }
-
-  return NS_OK;
-}
-
-NS_IMETHODIMP 
-nsHTMLBodyElement::SetBgColor(const nsAString& aBgColor)
-{
-  return SetAttr(kNameSpaceID_None, nsGkAtoms::bgcolor, aBgColor, PR_TRUE); 
-}
+NS_IMPL_STRING_ATTR(nsHTMLBodyElement, VLink, vlink)
+NS_IMPL_STRING_ATTR(nsHTMLBodyElement, ALink, alink)
+NS_IMPL_STRING_ATTR(nsHTMLBodyElement, Link, link)
+NS_IMPL_STRING_ATTR(nsHTMLBodyElement, Text, text)
+NS_IMPL_STRING_ATTR(nsHTMLBodyElement, BgColor, bgcolor)
 
 PRBool
 nsHTMLBodyElement::ParseAttribute(PRInt32 aNamespaceID,
                                   nsIAtom* aAttribute,
                                   const nsAString& aValue,
                                   nsAttrValue& aResult)
 {
   if (aNamespaceID == kNameSpaceID_None) {
--- a/content/html/document/src/nsHTMLDocument.cpp
+++ b/content/html/document/src/nsHTMLDocument.cpp
@@ -2386,30 +2386,44 @@ NS_IMETHODIMP
 nsHTMLDocument::GetHeight(PRInt32* aHeight)
 {
   NS_ENSURE_ARG_POINTER(aHeight);
 
   PRInt32 width;
   return GetBodySize(&width, aHeight);
 }
 
+static void
+LegacyRGBToHex(nscolor aColor, nsAString& aResult)
+{
+  if (NS_GET_A(aColor) == 255) {
+    char buf[10];
+    PR_snprintf(buf, sizeof(buf), "#%02x%02x%02x",
+                NS_GET_R(aColor), NS_GET_G(aColor), NS_GET_B(aColor));
+    CopyASCIItoUTF16(buf, aResult);
+  } else {
+    NS_NOTREACHED("non-opaque color property cannot be stringified");
+    aResult.Truncate();
+  }
+}
+
 NS_IMETHODIMP
 nsHTMLDocument::GetAlinkColor(nsAString& aAlinkColor)
 {
   aAlinkColor.Truncate();
 
   nsCOMPtr<nsIDOMHTMLBodyElement> body = do_QueryInterface(GetBodyContent());
 
   if (body) {
     body->GetALink(aAlinkColor);
   } else if (mAttrStyleSheet) {
     nscolor color;
     nsresult rv = mAttrStyleSheet->GetActiveLinkColor(color);
     if (NS_SUCCEEDED(rv)) {
-      NS_RGBToHex(color, aAlinkColor);
+      LegacyRGBToHex(color, aAlinkColor);
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsHTMLDocument::SetAlinkColor(const nsAString& aAlinkColor)
@@ -2438,17 +2452,17 @@ nsHTMLDocument::GetLinkColor(nsAString& 
   nsCOMPtr<nsIDOMHTMLBodyElement> body = do_QueryInterface(GetBodyContent());
 
   if (body) {
     body->GetLink(aLinkColor);
   } else if (mAttrStyleSheet) {
     nscolor color;
     nsresult rv = mAttrStyleSheet->GetLinkColor(color);
     if (NS_SUCCEEDED(rv)) {
-      NS_RGBToHex(color, aLinkColor);
+      LegacyRGBToHex(color, aLinkColor);
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsHTMLDocument::SetLinkColor(const nsAString& aLinkColor)
@@ -2477,17 +2491,17 @@ nsHTMLDocument::GetVlinkColor(nsAString&
   nsCOMPtr<nsIDOMHTMLBodyElement> body = do_QueryInterface(GetBodyContent());
 
   if (body) {
     body->GetVLink(aVlinkColor);
   } else if (mAttrStyleSheet) {
     nscolor color;
     nsresult rv = mAttrStyleSheet->GetVisitedLinkColor(color);
     if (NS_SUCCEEDED(rv)) {
-      NS_RGBToHex(color, aVlinkColor);
+      LegacyRGBToHex(color, aVlinkColor);
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsHTMLDocument::SetVlinkColor(const nsAString& aVlinkColor)
--- a/gfx/public/nsColor.h
+++ b/gfx/public/nsColor.h
@@ -93,19 +93,18 @@ NS_GFX_(PRBool) NS_HexToRGB(const nsStri
 // with operator OVER.
 NS_GFX_(nscolor) NS_ComposeColors(nscolor aBG, nscolor aFG);
 
 // Translate a hex string to a color. Return true if it parses ok,
 // otherwise return false.
 // This version accepts 1 to 9 digits (missing digits are 0)
 NS_GFX_(PRBool) NS_LooseHexToRGB(const nsString& aBuf, nscolor* aResult);
 
-// Translate a color to a hex string and prepend a '#'.
-// The returned string is always 7 characters including a '#' character.
-NS_GFX_(void) NS_RGBToHex(nscolor aColor, nsAString& aResult);
+// There is no function to translate a color to a hex string, because
+// the hex-string syntax does not support transparency.
 
 // Translate a color name to a color. Return true if it parses ok,
 // otherwise return false.
 NS_GFX_(PRBool) NS_ColorNameToRGB(const nsAString& aBuf, nscolor* aResult);
 
 // function to convert from HSL color space to RGB color space
 // the float parameters are all expected to be in the range 0-1
 NS_GFX_(nscolor) NS_HSL2RGB(float h, float s, float l);
--- a/gfx/src/nsColor.cpp
+++ b/gfx/src/nsColor.cpp
@@ -196,24 +196,16 @@ NS_GFX_(PRBool) NS_LooseHexToRGB(const n
   else {
     if (nsnull != aResult) {
       *aResult = NS_RGB(0, 0, 0);
     }
   }
   return PR_TRUE;
 }
 
-NS_GFX_(void) NS_RGBToHex(nscolor aColor, nsAString& aResult)
-{
-  char buf[10];
-  PR_snprintf(buf, sizeof(buf), "#%02x%02x%02x",
-              NS_GET_R(aColor), NS_GET_G(aColor), NS_GET_B(aColor));
-  CopyASCIItoUTF16(buf, aResult);
-}
-
 NS_GFX_(PRBool) NS_ColorNameToRGB(const nsAString& aColorName, nscolor* aResult)
 {
   if (!gColorTable) return PR_FALSE;
 
   PRInt32 id = gColorTable->Lookup(aColorName);
   if (eColorName_UNKNOWN < id) {
     NS_ASSERTION(id < eColorName_COUNT, "LookupName mess up");
     if (aResult) {
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/488649-1-ref.html
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+ <head>
+  <title>Reference for bug 488649</title>
+ </head>
+ <body>
+  <div style="color: black; font-size: 36px;">PASS</div>
+ </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/488649-1.html
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+ <head>
+  <title>Testcase for bug 488649</title>
+ </head>
+ <body onload="document.body.bgColor=document.body.bgColor;">
+  <div style="color: black; font-size: 36px;">PASS</div>
+  <div style="color: white; font-size: 36px;">FAIL</div>
+ </body>
+</html>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1225,15 +1225,16 @@ fails-if(MOZ_WIDGET_TOOLKIT=="gtk2") == 
 == 486052-2c.html 486052-2-ref.html
 == 486052-2d.html 486052-2-ref.html
 == 486052-2e.html 486052-2-ref.html
 == 486052-2f.html 486052-2-ref.html
 == 486052-2g.html 486052-2-ref.html
 == 486848-1.xul 486848-1-ref.xul
 == 487539-1.html about:blank
 == 488390-1.html 488390-1-ref.html
+== 488649-1.html 488649-1-ref.html
+== 488685-1.html 488685-1-ref.html
 == 492661-1.html 492661-1-ref.html
-== 488685-1.html 488685-1-ref.html
 == 490182-1a.html 490182-1-ref.html
 == 490182-1b.html 490182-1-ref.html
 == 490173-1.html 490173-1-ref.html
 == 490173-2.html 490173-2-ref.html
 fails-if(MOZ_WIDGET_TOOLKIT!="cocoa") == 488692-1.html 488692-1-ref.html # needs EXTEND_PAD on non-Mac for correct scaling behaviour