Last Comment Bug 70831 - Table background color is incorrect
: Table background color is incorrect
Status: VERIFIED FIXED
0.9 [fix in hand] have r=,sr=,a= rea...
: regression
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: mozilla0.9
Assigned To: David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
: Amarendra Hanumanula
Mentors:
http://www.freepascal.org/bugs/db.php...
: 71048 71775 72094 72424 75748 75887 (view as bug list)
Depends on:
Blocks: 67478
  Show dependency treegraph
 
Reported: 2001-03-04 05:02 PST by Junk_HbJ
Modified: 2013-12-27 14:35 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase without doctype (525 bytes, text/html)
2001-03-04 12:00 PST, Oliver Klee
no flags Details
Testcase with HTML 3.2 doctype (576 bytes, text/html)
2001-03-04 12:01 PST, Oliver Klee
no flags Details
Testcase with HTML 4.01 doctype (617 bytes, text/html)
2001-03-04 12:01 PST, Oliver Klee
no flags Details
testcase without doctype 2001-02-16 (5.58 KB, image/png)
2001-03-04 12:18 PST, Bernd
no flags Details
testcase without doctype 2001-02-28 (6.12 KB, image/png)
2001-03-04 12:18 PST, Bernd
no flags Details
fix (1.98 KB, patch)
2001-04-23 05:16 PDT, Pierre Saslawsky
no flags Details | Diff | Splinter Review
testcase messed up by above fix (527 bytes, text/html)
2001-04-23 15:10 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
better testcase (629 bytes, text/html)
2001-04-23 16:02 PDT, Pierre Saslawsky
no flags Details
testcase with image that shows that we never emulated the quirks correctly and probably never will (884 bytes, text/html)
2001-04-23 16:34 PDT, Pierre Saslawsky
no flags Details
same testcase, with a border, to show that this is a "parsing" bug in NN 4.x (915 bytes, text/html)
2001-04-23 16:59 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
better patch (2.81 KB, patch)
2001-04-23 17:53 PDT, Pierre Saslawsky
no flags Details | Diff | Splinter Review
proposed fix (this directly undoes the original regression) (1.80 KB, patch)
2001-04-23 18:36 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
proposed fix (this directly undoes the original regression), with diff -b (ignoring whitespace) (890 bytes, patch)
2001-04-23 18:36 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
perhaps a better fix? (1.66 KB, patch)
2001-04-23 19:02 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
apply only in quirks mode (1.56 KB, patch)
2001-04-24 09:47 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review

Description Junk_HbJ 2001-03-04 05:02:10 PST
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; 0.9) Gecko/20010302
BuildID:    2001030205

The rows of the table should be orange (like the empty fields), but are rendered
white. Mozilla 0.8 renders correct, btw.

Reproducible: Always
Steps to Reproduce:
1. Type in the URL

That's it...

Actual Results:  White table rows

Expected Results:  Orange table rows
Comment 1 Bernd 2001-03-04 05:29:02 PST
this is indeed a regression. The page renders fine with 2001-01-24-20. Win98 and
renders with white cells 2001-02-28. Adding a keyword. Confirming, a testcase
would be great.
Comment 2 Oliver Klee 2001-03-04 08:09:35 PST
Actually, the background is not always displayed white, but as your default
background colour (I have a shade of lilac as default background colour for
exactly finding bugs like this).
Comment 3 Oliver Klee 2001-03-04 08:23:40 PST
Making a test case.
Comment 4 Oliver Klee 2001-03-04 12:00:56 PST
Created attachment 26742 [details]
Testcase without doctype
Comment 5 Oliver Klee 2001-03-04 12:01:24 PST
Created attachment 26743 [details]
Testcase with HTML 3.2 doctype
Comment 6 Oliver Klee 2001-03-04 12:01:54 PST
Created attachment 26744 [details]
Testcase with HTML 4.01 doctype
Comment 7 Oliver Klee 2001-03-04 12:03:41 PST
Looks like some problem with Quirks mode: Without a doctype or with the HTML 3.2
doctype, the bug occurs. With the HTML 4.01 strict doctype, the bug does not occur.

Is this an intended Quirk?
Comment 8 Bernd 2001-03-04 12:18:12 PST
Created attachment 26747 [details]
testcase without doctype 2001-02-16
Comment 9 Bernd 2001-03-04 12:18:59 PST
Created attachment 26748 [details]
testcase without doctype 2001-02-28
Comment 10 Bernd 2001-03-04 12:23:49 PST
there have been three checkins in this time:
bug 68411
bug 44505
and dcone's checkin from 2702.
Comment 11 Bernd 2001-03-05 11:36:22 PST
none of these checkins are responsible for this regression. It also horks viewer
demo #6 and two table regression tests inside  layout/html/tests/table/other.
2001-02-23-10 was still fine.
Comment 12 karnaze (gone) 2001-03-05 11:59:36 PST
Adding others to the CC list who might have some insight.
Comment 13 Bernd 2001-03-06 10:29:03 PST
The regression has been caused by the checkin for bug 67478. Backing out the
changes for this bug makes the rendering of the testcase fine, the viewer #6 and
the table testcase in the /tests/table/other directory. The automated regression
test does not catch this kind of error :-((.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2001-03-06 22:19:55 PST
I'm on the road and don't have a Mozilla test environment. I have no idea what's 
going on here, maybe Marc can figure it out. Is the BodyFixupRule being applied 
to the table cell for some reason?

Try adding a background color to the BODY element in the testcase and see if the 
result works in any build.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2001-03-06 22:33:43 PST
Wait, I know what's going on here. In quirks mode table cells inherit their 
background from the document background (emulating old NS4 bug). Due to bug 
67478, the document background used to be set to transparent as long as the HTML 
and BODY elements had no background color. The fix to 67478 makes the canvas get 
its background color set to the correct default background color. So, now we're 
inheriting the default background color into the cells.

The solution is to change the quirk code so that it doesn't inherit into table 
cells unless a background is explicitly specified on the HTML and/or BODY 
element.
Comment 16 karnaze (gone) 2001-03-07 15:45:45 PST
Reassigning to Pierre.
Comment 17 Boris Zbarsky [:bz] 2001-03-15 13:09:16 PST
*** Bug 71048 has been marked as a duplicate of this bug. ***
Comment 18 Bernd 2001-03-16 22:21:20 PST
*** Bug 72094 has been marked as a duplicate of this bug. ***
Comment 19 Bernd 2001-03-16 22:23:21 PST
*** Bug 71775 has been marked as a duplicate of this bug. ***
Comment 20 karnaze (gone) 2001-03-17 16:36:18 PST
Moving to m0.9
Comment 21 Mike Pinkerton (not reading bugmail) 2001-03-18 16:27:24 PST
another url that shows this is 

http://www.headphone.com/ProductsHeadphones/SennheiserHD570.asp
Comment 22 Oliver Klee 2001-03-18 17:32:38 PST
*** Bug 72424 has been marked as a duplicate of this bug. ***
Comment 23 Oliver Klee 2001-03-18 17:33:45 PST
And yet another URL showing this (from another duplicate):
http://www.virtualbank.com
Comment 24 Amarendra Hanumanula 2001-03-22 13:16:07 PST
QA contact update
Comment 25 megaton411 2001-03-27 00:23:05 PST
Also seen while viewing 

http://til.info.apple.com/
Comment 26 Daniel Glazman (:glazou) 2001-04-03 05:44:22 PDT
I think this bug should be marked INVALID because the document contains
several markup errors, one of them being absolutely fatal. With this one
fixed, the rows become orange again :

1. the document contains <html><body><html> ; this is the fatal error

2. one table start tag is invalid : <TABLE WIDTH="100%"CELLPADDING=2 ...
   a space is missing between the closing double-quote and CELLPADDING

3. the document contains a </>

Comment 27 Luke Koleszar 2001-04-03 07:16:18 PDT
Also seen on http://my.yahoo.com
Comment 28 Bernd 2001-04-03 09:55:08 PDT
Are you in right bug Daniel?

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=26743

- does not have illegal <html> <body> <html>
- the table start tag is correct
- the testcase does not contain a <a>/

And as described also the 

resource:///res/samples/test6.html renders wrong.

Comment 29 Daniel Glazman (:glazou) 2001-04-03 10:17:33 PDT
Yes I am, click on the URL field in header of the bug.

http://www.freepascal.org/bugs/db.php3?statusfield=Unfixed
Comment 30 Bernd 2001-04-03 11:15:36 PDT
Daniel, before you invalidate this bug just look on the testcase I mentioned,
and may be for reference with a NN4.7. I don't care either about broken external
pages, but this bug breaks the table layout regression testcase
layout/html/table/other/nested2.html and this exactly what concerns me. (Another
small hint: pinkerton@netscape.com will love to reopen this bug if you really
close it as invalid, because that wrong inherited color is white under Winxx but
gray under Mac...). May be we should remove the URL.
Comment 31 Mike Pinkerton (not reading bugmail) 2001-04-03 11:40:19 PDT
and yes, i will reopen in a heartbeat. this happens on many pages and is plainly 
seen on win32 when you change your windows os background color to lime green.

there are 4 other urls w/in this one bug that illustrate this problem. don't be a 
pedant.
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2001-04-03 17:27:58 PDT
This is a straightforward bug. We know what's causing it and what to do about 
it, someone just needs to write the code.
Comment 33 Pierre Saslawsky 2001-04-06 16:07:00 PDT
Sorry everybody, I neglected my bug list in the past few weeks.

This bug should obviously not be marked Invalid.  As Luke pointed out, it also 
happens on Yahoo.  I'll work on it when I'm done with the style system memory 
footprint reduction ("real soon now").
Comment 34 Boris Zbarsky [:bz] 2001-04-12 14:34:20 PDT
*** Bug 75748 has been marked as a duplicate of this bug. ***
Comment 35 Bernd 2001-04-13 06:47:16 PDT
hwaara's patch in bug 46480  fixes this behaviour.
Comment 36 Bernd 2001-04-13 06:47:54 PDT
*** Bug 75887 has been marked as a duplicate of this bug. ***
Comment 37 Pierre Saslawsky 2001-04-23 05:15:34 PDT
I have a fix.  It is related in some way to bug 69143.
Comment 38 Pierre Saslawsky 2001-04-23 05:16:09 PDT
Created attachment 31825 [details] [diff] [review]
fix
Comment 39 Pierre Saslawsky 2001-04-23 07:05:46 PDT
My fix doesn't work for cases like http://www.virtualbank.com where there is 
background image and a background color, but I think it is safe and valuable 
enough to check it in for 0.9.
Comment 40 Pierre Saslawsky 2001-04-23 14:43:47 PDT
I sent a review request morning early this morning to drivers@mozilla.org but 
nobody replied.

Marc?  ChrisK?  ChrisW?  Brendan?  Any volunteer?  This is one of the bugs with 
the most dups.
Comment 41 karnaze (gone) 2001-04-23 14:53:14 PDT
r=karnaze
Comment 42 Chris Waterson 2001-04-23 15:00:06 PDT
sr=waterson
Comment 43 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 15:03:04 PDT
Wouldn't it be simpler to reset to transparent rather than to the closest
non-transparent background color?  I imagine the latter could do strange things
in the case of overflow (e.g., floating tables that extend below their parent).
Comment 44 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 15:10:36 PDT
Created attachment 31893 [details]
testcase messed up by above fix
Comment 45 Chris Waterson 2001-04-23 15:18:10 PDT
Ok, sr= withdrawn. Marc, why don't you take over?
Comment 46 Pierre Saslawsky 2001-04-23 15:34:27 PDT
Resetting the background to transparent is what is done already in 
StyleColorImpl::ResetFrom(null) but since in quirks mode we cut off all 
inheritance for the second style resolution, we need to fetch the background from 
the nearest parent.  As a comment says, "this really blows".

Your comment made me think of another way of doing it, though.  We could save the 
computed style from the first style resolution and override the style from the 
second resolution only if the first one has a transparent background (meaning 
that no rules and html attributes applied to the table element).
Comment 47 Pierre Saslawsky 2001-04-23 16:01:30 PDT
dbaron's testcase can be simplified: the table doesn't need to be floating and 
all the colors should not be inherited.

I'm going to attach another testcase that shows that the table background should 
not come from the first non-transparent background but from the BODY indeed, and 
that the text color should not be inherited at all but set to the default.
Comment 48 Pierre Saslawsky 2001-04-23 16:02:05 PDT
Created attachment 31912 [details]
better testcase
Comment 49 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 16:24:04 PDT
Why BODY?  Because NN 4.x completely messes up that test case?

I really think it ought to be transparent, which I think is what it was before,
right?  What exactly was it that caused it to stop being transparent?

(I also think this whole bit of code would be better off in quirks.css (or maybe
the prefs sheet) than in C++ -- I vaguely remember arguing about that around two
years ago...  and I think it's probably making us more quirky than we really
need to be.)
Comment 50 Pierre Saslawsky 2001-04-23 16:32:40 PDT
I'm going to attach another testcase with a background image that shows that:
- we never emulated this quirk correctly when an image is present.
- it is probably impossible to emulate all the quirks that occur with tables.

If we are stressed by time, I propose to checkin the attached fix which covers 
most of the usual cases.  And later we can work work on transparent backgrounds 
for tables over images even when they are embedded within a block with a non-
transparent background...
Comment 51 Pierre Saslawsky 2001-04-23 16:34:31 PDT
Created attachment 31919 [details]
testcase with image that shows that we never emulated the quirks correctly and probably never will
Comment 52 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 16:42:41 PDT
I think you're focusing on a bug that was in NN 4.x only (that tables tend to
end block containers -- more of a parser bug than a style bug) rather than the
accepted table quirk that is in IE 3+ and NN 4.x.
Comment 53 Pierre Saslawsky 2001-04-23 16:45:24 PDT
>
>Why BODY?  Because NN 4.x completely messes up that test case?
>

Sure.  The whole discussion is around Nav quirks, there is no bug in strict mode, 
and it appears that Nav makes the table background show the BODY background and 
nothing else.  Just beeing transparent doesn't cut it.  Inheriting the whole BODY 
color & background style doesn't either.

The solution cannot be in the table drawing code either because, as my latest 
testcase shows, the table ignores its parent's non-transparent background.  
Finally, I don't see how this quirk could be described using normal style 
declarations in quirks.css.  Run the testcase, you will see.

Again, I have a quick solution that covers the most obvious aspects of this bug.  
If drivers@mozilla.org want to ship the bits today or tomorrow, I recommend we 
take my fix rather than nothing.
Comment 54 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 16:49:44 PDT
> The solution cannot be in the table drawing code either because, as my latest 
> testcase shows, the table ignores its parent's non-transparent background.  

That's a bug in NN 4.x and is not present in IE.  Quirks mode is for quirks that
existing web pages depend on to be rendered correctly, and if the bug isn't in
IE then it's very unlikely that web pages depend on it.  (The bug you're looking
at is more of a parser-ish bug anyway.)

