Closed Bug 493881 Opened 15 years ago Closed 15 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)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: zwol, Assigned: zwol)

References

Details

Attachments

(4 files, 2 obsolete files)

test case
(deleted), text/html
Details
rediffed vs latest trunk
(deleted), patch
zwol
: review+
zwol
: superreview+
Details | Diff | Splinter Review
mochitest test case
(deleted), patch
zwol
: review+
Details | Diff | Splinter Review
Makefile change for mochitest
(deleted), 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.
see also bug 493882.
Attached file test case (deleted) — 
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
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.
Apart from the behavior on "document.<prop> = undefined" I have no objection to Opera's behavior.
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....
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.
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?
(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.
(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.
Can you inject a <script> as the first element of <head>?
Attached patch candidate patch (obsolete) (deleted) — — Splinter Review 
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)
Attachment #378984 - Flags: superreview+
Attachment #378984 - Flags: review?(bzbarsky)
Attachment #378984 - Flags: review+
Patch will need rediffing before landing.  Please stand by.
Attached patch rediffed vs latest trunk (deleted) — — Splinter Review 
Rediffed.
Attachment #378984 - Attachment is obsolete: true
Attachment #381178 - Flags: superreview+
Attachment #381178 - Flags: review+
Keywords: checkin-needed
Whiteboard: [c-n: trunk only]
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
Status: NEW → ASSIGNED
Pushed http://hg.mozilla.org/mozilla-central/rev/31f1a4b3038f

Just realized this still needs tests.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [c-n: trunk only]
Attached patch mochitest test case (obsolete) (deleted) — — Splinter Review 

        Attachment #383774 -
        Flags: review?(zweinberg)

    
    

    
  
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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        mochitest test case (deleted)
        — 
          — Splinter Review
      

    


  
updated per Zack's review comments.

        Attachment #383774 -
        Attachment is obsolete: true

        Attachment #383785 -
        Flags: review?(zweinberg)

    
  

        Attachment #383785 -
        Flags: review?(zweinberg) → review+

    
    

    
  
Comment on attachment 383785 [details] [diff] [review]
mochitest test case

Looks good.

        Attachment #383785 -
        Attachment description: mochitest test case → mochitest test case (checkin-needed)

    
    

    
  
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

    
    

    
  
> ... 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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Makefile change for mochitest (deleted)
        — 
          — Splinter Review
      

    


  
Oops...here's the Makefile change for the mochitest, which needs to be checked in.

        Attachment #383785 -
        Attachment description: mochitest test case (checkin-needed) → mochitest test case

    
    

    
  
Done.
http://hg.mozilla.org/mozilla-central/rev/9b9e37b3fda2
Flags: in-testsuite? → in-testsuite+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: