Closed
Bug 493881
Opened 16 years ago
Closed 16 years ago
ignore attempts to set legacy color props on a HTML document object before its body element exists
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: zwol, Assigned: zwol)
References
Details
Attachments
(4 files, 2 obsolete files)
2.96 KB,
text/html
|
Details | |
5.30 KB,
patch
|
zwol
:
review+
zwol
:
superreview+
|
Details | Diff | Splinter Review |
4.23 KB,
patch
|
zwol
:
review+
|
Details | Diff | Splinter Review |
683 bytes,
patch
|
Details | Diff | Splinter Review |
We generally try to make the legacy color properties (bgColor, text, link, vLink, aLink) of the 'document' object forward to the analogous properties/attributes on the 'body' element, but if you set them from script within the <head> tag, there is no body element yet. There is fallback code that tries to stash these properties in a special style sheet stored in the document itself, but that is not implemented for all five properties and seems liable to confuse people no end (they set the property from <head>, then the real <body> shows up, so reading the property back returns nothing, but it's still effective...)
I'm making this depend on bug 488649 because I'd rather not try to fix it without the cleanups in that bug already in place.
Assignee | ||
Comment 1•16 years ago
|
||
see also bug 493882.
Assignee | ||
Comment 2•16 years ago
|
||
Here's a fairly complex but self-contained test case that characterizes browser behavior when the legacy color properties on the document object are set before there's a body element. I tested it in four browsers I have to hand; here are the results in increasing order of wackiness.
Opera 9.52: Before there is a body element, all five properties read as the empty string, and attempts to write them are silently ignored. After there is a body element, all five properties are normal string properties (exact text is preserved) and are honored in page rendering. Quirk: writing the undefined value into the properties sets them to the string "undefined".
IE7: All five properties can be set before there is a body element. Values set then are reported accurately both before and after the body element appears and are honored in page rendering (except 'alinkColor' -- IE doesn't seem to do the active-link distinction). However, they always read back as #rrggbb regardless of how set, and passing in a string that isn't parseable as a named color or as #rrggbb produces bizarre colors (e.g. " orange " becomes "#00a0e0"). Writing the undefined value into the properties has no effect.
Firefox trunk: The 'fgColor' and 'bgColor' properties read as the empty string before body exists, and writes to them are ignored. The other three initially read as the empty string, but *can* be written; if read back immediately after setting, they read as the #rrggbb form of whatever color was set, and the setting is honored for page rendering. However, once there is a body element, these properties again read as the empty string, even though the colors are being used for page rendering.
Once there is a body element, all five properties behave as normal string properties from Javascript's point of view, but do not preserve whitespace surrounding color names. As with Opera, writing the undefined value sets to "undefined". For link/vlink/alinkColor but not fg/bgColor, writing the undefined value DOES NOT cancel the most recent previous setting for purposes of page rendering, even though it does change the value for Javascript inspection.
Firefox 3.0: As trunk, with additional quirks: if any of the properties is successfully set to a color name it will read back as the #rrggbb form of that color. From script in <head> link/vlink/alinkColor read as '#000000' before first set, whereas fg/bgColor read as the empty string. From script in body.onload, all five properties first read as the #rrggbb form of the *system default* color for this purpose -- not whatever was set previously, or anything specified by CSS.
Assignee: nobody → zweinberg
Comment 3•16 years ago
|
||
I expect the spec to say exactly what Opera does. I'll fix the spec in a few weeks hopefully. Let me know if what Opera does causes compatibility problems.
Assignee | ||
Comment 4•16 years ago
|
||
Apart from the behavior on "document.<prop> = undefined" I have no objection to Opera's behavior.
Comment 5•16 years ago
|
||
I'm probably fine with Opera's behavior here, though the lack of compat with IE in terms of what colors are used for rendering somewhat worries me....
Assignee | ||
Comment 6•16 years ago
|
||
No change from IE7 to IE8. But we're already incompatible with IE here, and I doubt assignments to these legacy props from script in <head> (as opposed to, say, onload handlers) are common.
Comment 7•16 years ago
|
||
Right; we're not DOM-compatible with IE, but we're rendering-compatible, right?
Bob, do you think you might be able to do a spider run for pages that set these properties while document.body is null? Or can spider not condition on such things easily?
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #7)
> Right; we're not DOM-compatible with IE, but we're rendering-compatible, right?
We're only partially rendering-compatible; we don't honor fgColor and bgColor before the body exists.
On another note, is it possible to set pseudoclass styles such as ":link { color: #rrggbb}" from a MapAttributesIntoRule function? If not, we appear to be stuck with the special treatment of A tags in nsHTMLStyleSheet.
Comment 9•16 years ago
|
||
(In reply to comment #7)
> Bob, do you think you might be able to do a spider run for pages that set these
> properties while document.body is null? Or can spider not condition on such
> things easily?
I'm not sure. My first thought is to defined a setter that would do the check but I don't know how to do that before the page loads.
Assignee | ||
Comment 10•16 years ago
|
||
Can you inject a <script> as the first element of <head>?
Assignee | ||
Comment 11•16 years ago
|
||
This made it all the way through the try server, as far as I can tell (mochitests timed out in the video tests on Mac and Linux). Builds are at
http://build.mozilla.org/tryserver-builds/zweinberg@mozilla.com-legacycolorprops
Attachment #378984 -
Flags: review?(bzbarsky)
Updated•16 years ago
|
Attachment #378984 -
Flags: superreview+
Attachment #378984 -
Flags: review?(bzbarsky)
Attachment #378984 -
Flags: review+
Assignee | ||
Comment 12•16 years ago
|
||
Patch will need rediffing before landing. Please stand by.
Assignee | ||
Comment 13•16 years ago
|
||
Rediffed.
Attachment #378984 -
Attachment is obsolete: true
Attachment #381178 -
Flags: superreview+
Attachment #381178 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: trunk only]
Assignee | ||
Comment 14•16 years ago
|
||
Per http://html5.org/tools/web-apps-tracker?from=3129&to=3130 HTML5 now specifies the behavior implemented by the latest patch.
Summary: weird things happen if you set a legacy color prop on the document object before the body element exists → ignore attempts to set legacy color props on a HTML document object before its body element exists
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment 15•16 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/31f1a4b3038f
Just realized this still needs tests.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: trunk only]
Comment 16•16 years ago
|
||
Attachment #383774 -
Flags: review?(zweinberg)
Assignee | ||
Comment 17•16 years ago
|
||
Comment on attachment 383774 [details] [diff] [review]
mochitest test case
r=zwol
The test logic itself looks good. It would be nice to also verify that setting the legacy color props to "undefined" causes them to have no effect on rendering (getComputedStyle checks are fine).
Also, some nitpicks with the style:
It's not obvious to a casual observer that addLoadEvent functions are called in the order they are registered, so I would prefer that there be just one addLoadEvent function that does all of the tests in sequence. This would also remove some of the visual clutter around the test cases.
Please move setAndTestProperty above where it is used.
Please make sure that the files as checked in are not missing their final newlines.
Attachment #383774 -
Flags: review?(zweinberg) → review+
Comment 18•16 years ago
|
||
updated per Zack's review comments.
Attachment #383774 -
Attachment is obsolete: true
Attachment #383785 -
Flags: review?(zweinberg)
Assignee | ||
Updated•16 years ago
|
Attachment #383785 -
Flags: review?(zweinberg) → review+
Assignee | ||
Comment 19•16 years ago
|
||
Comment on attachment 383785 [details] [diff] [review]
mochitest test case
Looks good.
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
Attachment #383785 -
Attachment description: mochitest test case → mochitest test case (checkin-needed)
Comment 20•16 years ago
|
||
pushed the test: http://hg.mozilla.org/mozilla-central/rev/5d2cb7932409
... but it seems that the test won't be run, because it hasn't been added to the makefile, right?
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 21•16 years ago
|
||
> ... but it seems that the test won't be run, because it hasn't been added to
> the makefile, right?
Doh. I should have caught that. Can you push the obvious fix, Markus?
Comment 22•16 years ago
|
||
Oops...here's the Makefile change for the mochitest, which needs to be checked in.
Updated•16 years ago
|
Attachment #383785 -
Attachment description: mochitest test case (checkin-needed) → mochitest test case
Comment 23•16 years ago
|
||
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•