> Finally, I don't see how this quirk could be described using normal style 
> declarations in quirks.css.  Run the testcase, you will see.

Examine the quirk that is needed for existing web pages -- i.e., the one that's
common between NN 4.x and IE.  (See bug 1044 and related bugs for background.)
Comment 55 Pierre Saslawsky 2001-04-23 16:52:00 PDT
>
> I think you're focusing on a bug that was in NN 4.x only

No, I pointed out that aspect just to say that we will probably never emulate all 
the quirks, but the main point is that a TABLE within a DIV withing the BODY 
shows the BODY background, not the DIV background.

This can't be emulated in quirks.css and I don't think it can be done only within 
style resolution, or only within the table drawing code either.
Comment 56 Pierre Saslawsky 2001-04-23 16:59:31 PDT
I take your point that all these quirks are not in IE either.
Comment 57 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 16:59:41 PDT
Created attachment 31923 [details]
same testcase, with a border, to show that this is a "parsing" bug in NN 4.x
Comment 58 Pierre Saslawsky 2001-04-23 17:30:12 PDT
If we play at denigrating each other's testcases, I can say that yours, showing a 
floating table in quirks mode, isn't very valid either because floating elements 
did not exist in Nav4.

Remains a better-than-nothing patch, if someone wants it.
Comment 59 Pierre Saslawsky 2001-04-23 17:53:05 PDT
I will attach another patch that's not going to make everybody happy but at least 
it seems to work with all the reported URLs, and with the different testcases 
before they got out of hands.
Comment 60 Pierre Saslawsky 2001-04-23 17:53:35 PDT
Created attachment 31935 [details] [diff] [review]
better patch
Comment 61 Pierre Saslawsky 2001-04-23 18:12:11 PDT
Could someone please review?

