Closed Bug 70831 Opened 25 years ago Closed 25 years ago

Table background color is incorrect

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: Junk_HbJ, Assigned: dbaron)

References

(
URL
)

Details

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

Attachments

(15 files)

Testcase without doctype
525 bytes, text/html
Details
Testcase with HTML 3.2 doctype
576 bytes, text/html
Details
Testcase with HTML 4.01 doctype
617 bytes, text/html
Details
testcase without doctype 2001-02-16
5.58 KB, image/png
Details
testcase without doctype 2001-02-28
6.12 KB, image/png
Details
fix
1.98 KB, patch
Details | Diff | Splinter Review
testcase messed up by above fix
527 bytes, text/html
Details
better testcase
629 bytes, text/html
Details
testcase with image that shows that we never emulated the quirks correctly and probably never will
884 bytes, text/html
Details
same testcase, with a border, to show that this is a "parsing" bug in NN 4.x
915 bytes, text/html
Details
better patch
2.81 KB, patch
Details | Diff | Splinter Review
proposed fix (this directly undoes the original regression)
1.80 KB, patch
Details | Diff | Splinter Review
proposed fix (this directly undoes the original regression), with diff -b (ignoring whitespace)
890 bytes, patch
Details | Diff | Splinter Review
perhaps a better fix?
1.66 KB, patch
Details | Diff | Splinter Review
apply only in quirks mode
1.56 KB, patch
Details | Diff | Splinter Review
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
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
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).
Making a test case.
Keywords: makingtest
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
there have been three checkins in this time: bug 68411 bug 44505 and dcone's checkin from 2702.
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.
Adding others to the CC list who might have some insight.
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.
Reassigning to Pierre.
Assignee: karnaze → pierre
*** Bug 71048 has been marked as a duplicate of this bug. ***
*** Bug 72094 has been marked as a duplicate of this bug. ***
*** Bug 71775 has been marked as a duplicate of this bug. ***
Target Milestone: --- → mozilla0.9
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
*** Bug 72424 has been marked as a duplicate of this bug. ***
And yet another URL showing this (from another duplicate): http://www.virtualbank.com
QA contact update
QA Contact: chrisd → amar
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 </>
Also seen on http://my.yahoo.com
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.
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.
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. ***
hwaara's patch in bug 46480 fixes this behaviour.
*** Bug 75887 has been marked as a duplicate of this bug. ***
Whiteboard: interested for 0.9
I have a fix. It is related in some way to bug 69143.
Whiteboard: interested for 0.9 → 0.9 [fix in hand]
Attached patch fix — — Splinter Review
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.
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
r=karnaze
sr=waterson
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).
Ok, sr= withdrawn. Marc, why don't you take over?
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).
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.
Attached file better testcase —
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.)
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...
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.
> >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.
> 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.)
> > 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.
I take your point that all these quirks are not in IE either.
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.
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.
Attached patch better patch — — Splinter Review
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.
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); } }
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.)
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?
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.
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
Blocks: 67478
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.
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
I agree - see if the drivers will let it in (or I'll do that for you if you want).
I just sent mail to drivers, letting them know that I'm giving a=roc+moz on their behalf :-).
r=attinasi for patch 32002
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?
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.
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
Whiteboard: 0.9 [fix in hand] → 0.9 [fix in hand] have r=,sr=,a= ready to check in.
a=chofmann
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.
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
Closed: 25 years ago
Resolution: --- → FIXED
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
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: