Closed Bug 488649 Opened 15 years ago Closed 15 years ago

window.onload="document.body.bgColor = document.body.bgColor" makes the background color black

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: ittay.dror, Assigned: zwol)

References

(
URL
)

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(3 files, 5 obsolete files)

Testcase
358 bytes, text/html
Details
body properties readout
4.28 KB, text/html
Details
rev 5a: with useful checkin comment
11.44 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090330 Ubuntu/8.10 (intrepid) Shiretoko/3.5b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090330 Ubuntu/8.10 (intrepid) Shiretoko/3.5b4pre

the page has white background in firefox 3.0.7 and black in 3.5b4pre, making it unreadable. 

Reproducible: Always

Steps to Reproduce:
1. load the page
Actual Results:  
background color is black

Expected Results:  
background color should be white
Version: unspecified → 3.1 Branch
Version: 3.1 Branch → unspecified
Confirmed on Windows XP. Formerly it showed the "no background color specified" color.
Regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-12+07%3A00%3A00&enddate=2008-09-12+10%3A00%3A00
which points to bug 453566.
Blocks: 453566
Component: General → Layout
Keywords: regression
OS: Linux → All
Product: Firefox → Core
QA Contact: general → layout
Hardware: x86 → All
Version: unspecified → Trunk
Attached file Testcase — 
Looks like it all boils down to <body onload="document.body.bgColor=document.body.bgColor">. This is obviously a no-op, but for some reason, it sets the background color to black. I suspect that document.body.bgColor gets evaluated before it is initialized to #ffffff.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Summary: background color is wrong → window.onload="document.body.bgColor = document.body.bgColor" makes the background color black
If there's no bgcolor= attribute on the <body> element, nsHTMLBodyElement::GetBgColor() retrieves the background color specified in CSS.  As there is none, we get the CSS default,  which is *not* white, but transparent, represented internally as rgba(0,0,0,0).  But GetBgColor() passes that to NS_RGBToHex(), which ignores transparency, so the color that comes out is #000000 - black.  The "no background color specified" color does not get involved here.

The obvious fix would be to use NS_RGBAToHex() instead, but there is no such function.  Shall I add one?  I suppose it might break old JS that expects exactly six hex digits after the #...