My apologies for not asserting more vigorously the validity of the patch in my 
previous comment but we are talking about realtively weird quirks here.  The 
patch makes table inherit the background color from the nearest parent if it 
doesn't have an image.  The point that needs to be discussed is whether it should 
be applied to floats and fixed positioned elements.
Comment 62 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 18:21:37 PDT
I think the real problem here has something to do with the mDocumentColorRule
in nsHTMLStyleSheet.cpp (and its interaction with the changes for bug 67478). 
It's not clear why it was made to match elements in tables, since buster's
checkin comment (revision 3.189) refers to the BodyFixupRule.

As evidence, the following patch fixes the bug, although it probably breaks
other things (but I really don't know what).

Index: nsHTMLStyleSheet.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/html/style/src/nsHTMLStyleSheet.cpp,v
retrieving revision 3.216
diff -u -d -r3.216 nsHTMLStyleSheet.cpp
--- nsHTMLStyleSheet.cpp	2001/02/19 12:55:27	3.216
+++ nsHTMLStyleSheet.cpp	2001/04/24 01:18:52
@@ -899,9 +899,6 @@
         nsCompatibility mode;
         aPresContext->GetCompatibilityMode(&mode);
         if (eCompatibility_NavQuirks == mode) {
-          if (mDocumentColorRule) {
-            aResults->AppendElement(mDocumentColorRule);
-          }
           aResults->AppendElement(mTableBackgroundRule);
         }
       }
Comment 63 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 18:36:15 PDT
Created attachment 31946 [details] [diff] [review]
proposed fix (this directly undoes the original regression)
Comment 64 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 18:36:53 PDT
Created attachment 31947 [details] [diff] [review]
proposed fix (this directly undoes the original regression), with diff -b (ignoring whitespace)
Comment 65 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 18:51:48 PDT
I think the HTMLDocumentColorRule is a mostly vestigial (except perhaps for its
effect on tables) predecessor of the BodyFixupRule.  But what my fix does is
limits roc's patch to the BodyFixupRule for bug 67478 (which caused this
regression) to affecting only the canvas's style context and not the
HTMLDocumentColorRule as well.

I'm not sure whether I should propagate the foreground color anyway, though. 
Maybe it would be better to just use the bodyStyleColor in this part of the code
if the canvasStyleColor was neither the bodyStyleColor nor the htmlStyleColor
and leave everything else as is until there's a chance to investigate it
further.  (I think that would actually be directly undoing the original regression,
whereas my patch above is slightly different.)
Comment 66 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 19:02:18 PDT
Created attachment 31951 [details] [diff] [review]
perhaps a better fix?
Comment 67 Pierre Saslawsky 2001-04-23 20:01:37 PDT
I prefered to assume that the fix in bug 67478 was the right thing to do because 
the BodyFixupRule is a can of worms that many people stumbled over in the last 
several years, and it looked like we finally had reached a consensus on it, so I 
focused on fixing only what was broken: table backgrounds in quirks mode.

dbaron's patch should be reviewed by the people who participated to bug 67478: 
roc+moz, attinasi, kmcclusk.

My patch is limited to table code.  Anybody and their worms can review it.

Hey!  Two fixes for the same problem!  Isn't it a reviewer dream?
Comment 68 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-23 20:12:55 PDT
While I think the BodyFixupRule is pretty good right now, the code I changed was
the part of the BodyFixupRule code that sets up the HTMLDocumentColorRule, which
I think is at least partially vestigial code that predated BodyFixupRule and may
even be causing some other bugs that should probably be investigated.

