321195 - Light/dark motifs randomly work and change in Firefox 1.5

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Anthony Foiani]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

I recently upgraded to the latest Chatzilla, and I noticed that the dark motif wasn't working well: the background stayed white, and that made most of the text unreadable.

I eventually tracked it down to a bad CSS property in the motif files; both output-dark.css and output-light try to use a property called "background" for body.chatzilla-body.  When I changed it from "background" to "background-color", it started working as expected.

Reproducible: Always

Steps to Reproduce:
On OSX (at least), install:

1. latest FireFox release (1.5)
2. latest Chatzilla release (0.9.69.1)
3. switch to dark motif: /motif dark
Actual Results:  
Some text got colored grey (e.g., "[INFO]"), some got colored white (e.g., main text) but the background stayed the (default?) white.

Expected Results:  
Background should have gone to black

Fix to output-dark.css is as follows:

--- output-dark-orig.css        2005-12-21 22:29:41.000000000 -0800
+++ output-dark.css     2005-12-21 23:32:30.000000000 -0800
@@ -44,7 +44,7 @@
 @import url(chrome://chatzilla/content/output-base.css);
 
 body.chatzilla-body {               /* The topmost container in the ChatZilla */
-    background: black;              /* output window. */
+    background-color: black;        /* output window. */
     color: lightgrey;    
 }

Comment 1

[Anthony Foiani]

Similar fix for output-light.css:

--- output-light-orig.css       2005-12-21 23:35:46.000000000 -0800
+++ output-light.css    2005-12-21 23:36:07.000000000 -0800
@@ -44,7 +44,7 @@
 @import url(chrome://chatzilla/content/output-base.css);
 
 body.chatzilla-body {               /* The topmost container in the ChatZilla */
-    background: white;              /* output window.                         */
+    background-color: white;        /* output window.                         */
     color: #222222;    
 }
 

Comment 2

[:Gijs (he/him)]

First of all, I can't reproduce this immediately. However:

According to the CSS 2.1 spec, the use of:

body {
  background: black;
}

is perfectly valid. I'm betting that what you're actually seeing is a bug in Core which we're actively researching at the moment. It has to do with line numbers for css rules being totally screwed up by Gecko when using a css file on an html document in a browser with type="chrome".

Because the line numbers are sometimes wrong (when and why, we're still trying to figure out), the css rules get applied in the wrong order, resulting in bad colors or fonts in certain places. I'd be interested to see if this fixes things 100% for you (I'm personally assuming your problem will still occur, see above), and in some better steps to reproduce, because currently I can't.

Lastly, if it is really the case that the background property is broken, then this is a Core Layout bug, from what I can tell.

Comment 3

[Anthony Foiani]

(In reply to comment #2)

> I'm betting that what you're actually seeing is a bug in
> Core which we're actively researching at the moment. It has to do with line
> numbers for css rules being totally screwed up by Gecko when using a css file
> on an html document in a browser with type="chrome".

Sure enough; if I use the original CSS file as a separate file (and not within chrome), it works as expected.

Could you add a pointer to the bug(s) where the above issue is being discussed, then mark this one as invalid?  Thanks!

Comment 4

[James Ross]

Morphing this bug into one to track the general CSS problem that is occuring.

Comment 5

[James Ross]

One thing we found out last night is that this problem stops if you disable the XUL Cache...

Comment 6

[James Ross]

Alright, thanks to some good debugging by Hannibal and tH last night, and some random guessed changes to code, we have the bug nailed down and fixed.

*Why* it affects only some bits of style information, only in ChatZilla's type=chrome window, and only with XUL Cache enabled is still a mystery.

Alright, on to the bug. In nsCSSStyleRule.cpp there are two clone functions, neither of which work properly - in each case, they omit one key item. One of these clone functions is nsCSSSelectorList::Clone, which when fixed stops the random motif effects:
  http://lxr.mozilla.org/seamonkey/source/layout/style/nsCSSStyleRule.cpp#788

What's the bug there? Well, the field "mWeight" is not copied across to the new nsCSSSelectorList objects, which means that (thanks to a distinct lack of initialisation code) the vlaue ends up totally random.

The other one, CSSStyleRuleImpl::Clone (which defers to a copy ctor for the actual cloning), affects the display of the rule line numbers in DOMI (only place that actually uses them directly, it seems). Fixing this has no effect on the Gecko display, but does fix the display in DOMI, which is clearly good:
  http://lxr.mozilla.org/seamonkey/source/layout/style/nsCSSStyleRule.cpp#1271

Here, the copy ctor fails to copy the field "mLineNumber", which has similar effects to the first issue - random line numbers appearing in DOMI.

I have a patch coming up that fixes both the missing copy code, and adds some initilisation for both these fields, which seem to be lacking it rather a lot.

Comment 7

[James Ross]

Attachment: Clone mWeight and mLineNumber

This is the patch I'm currently running with, and fixes the problem here (and a believe Hannibal and tH tried basically the same changes towards the end of our debugging).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Silver, want to make the initializer order match member declaration order? With that, the patch looks great.

One reason this may only be biting us in some cases is that cloning sheets is not something that happens much; it only happens if the same sheet is used multiple times on one page and for UA and chrome sheets.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #8)
> Silver, want to make the initializer order match member declaration order? With
> that, the patch looks great.

Which just means putting the mWeight initialization before mNext.  r+sr=dbaron with that.

Comment 10

[James Ross]

Attachment: Updated order of init

(carrying forward r+sr from dbaron)

Comment 11

[James Ross]

Checked in --> FIXED.

Thanks bz and dbaron for making this so quick. :)

I believe this is suitable for inclusion on branch, in 1.8.1 (would be great to be picked up by the next Firefox update from branch, as this seriously impacts usability of the built-in motifs in ChatZilla) once it has spent a while in trunk baking. I will request approval next week, unless anyone has a better idea.

Comment 12

[:Gijs (he/him)]

Comment on attachment 206714 [details] [diff] [review]
Updated order of init

I am requesting approval for 1.8.0.1 as well. I'm doing so because I've seen various other bugs that are non-security issues being added to this release, and I would really appreciate having this fix before Firefox 2.0.
Per http://developer.mozilla.org/devnews/index.php/2005/12/16/whats-next/ :
* There have been no regressions (to my knowledge), and the patch has been on trunk for more than 2 weeks.
* Summary of changes:
 + Initialize the weight of nsCSSSelectorList and line number of nsCSSStyleRule with 0, and make sure these are copied properly when cloning the Rule or List.
 + There is no risk of any web content breaking. There is a virtually non-existant risk that extensions or themes somehow manage to rely on the random weights and/or line numbers. They will break. This risk is very slim, and these extensions should be updated anyway. Relying on completely broken behaviour and/or code is bad.
 + Testcase: reloading content in a type="chrome" browser, with stylesheets using various selector rules.
 + l10n impact: none.
* The patch does not change a public API.
* The patch does not change website compatibility or rendering.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 206714 [details] [diff] [review]
Updated order of init

a=dveditz for drivers.

We are extremely appreciative of the detailed comment 12 -- makes approving this easy.

Comment 14

[James Ross]

Landed on 1_8 and 1_8_0. Thanks!

Comment 15

[Marcia Knous [:marcia]]

verified fixed on the branch using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1. The dark motif in Chatzilla works fine on this build.