(I've got an appointment & won't be reading bugmail till after lunch.)
[ Minor clarification: by 'the "no background color specified" color' I'm talking about the color in pref browser.display.background_color, which is not the same thing as the CSS default box background color. ]
Note that per HTML5 bgColor is just a shorthand for getting and setting the content attribute. E.g. getting it would give you the empty string if the attribute is not specified.
Yeah, we should do the HTML5 thing here.
Attached patch candidate patch (obsolete) — — Splinter Review 
> Yeah, we should do the HTML5 thing here.

That's easy; just delete most of GetBgColor :-)  Candidate patch attached, passes reftests & content mochitests with one adjustment to a test that was specifically looking for the old behavior.  I did not run all of the mochitests, but I've posted this to the try server and we'll see what comes out.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #373200 - Flags: review?(roc)
However, we might want to audit all uses of NS_RGBToHex() for similar bugs.
Continuing that thought, http://mxr.mozilla.org/mozilla-central/ident?i=NS_RGBToHex says that there are hardly any uses of NS_RGBToHex() and it looks to me like they're all broken in the presence of transparency.  Assuming HTML5 says the same thing about the other <BODY> presentational attributes that it does about 'bgcolor', everything in nsHTMLBodyElement.cpp can get the same treatment that I just applied to bgcolor.  nsHTMLDocument is a little trickier because it's got this mAttrStyleSheet thing which looks vestigial to me, but if not for that, where _do_ we put the presentational attributes when they're set on a document that has no <body> element yet?  And then there's nsAttrValue::ToString() which should maybe just start returning #rrggbbaa instead of #rrggbb.  Thoughts?
Comment on attachment 373200 [details] [diff] [review]
candidate patch

-td.setAttribute("colspan", "100k");
-is(td.getAttribute("colspan"), "100k", "Colspan attribute didn't store the original value");

Why are you removing this?

You can probably just use NS_IMPL_STRING_ATTR now.

I can't really review this myself.
Can you check IE to see if its document.body.bgColor just reflects the attribute value? If it does, then we almost certainly don't need to worry about Web compat here, otherwise we do.

If that looks OK then we should go ahead and give the other <body> attributes the same treatment. Not sure about the document attributes.
Well I guess there's probably no reason to not make the document DOM attributes reflect the content attributes too. mAttrStyleSheet shouldn't affect that.
(In reply to comment #10)
> (From update of attachment 373200 [details] [diff] [review])
> -td.setAttribute("colspan", "100k");
> -is(td.getAttribute("colspan"), "100k", "Colspan attribute didn't store the
> original value");
> 
> Why are you removing this?

It's an exact duplicate of a subtest elsewhere in the file - if you read the whole file, it looks like a clear copy-and-paste error to me.

> You can probably just use NS_IMPL_STRING_ATTR now.

Will do.

> Can you check IE to see if its document.body.bgColor just reflects the
> attribute value? If it does, then we almost certainly don't need to worry
> about Web compat here, otherwise we do.

It's Complicated.  I will attach a test case and discuss the results.

> Well I guess there's probably no reason to not make the document DOM
> attributes reflect the content attributes too. mAttrStyleSheet shouldn't
> affect that.

It's that mAttrStyleSheet is (I think) the *only* place where the document DOM stores the attributes if there's no body element to hang them off.
(In reply to comment #9)
> And then there's nsAttrValue::ToString() which
> should maybe just start returning #rrggbbaa instead of #rrggbb.

We should probably return something that's a valid CSS color, i.e., rgba() functions, with the alpha to 3 decimal places.  (I think we have code to do that serialization somewhere already.)
> It's Complicated.  I will attach a test case and discuss the results.

Here's the test case.  I'm not sure it's actually doing what it's supposed to, because there *was* a mochitest trying to make sure that if you set body.bgcolor to 'red' it read back out as '#ff0000', but that is not the behavior of this test.

FF3.0, trunk, and IE8 all exhibit this behavior: the _attributes_ 'vlink' 'alink' 'bgcolor' are maintained as distinct quantities from the _properties_ of the same names, and both properties and attributes read back out in the form they were set (I tested '', 'red', and '#ff0000').  'text' and 'link' are different: the attribute and the property maintain the same value, but the attribute reflects the textual form that was set, and the property is always in the #rrggbb form.

IE7 is different: for 'vlink', 'alink', and 'bgcolor', setting the attribute sets the property instead, but the property always reads out in the form that was set.  For 'text' and 'link', attribute and property always have the same value, and if it's set, it reads back out in #rrggbb form.
What does IE7 return when you read the content attribute 'bgcolor'? Exactly what was set? If so, then I think we can conclude that for 'bgcolor' at least HTML5 agrees with IE7 and we're good to go here.
> What does IE7 return when you read the content attribute 'bgcolor'? Exactly
> what was set?

Yup, perhaps with whitespace trimmed.
Ack, no, I take that back.  IE8 does that.  IE7 body.getAttribute("bgcolor") always returns null, even if you just set it.
FWIW try server unit tests are green on linux - <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1239918945.1239925581.16452.gz>.  Builds are here: <https://build.mozilla.org/tryserver-builds/2009-04-16_14:29-zweinberg@mozilla.com-1239917293/> - there's no windows build yet.  I don't see unit test results for mac or windows but I doubt there would be platform variation in this.
This is a straightforward extension of the original patch to all of the legacy color properties (bgcolor, text, link, alink, vlink) and their equivalents on the document object.  There is (I would argue) undesirable behavior if the document-object properties are set when there is no body element (e.g. from script in <head>, or for non-HTML) but it's out of scope for this bug, and messy to fix anyway.

Reftests are good; throwing at the try server now.
Attachment #373200 - Attachment is obsolete: true
Attachment #373929 - Flags: review?
Attachment #373200 - Flags: review?(jst)
Attachment #373929 - Flags: review? → review?(jst)
With the patch as posted there are 16 mochitest failures:

*** 12162 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug433533.html | Bgcolor attribute didn't store the original value - got "rgb(255, 0, 0)", expected "#ff0000"
*** 12163 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug433533.html | .bgColor didn't return the right value! - got "rgb(255, 0, 0)", expected "#ff0000"
*** 37279 ERROR TEST-UNEXPECTED-FAIL | aLinkLink | got "rgb(0, 0, 255)", expected "#0000ff"
*** 37289 ERROR TEST-UNEXPECTED-FAIL | bgColorLink | got "rgb(255, 255, 0)", expected "#ffff00"
*** 37294 ERROR TEST-UNEXPECTED-FAIL | linkLink | got "rgb(255, 0, 0)", expected "#ff0000"
*** 37299 ERROR TEST-UNEXPECTED-FAIL | textLink | got "rgb(0, 0, 0)", expected "#000000"
*** 37304 ERROR TEST-UNEXPECTED-FAIL | vLinkLink | got "rgb(0, 255, 255)", expected "#00ffff"
*** 38252 ERROR TEST-UNEXPECTED-FAIL | colorLink | got "rgb(0, 0, 0)", expected "#000000"
*** 39279 ERROR TEST-UNEXPECTED-FAIL | bgColorLink | got "rgb(0, 255, 255)", expected "#00ffff"
*** 39284 ERROR TEST-UNEXPECTED-FAIL | bgColorLink | got "rgb(255, 0, 0)", expected "#ff0000"
*** 39507 ERROR TEST-UNEXPECTED-FAIL | bgColorLink | got "rgb(255, 0, 0)", expected "#ff0000"
*** 39706 ERROR TEST-UNEXPECTED-FAIL | bgColorLink | got "rgb(0, 255, 255)", expected "#00ffff"
*** 40109 ERROR TEST-UNEXPECTED-FAIL | aLinkLink | got "rgb(0, 0, 255)", expected "#0000ff"
*** 40340 ERROR TEST-UNEXPECTED-FAIL | bgcolorLink | got "rgb(255, 0, 0)", expected "#ff0000"
*** 40405 ERROR TEST-UNEXPECTED-FAIL | bgcolorLink | got "rgb(255, 0, 0)", expected "#ff0000"
*** 40445 ERROR TEST-UNEXPECTED-FAIL | bgcolorLink | got "rgb(0, 255, 255)", expected "#00ffff"

These are all problems with the round-tripping of the attribute: if the user put in <body alink="#0000ff"> it's reasonable to expect that they get "#0000ff" back out from DOM queries, but we aren't.  I presume that this is because I changed nsAttrValue::ToString() to use CSS color syntax unconditionally, but I'm not sure why this is affecting tests with the #rrggbb syntax and not tests with color keywords ("red" e.g.) so I don't know how to fix it.
Why are we normalizing these color attributes? Isn't it easy to just store them as normal attribute strings?
(In reply to comment #22)
> Why are we normalizing these color attributes? Isn't it easy to just store them
> as normal attribute strings?

I would think it would be easy, but I don't know where the code is that is doing this normalization.  (Would dig, but am head down in other stuff right now.)
This is causing a regression on some sites, nominating for blocking; not a major issue (as we haven't had a ton of reports) but it will be felt.
Flags: blocking1.9.1?
This revision fixes all the failing mochitests and also knocks off two todos in the same file (corresponding to bug 444377).  There is no longer a special "color" value type for content attributes; they are just stored as strings until something calls GetColorValue(), at which point the string is handed to the CSS parser.  Thus, in standards mode the value syntax of a color attribute is now identical to the value syntax of a CSS color property; in particular, rgb/rgba/hsl/hsla() notation are acceptable.  (This may be an extension relative to html5.)

ParseColor() still exists, but only to handle quirks mode, where hex colors have a much looser syntax.

I hope creating a new CSS parser object for every call to nsStyleUtil::ParseCSSColor is not an overhead problem.

Reftests and content/base mochitests are green; bouncing to try server for complete test run.
Attachment #373929 - Attachment is obsolete: true
Attachment #377238 - Flags: review?(jst)
Attachment #373929 - Flags: review?(jst)
Blocks: 444377
https://build.mozilla.org/tryserver-builds/2009-05-13_13:20-zweinberg@mozilla.com-bug488649/

All unit tests green except for one mochitest failure on Windows: 

*** 28003 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_ended2.html | Expect at least one playing event

which does not seem like it can be related.  Logs are:

Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1242246020.1242251805.31696.gz
OSX: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1242246020.1242253226.1661.gz
Windows: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1242246020.1242254696.4063.gz

Try server does not appear to be completing talos runs.
> I hope creating a new CSS parser object for every call to
> nsStyleUtil::ParseCSSColor is not an overhead problem.

I'd suspect ti almost certainly is.  Further, just parsing on every GetColorValue is likely to be an overhead problem.  It'll happen on every single style reresolve for the node in question, and worse yet differences in the color string will prevent sharing of mapped attributes that could otherwise be shared...  This last we might have to just live with if we really really need to round-trip here.  :(

Then again, do we actually really really need to?  Or can that wait until we figure out a way to make mapped attrs deal with this situation better?
One of the affected sites is: https://ar.eer.ee/index.py?lang=eng

This is the Eestonia's online Commercial Register. It's used thousend times daily by bussiness and goverment.
I guess I would be ok with just doing the parsing "every time" for the body's attributes. That is, change nsHTMLBodyElement::ParseAttribute to not ParseColor on those attributes and do the parsing in the mapping function instead.  But I don't think we want to mess with things like <font color=""> as this last patch does, at this stage in the game.
An alternative that comes to mind is to parse to an nscolor in ParseColor, but do that via the CSS parser (we could then hang a CSS parser off the nsDocument, if there isn't already one that can be had from there, and avoid the overhead of creating one every time) and also keep the string around for round-tripping.  I'm not sure what you mean by "differences in the color string will prevent sharing of mapped attributes" though -- if we do keep the string around for round-tripping, can we still share based on nscolor equivalence?  (Presumably not if it's the nsAttrValue that gets shared, but I get the impression you are talking about the style rules that arise from various calls to MapAttributesIntoRule functions ... except then I don't see why they can't be shared with the current code.)

Another factor to consider is that HTML5 doesn't permit CSS color functions in "legacy color properties", and that would be a dandy excuse not to bother with calling into nsCSSParser at all.  Unfortunately, I currently don't see any way to avoid that but still get round-tripping *of the color itself* in the presence of partial transparency and Javascript -- consider

  <style>body { background-color: rgba(255,0,0,0.5); }</style>
  <body onload="document.body.bgColor = getComputedStyle(document.body, "").backgroundColor;"> ... </body>

which seems like a thing that authors might expect to work.
> we could then hang a CSS parser off the nsDocument

We already do, via nsCSSLoader's recycled parser list.

> (Presumably not if it's the nsAttrValue that gets shared, but I get the
> impression you are talking about the style rules that arise from various calls
> to MapAttributesIntoRule functions ... except then I don't see why they can't
> be shared with the current code.)

The style rule and the list of mapped nsAttrValues are the same object, at the moment.  See nsMappedAttributes.  So if you're sharing one you're sharing the other.  As I said, we could probably rearchitect mapped attrs to work somehow differently, but that's not a "do by wednesday" project, I suspect.

> which seems like a thing that authors might expect to work.

If it doesn't work in any other UA, then I don't think we should be making it work in ours, honestly.  We're not exactly trying to encourage use of bgColor here, just fix this bug, right?
So here's another revision that uses the string slot in MiscContainer to achieve 100% round-tripping of whatever junk the page put into the attribute, while still doing all the parsing in ParseColor (which is tidier anyway, 'cos the document is right there).  Also, I went back to the old syntax for acceptable color values (#rrggbb, a named color, or, in quirks mode only, anything NS_LooseHexToRGB can make sense of).  It's easy to switch to CSS <color> if we want to do that in the future, now the parsing is again being done in a function that can get at the document object.

Reviewer(s): Would it be safe to rip out the fallback code in nsAttrValue::ToString (look for the "This shouldn't happen but we implement it just in case" comment)?  Also, why do I see a bare, nonempty eString value in GetColorValue() if I set the bgcolor property to an unparseable value from Javascript?

(In reply to comment #32)
> The style rule and the list of mapped nsAttrValues are the same object, at the
> moment.  See nsMappedAttributes.  So if you're sharing one you're sharing the
> other.  As I said, we could probably rearchitect mapped attrs to work somehow
> differently, but that's not a "do by wednesday" project, I suspect.

So I think this new revision will share just as much as it used to, but I'm still not sure I understand how the sharing works; please check.

> If it doesn't work in any other UA, then I don't think we should be making it
> work in ours, honestly.  We're not exactly trying to encourage use of bgColor
> here, just fix this bug, right?

Right.  I checked that it doesn't work in FF3.0 and that's good enough for now, really - not a regression.
Attachment #377238 - Attachment is obsolete: true
Attachment #378189 - Flags: review?(bzbarsky)
Attachment #377238 - Flags: review?(jst)
That patch changes behavior for cases when the color can't be parsed, doesn't it?  For example, say I have:

  <font color="something unparseable">text</font>

If I read the patch right, GetColorValue() will now always return true, and in this case will return true while setting the color to transparent.  So we'll use the transparent color here instead of the current behavior of ignoring the @color altogether.

> Also, why do I see a bare, nonempty eString value in GetColorValue()

Because ParseColorValue() returned false?

> So I think this new revision will share just as much as it used to

I believe so, as written, but that comes at the cost of round-tripping issues (which I think is fine).  For example:

  <font color="#ffffff">one</font>
  <font color="  #FFFFFF">two</font>

will share the attr value, and hence will have the same value for getAttribute("color").  Which it is will depend on the exact order of the attribute sets.

I'm still wondering: at this stage in the game, is there a reason we're trying to do more than just stop returning various computed-style like things for bgColor and just returning our attr value, in the same serialization as now, without changing anything else?  Is that not compatible enough with what IE does?
(In reply to comment #34)
> That patch changes behavior for cases when the color can't be parsed, doesn't
> it?

It is not intended to.

> For example, say I have:
> 
>   <font color="something unparseable">text</font>
> 
> If I read the patch right, GetColorValue() will now always return true, and in
> this case will return true while setting the color to transparent. So we'll
> use the transparent color here instead of the current behavior of ignoring the
> @color altogether.

That would be a mistake on my part; I had the impression that GetColorValue always needed to return *some* color.

> > Also, why do I see a bare, nonempty eString value in GetColorValue()
> Because ParseColorValue() returned false?

Oh, so if Parse* return false, the attribute is saved as a string?  I was expecting it either to be dropped or saved the way the Parse* function set it up.  This actually means I can simplify ParseColor a bit, I think.

> > So I think this new revision will share just as much as it used to
> 
> I believe so, as written, but that comes at the cost of round-tripping issues
> (which I think is fine).  For example:
> 
>   <font color="#ffffff">one</font>
>   <font color="  #FFFFFF">two</font>
> 
> will share the attr value, and hence will have the same value for
> getAttribute("color").  Which it is will depend on the exact order of the
> attribute sets.

Yeah, I think we can live with that.

> I'm still wondering: at this stage in the game, is there a reason we're trying
> to do more than just stop returning various computed-style like things for
> bgColor and just returning our attr value, in the same serialization as now,
> without changing anything else?  Is that not compatible enough with what IE
> does?

To the extent that I am doing anything more than that, blame html5 :-/
I don't think that last question was rhetorical ... why don't we just return the attribute value and forget about the rest, at least for 1.9.1?
(In reply to comment #36)
> I don't think that last question was rhetorical ... why don't we just return
> the attribute value and forget about the rest, at least for 1.9.1?

My answer was poor.  I believe that 90% of this patch *is* "just return the attribute value" -- its size is because it's ripping out all kinds of code that used to do otherwise.  The other 10% makes JS properties on the 'body' object equivalent to the attributes, which is not strictly required to fix this bug but does remove still more code and brings us in line with what html5 says about those properties (as long as they're not set from JS on the document object before there *is* a body object).
> I had the impression that GetColorValue always needed to return *some* color

Only if it returns true.  If it returns false, that means the value is not a color, and the caller should not use it.

> Oh, so if Parse* return false, the attribute is saved as a string?

Correct.

> The other 10% makes JS properties on the 'body' object equivalent to the
> attributes, which is not strictly required to fix this bug but does remove
> still more code and brings us in line with what html5 says about those
> properties

I understand that.  It's also touching code that's used by all sorts of other color-related attributes, and that scares me at this stage in the game.  If it's needed, it's needed.  If it's not, I'd somewhat prefer we do that as a followup.  That is, take the safest patch we can that fixes this bug for now, and spin off other cleanup (which I'm fine with doing, generally), into a separate bug.
At the level of user visible behavior, the only intended changes are:

 - delete the fallback behavior of the legacy color properties which is
   causing them to report black when no value has been set (because the
   canonical internal value of "transparent" degrades to "black" when you
   ignore the alpha)
 - all color properties/attributes ignore certain ill-formed input strings
   rather than treating them as black, but preserve them for JS inspection
 - unify certain properties of the 'body' element with the corresponding
   attributes, as specified in html5

It seems to me that *not* modifying the behavior of all legacy-color props/attrs at once would be more code and would merely leave equivalent bugs in place for everything except bgcolor, so I don't want to do that.

I will look at splitting out the html5-driven unification, but again it may actually be more code to not do that, because it's very easy to make a property equivalent to an attribute (NS_IMPL_STRING_ATTR) but if you don't you have to find somewhere else to keep the data (the place where it's being kept now only holds an nscolor, which is part of the thing that needs changing to fix the bug).
I'm completely on board with the first item in the list of intended changes.

Item 2 should already be happening, if we're just using ParseColor on the nsAttrValue, no?  That is, if the input is malformed it should be parsed as string not color, which would cause it to be ignored for styling purposes and stored as a string.

Item 3 seems like the only one that would possibly involve changing the behavior of anything other than <body>'s attributes; I question whether it's desirable given that.

That said, HTML <font> already uses NS_IMPL_STRING_ATTR for @color; why does just using that for <body> without changing anything in the nsAttrValue code work?
(In reply to comment #40)
>
> That said, HTML <font> already uses NS_IMPL_STRING_ATTR for @color; why does
> just using that for <body> without changing anything in the nsAttrValue code
> work?

I did that first and got the test failures listed in comment #21.  nsAttrValue::ParseColor at present doesn't preserve the string; at a minimum I need to make it do that.  I think I can do that in a much less invasive way, though.  Stay tuned.
> I did that first and got the test failures listed in comment #21. 

Right, because you changed nsAttrValue::ToString in a way in which it probably shouldn't be changed.  Why was that change needed?

If the issue was that we just removed NS_RGBToHex, then I'm fine inlining it there, and asserting that alpha is 255 (which it should be, in that code, no).
Note that bug 444377 already covers trying to preserve initial values for color attrs; I think it's largely out of scope for this bug.
(In reply to comment #42)
> > I did that first and got the test failures listed in comment #21. 
> 
> Right, because you changed nsAttrValue::ToString in a way in which it probably
> shouldn't be changed.  Why was that change needed?
> 
> If the issue was that we just removed NS_RGBToHex, then I'm fine inlining it
> there, and asserting that alpha is 255 (which it should be, in that code, no).

Yes, it was just because we removed NS_RGBToHex.  I'll pull out the preserving-initial-values changes and see how that goes.
This needs a review, with a harsh eye towards the safest approach for us considering our timelines and comment 36 and comment 37
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch rev 5: minimize visible changes (obsolete) — — Splinter Review 
I have pulled out all the string round-tripping changes from this patch (I'll repost them as a separate patch in bug 444377).  The user visible changes from this patch should be:

 - document.body.{bgColor,text,link,vLink,aLink} are completely unified with the
   element attributes of the same names.  They do not look up CSS or prescontext
   defaults when queried but unset.
 - Any attempt to convert a non-opaque color to a JS string in #rrggbb notation
   now spits out a warning and returns "".

Also, NS_RGBToHex is still deleted, since it is not a safe API for general use.  (nsHTMLDocument.cpp has a private copy called LegacyRGBToHex because it needs that functionality in five different functions, to handle a corner case which I'm going to file a follow-up bug about.)

I believe this to be the minimal patch that fixes all five of the legacy color properties on <body>.
Attachment #378189 - Attachment is obsolete: true
Attachment #378468 - Flags: review?(bzbarsky)
Attachment #378189 - Flags: review?(bzbarsky)
This is exactly the same patch as "rev 5" but with a sensible checkin comment in the mq header.  Sorry for not catching that before.
Attachment #378468 - Attachment is obsolete: true
Attachment #378469 - Flags: review?(bzbarsky)
Attachment #378468 - Flags: review?(bzbarsky)
Blocks: 493881
Comment on attachment 378469 [details] [diff] [review]
rev 5a: with useful checkin comment

>@@ -387,8 +387,15 @@ nsAttrValue::ToString(nsAString& aResult
>+        NS_WARNING("non-opaque color attribute cannot be stringified");

That should be an assertion; colors with alpha can't happen here.  Same for the other callsite.  With that, looks great; it's nice to see all this code disappearing!
Attachment #378469 - Flags: superreview+
Attachment #378469 - Flags: review?(bzbarsky)
Attachment #378469 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/821405837446 with that change.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
And pushed http://hg.mozilla.org/mozilla-central/rev/9731a54fbb99 to fix the resulting red.
Whiteboard: [needs 191 landing] → [needs landing]
Whiteboard: [needs landing] → [needs 191 landing]
Depends on: 494225
verified FIXED on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre ID:20090527031500

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Depends on: 495543
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: