Table background color is incorrect

VERIFIED FIXED in mozilla0.9

Status

()

Core
Layout: Tables
--
major
VERIFIED FIXED
17 years ago
4 years ago

People

(Reporter: Junk_HbJ, Assigned: dbaron)

Tracking

({regression})

Trunk
mozilla0.9
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: 0.9 [fix in hand] have r=,sr=,a= ready to check in., URL)

Attachments

(15 attachments)

525 bytes, text/html
Details
576 bytes, text/html
Details
617 bytes, text/html
Details
5.58 KB, image/png
Details
6.12 KB, image/png
Details
fix
1.98 KB, patch
Details | Diff | Splinter Review
527 bytes, text/html
Details
629 bytes, text/html
Details
884 bytes, text/html
Details
915 bytes, text/html
Details
2.81 KB, patch
Details | Diff | Splinter Review
1.80 KB, patch
Details | Diff | Splinter Review
890 bytes, patch
Details | Diff | Splinter Review
1.66 KB, patch
Details | Diff | Splinter Review
1.56 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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

17 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: qawanted, regression

Comment 2

17 years ago
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

17 years ago
Making a test case.
Keywords: makingtest

Comment 4

17 years ago
Created attachment 26742 [details]
Testcase without doctype

Comment 5

17 years ago
Created attachment 26743 [details]
Testcase with HTML 3.2 doctype

Comment 6

17 years ago
Created attachment 26744 [details]
Testcase with HTML 4.01 doctype

Comment 7

17 years ago
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?
Keywords: makingtest

Comment 8

17 years ago
Created attachment 26747 [details]
testcase without doctype 2001-02-16

Comment 9

17 years ago
Created attachment 26748 [details]
testcase without doctype 2001-02-28

Comment 10

17 years ago
there have been three checkins in this time:
bug 68411
bug 44505
and dcone's checkin from 2702.

Comment 11

17 years ago
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

17 years ago
Adding others to the CC list who might have some insight.

Comment 13

17 years ago
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 :-((.
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.
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

17 years ago
Reassigning to Pierre.
Assignee: karnaze → pierre
*** Bug 71048 has been marked as a duplicate of this bug. ***

Comment 18

16 years ago
*** Bug 72094 has been marked as a duplicate of this bug. ***

Comment 19

16 years ago
*** Bug 71775 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Target Milestone: --- → mozilla0.9

Comment 20

16 years ago
Moving to m0.9
another url that shows this is 

http://www.headphone.com/ProductsHeadphones/SennheiserHD570.asp
Keywords: mozilla0.9, nsbeta1, nsmac1
OS: Windows 98 → All
Hardware: PC → All

Comment 22

16 years ago
*** Bug 72424 has been marked as a duplicate of this bug. ***

Comment 23

16 years ago
And yet another URL showing this (from another duplicate):
http://www.virtualbank.com

Comment 24

16 years ago
QA contact update
QA Contact: chrisd → amar

Comment 25

16 years ago
Also seen while viewing 

http://til.info.apple.com/
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

16 years ago
Also seen on http://my.yahoo.com

Comment 28

16 years ago
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.

Yes I am, click on the URL field in header of the bug.

http://www.freepascal.org/bugs/db.php3?statusfield=Unfixed

Comment 30

16 years ago
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.
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.
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

16 years ago
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").
Severity: normal → major
Status: NEW → ASSIGNED
*** Bug 75748 has been marked as a duplicate of this bug. ***

Comment 35

16 years ago
hwaara's patch in bug 46480  fixes this behaviour.

Comment 36

16 years ago
*** Bug 75887 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Whiteboard: interested for 0.9

Comment 37

16 years ago
I have a fix.  It is related in some way to bug 69143.
Whiteboard: interested for 0.9 → 0.9 [fix in hand]

Comment 38

16 years ago
Created attachment 31825 [details] [diff] [review]
fix

Comment 39

16 years ago
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

16 years ago
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.
Keywords: review

Comment 41

16 years ago
r=karnaze

Comment 42

16 years ago
sr=waterson
(Assignee)

Comment 43

16 years ago
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).
(Assignee)

Comment 44

16 years ago
Created attachment 31893 [details]
testcase messed up by above fix

Comment 45

16 years ago
Ok, sr= withdrawn. Marc, why don't you take over?

Comment 46

16 years ago
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

16 years ago
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

16 years ago
Created attachment 31912 [details]
better testcase
(Assignee)

Comment 49

16 years ago
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

16 years ago
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

16 years ago
Created attachment 31919 [details]
testcase with image that shows that we never emulated the quirks correctly and probably never will
(Assignee)

Comment 52

16 years ago
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

16 years ago
>
>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.
(Assignee)

Comment 54

16 years ago
> 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

16 years ago
>
> 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

16 years ago
I take your point that all these quirks are not in IE either.
(Assignee)

Comment 57

16 years ago
Created attachment 31923 [details]
same testcase, with a border, to show that this is a "parsing" bug in NN 4.x

Comment 58

16 years ago
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

16 years ago
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

16 years ago
Created attachment 31935 [details] [diff] [review]
better patch

Comment 61

16 years ago
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.
(Assignee)

Comment 62

16 years ago
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);
         }
       }
(Assignee)

Comment 63

16 years ago
Created attachment 31946 [details] [diff] [review]
proposed fix (this directly undoes the original regression)
(Assignee)

Comment 64

16 years ago
Created attachment 31947 [details] [diff] [review]
proposed fix (this directly undoes the original regression), with diff -b (ignoring whitespace)
(Assignee)

Comment 65

16 years ago
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.)
(Assignee)

Comment 66

16 years ago
Created attachment 31951 [details] [diff] [review]
perhaps a better fix?

Comment 67

16 years ago
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?
(Assignee)

Comment 68

16 years ago
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

16 years ago
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
Assignee: pierre → dbaron
Status: ASSIGNED → NEW

Updated

16 years ago
Blocks: 67478

Comment 70

16 years ago
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.
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.
(Assignee)

Comment 72

16 years ago
Created attachment 32002 [details] [diff] [review]
apply only in quirks mode
(Assignee)

Comment 73

16 years ago
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).
I like that patch! Let's get it in! sr=roc+moz

Comment 75

16 years ago
I agree - see if the drivers will let it in (or I'll do that for you if you 
want).
I'll do that.
I just sent mail to drivers, letting them know that I'm giving a=roc+moz on 
their behalf :-).

Comment 78

16 years ago
r=attinasi for patch 32002
(Assignee)

Comment 79

16 years ago
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

16 years ago
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.
(Assignee)

Comment 81

16 years ago
Ian's tests look like they're the same as the current bugs Ian described should
be in them.  The boxacidtest is fine.
It's fine with me either way. sr=roc+moz

Updated

16 years ago
Whiteboard: 0.9 [fix in hand] → 0.9 [fix in hand] have r=,sr=,a= ready to check in.

Comment 83

16 years ago
a=chofmann

Comment 84

16 years ago
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.
(Assignee)

Comment 85

16 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 86

16 years ago
 Verified with the test cases mentioned by Ian and the other test cases. Works 
fine for me.. Marking verified on Build ID : 2001060509

Status: RESOLVED → VERIFIED

Updated

16 years ago
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.