The patch you proposed to the table style hack is not the right thing -- it just
adds code in one place to cover up a bug somewhere else, and it doesn't even do
that very well since it creates a whole cascade (no pun intended) of other bugs.
(I could easily come up with other testcases that your "better patch" still
breaks, and, belive it or not, authors will expect tables and floats to work
properly in quirks mode as NN 4.x loses market share.)  The table style hack
itself is something I think we need to reexamine (I think it could be simplified
greatly), and I'd rather not see yet another bug "fix" depend on it.
Comment 69 Pierre Saslawsky 2001-04-23 22:45:46 PDT
No problem: reassigned to you then.  Marc who wrote the BodyFixupRule and roc+moz 
who made the patch for bug 67478 should review the last proposed fix at 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31951
Comment 70 Pierre Saslawsky 2001-04-24 03:48:55 PDT
On a second thought: r=pierre, hoping it can help to get this checked in for 
moz0.9.  I verified the testcases attached to the different bugs filed against 
the BodyFixupRule in the past, and the testcases attached to the present bug.  
Note that some of the testcases we both did here are not rendered similarly to 
Nav4 but rather like IE in the way that the foreground color from the body is 
inherited within the table.  But it's fine with me.  Thanks.
Comment 71 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2001-04-24 08:56:48 PDT
I like dbaron's fix but I think it should only apply in Quirks mode. In Strict 
mode we should set the DocumentForegroundColor and DocumentBackgroundColor 
correctly.
Comment 72 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-24 09:47:52 PDT
Created attachment 32002 [details] [diff] [review]
apply only in quirks mode
Comment 73 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-24 09:57:36 PDT
OK -- I made the change that you suggested.

I think the real fix is to split up the 3 uses for HTMLDocumentStyleColor:
 1) We use it to implement document.fgColor and document.fgColor in HTML
documents when we have no body element (i.e, if the script is running before the
body element is created).  (This issue is messy -- I'm also wondering if the
code in the body element itself needs some fixing too, but that's another issue...)

 2) We use it to match the HTML element in the attribute stylesheet
(nsHTMLStyleSheet).  I think this predates the BodyFixupRule and can be removed
(and it may mess up user stylesheets in some cases).

 3) We use it to apply to inner table contents in quirks mode.  We should
reexamine whether this is really the best way to handle this, but if it is, then
perhaps the HTMLDocumentColorRule could be only for this and (1) could be
handled by something else (or vice versa, although I think the solution in (1)
probably doesn't get along with the BodyFixupRule).
Comment 74 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2001-04-24 10:01:35 PDT
I like that patch! Let's get it in! sr=roc+moz
Comment 75 Marc Attinasi 2001-04-24 10:21:28 PDT
I agree - see if the drivers will let it in (or I'll do that for you if you 
want).
Comment 76 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2001-04-24 10:23:27 PDT
I'll do that.
Comment 77 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2001-04-24 10:28:22 PDT
I just sent mail to drivers, letting them know that I'm giving a=roc+moz on 
their behalf :-).
Comment 78 Marc Attinasi 2001-04-24 11:51:42 PDT
r=attinasi for patch 32002
Comment 79 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-24 15:28:27 PDT
I hate to say this... but there was a minor typo in that patch.  (It probably
doesn't make much of a difference, but I'd prefer it as close to the way it was
before.)

Instead of (within the navquirks check):

+      documentStyleColor = htmlStyleColor;

I meant to say:

+      documentStyleColor = bodyStyleColor;


Could you re-review?
Comment 80 Marc Attinasi 2001-04-24 15:49:17 PDT
Uh, OK, I still like it. r=attinasi

This time I'll ask the important question: did you run through some BODY/HTML
background color tests?

Here are Ian's:
http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/background/)

And the Box Acid Test would be good too.
Comment 81 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-24 17:23:32 PDT
Ian's tests look like they're the same as the current bugs Ian described should
be in them.  The boxacidtest is fine.
Comment 82 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2001-04-24 18:00:59 PDT
It's fine with me either way. sr=roc+moz
Comment 83 chris hofmann 2001-04-24 18:50:45 PDT
a=chofmann
Comment 84 Pierre Saslawsky 2001-04-24 19:21:50 PDT
I would be more comfortable if you could check against all the testcase as I did 
yesterday.  You can find them under the current bug (+ the URLs like 
http://www.virtualbank.com), bug 67478, bug 17911, bug 44199, bug 2285, bug 
40217, bug 33710 and bug 2054.
Comment 85 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2001-04-24 19:59:10 PDT
I also checked most of the testcases on this bug and testcases on all of the
other bugs mentioned (in addition to Ian's test suite and the boxacidtest, which
I checked earlier) before checking in the fix at 2001-04-24 19:55 PDT.

I filed bug 77287 on myself to investigate the issues I mentioned with
HTMLDocumentColorRule.
Comment 86 Amarendra Hanumanula 2001-06-06 14:42:06 PDT
 Verified with the test cases mentioned by Ian and the other test cases. Works 
fine for me.. Marking verified on Build ID : 2001060509


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