46268 - [QUIRKS] bgcolor does not fill entire table

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[Kevin Ar18]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows 98)
BuildID:    2000061311

When bgcolor is used on a table, Mozilla does not display the background color 
on the entire table.  Instead, wherever there are spaces between table cells 
the background table color is missing.

Reproducible: Always
Steps to Reproduce:
1. Make a table in a html page
2. Insert the bgcolor="#000000" tag into the table tag like so:
  <table bgcolor="#000000">

Actual Results:  The individual cells of the table fill with the specified 
bgcolor (background color) however, the spaces in between cells do not fill 
with the specified color.  Thus, the entire table does not fill with the 
specified bgcolor.					

Expected Results:  The entire table (including space between and around cells 
should fill with the specified table background color.  See Internet Explorer 
as an example.

Comment 1

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

Probably duplicate of bug 4510.

Comment 2

[fantasai]

No, this is different. It deals with HTML's 'bgcolor', which, AFAICS, should 
only affect table cells, not the cellspacing; Nav4 and Mozilla display this 
correctly.
     http://www.w3.org/TR/html4/present/graphics.html#adef-bgcolor

No matter how you interpret "canvas", MSIE is displaying things wrong:
Either you put the backgrounds on the cells, or you put the backgrounds on the 
entire element it is specified on. MSIE is not doing this for table row 
backgrounds. 

However, painting the background on table cells is *explicitly specified in the 
definition*, not ambiguously in the prose, and Navigator has been doing that for 
years.

FYI:
      - Nav3 & Nav4 display backgrounds only on table cells.
      - Opera, IE3, & IE5 also display backgrounds specified on <table> in the 
        cellspacing, but don't do this for <tr>.
      - Amaya paints the background on the entire <table> and the entire <tr>.

I'd like to resolve this as invalid, but in light of the behavior of current 
browsers, I leave the decision to others.

Comment 3

[fantasai]

Attachment: testcase

Comment 4

[fantasai]

Mozilla is rendering the backgrounds on cellspacing in the testcase.. but not 
where I extracted it from.

The Nav4 behavior is caused by Quirks mode.

Comment 5

[fantasai]

Attachment: testcase in Quirks mode (missing doctype)

Comment 6

[fantasai]

http://lists.w3.org/Archives/Public/www-html/2000Jul/0146.html

Comment 7

[fantasai]

Although I can only see the HTML4 specification as allowing bgcolor to affect 
table cells, MSIE implemented this in version 2.0 (before the HTML4 spec added 
'bgcolor' to the list of valid attributes for <table>). 

*If MSIE's definition is to be followed for Quirks, which it should if Gecko is 
displaying this differently in Quirks at all, then 'bgcolor' on <table> should 
color the whole table, including cellspacing, but 'bgcolor' on <tr> should not 
affect cellspacing.*

Confirming bug to that end. Sorry for the mess.

Comment 8

[Gervase Markham [:gerv]]

*** Bug 32248 has been marked as a duplicate of this bug. ***

Comment 9

[Jerry Baker]

The spec does not say "if applied to a table element, it should only affect 
cells". I don't know what they meant by "body and table cells", but they go on 
to say it sets the background color for "tables". A table is a single entity and 
not a collection of independent cells. Rendering bgcolor in cells only leaves 
developers with having to do:
<table bgcolor="#000000">
<table cols ="2" rows="2">
...

Nesting tables everytime one wants the appearance of a *single* table with a 
solid background will irritate developers a lot. Where the spec is ambiguous, 
err on the side of what developers will want. That is what the specs are 
ostensibly for anyway.

Comment 10

[fantasai]

To clarify: This bug was confirmed to change the Quirks handling to imitate
Microsoft's, which /does/ fill cellspacing on the table (just not the <tr>). So
the spec wording here is a non-issue.

Comment 11

[Jerry Baker]

Ooohhhh. I should have looked at the testcase. This is bad. I had assumed that 
we were talking about border=0 cellspacing=>0. There is no way, whether quirks 
or not, that bgcolor of a table should be rendered in the cell spacing *when the 
border is set to a positive integer*. When the border=0 it should fill in the 
cellspacing since there is no border to cover it up, but not when there is a 
border defined.

Comment 12

[fantasai]

> When the border=0 it should fill in the cellspacing since there is no border 
> to cover it up, but not when there is a border defined.

That's inconsistent. Background color rules should not depend on the border 
property. And if the author wants such an effect, they just increase the 
cellpadding and set the cellspacing to zero, which has the advantage of working 
in Netscape 3.x & 4.x, too.

Comment 13

[Jerry Baker]

Not inconsistent at all. The very fact that you can assign a color to a table 
border implies that they overlay the background (otherwise what's the point?). 
Another way to think of it is that a background is a background after all, and 
table borders are not part of the background.

Comment 14

[post]

I'd just like to say, I think the BGcolor on a table should effect the
cellspacing. I can't think of a single reason why it should not, and I can think
of serveral reasons why it should.
this "bug" gets my vote:)

Comment 15

[Jerry Baker]

Because a border is not transparent might be one reason???

Comment 16

[Kevin Ar18]

The actual issue here is (to clarify my origional post) the cellspacing between 
each of the cells, not the table borders.

Looking at it from a pure html perspective - the bgcolor in the table tag 
should apply to the entire table, not pieces of a table - If you want to apply 
colors just to the cells of a table and not the cellspacing, you put the 
bgcolor in each of the td tags (cells) to be colored.  From a pure html 
purpective (and only from that perspective), that is how it naturally 
<i>should</i> work.  Properties of a tag should apply to the entire tag, not 
pieces of if.

From this perspective, it could also bring up the issue of coloring table 
borders:
http://bugzilla.mozilla.org/show_bug.cgi?id=46265

From a somewhat practical perspective, should cellspacing not contain the 
entire table's color?  Perhaps....  The only practical example of why you would 
want it to defer from the exepected <i>norm</i> is in a 3D border for example:
bgColor=3D"#000080" border=3D"0".
In this instance it might seem odd to have the bgcolor fill the cellspacing.

I've may have brought up more questions that I've clarified.
From a pure html standpoint, properies applied to a tag like the table should 
apply to the whole tag, and not pieces of it.  While from a more pratical 
application standpoint, you could go either way, with the exception maybe of 3D 
borders.

The question to answer is, are cells to be treated as divisions within a 
table?  or are cells to be treated as entities to themselves?
The way I see it, tables cover all of the cells within it.  Any property 
applied to the table affects the entire table and is separate from the property 
of individual cells.

Comment 17

[Jerry Baker]

That's like saying that a page's background should color every element since 
they are just parts of the body. It's silly, and so is coloring borders the 
color of the background in a table.

BTW there is no "3D" attribute of table borders in the W3C specification that I 
am aware of.

Comment 18

[fantasai]

 > Because a border is not transparent might be one reason???
Why'd the borders come in? This is purely a background bug!

Re: kevinar's post
Looking at it from a pure HTML standpoint, 'bgcolor' shouldn't exist at all.
Looking at a strict interpretation of the HTML spec, bgcolor doesn't affect 
cellspacing. (I'll send you a short proof if you want it.)
But since its original inventor (MS) decided that it does, and most people 
expect that it will, 'bgcolor' should, at least in Quirks mode, color the entire 
inner table frame.

I hope that last sentence summarizes this report's intent? Correct me if I'm 
wrong.

Comment 19

[Kevin Ar18]

Re: Jerry Baker's Comments
Actually, a page's background does affect the whole body of the page.  When you 
insert tables and other things, the background still remains behind those 
elements.
I believe I understand what you mean, and I think you misunderstood my post.  I 
don't mean that a td or tr should be overwritten by the bgcolor in the table 
tag, but that things should work in the manner like in the body.  bgcolor does 
affect the whole body as bgcolor (in my opinion) should affect the whole table 
unless things are inserted like cells (td) with different colors or another 
table, etc...  into the table.

As for coloring the borders, that was a whole other issue in another "bug" 
report.  I don't mean the table bgcolor should change the boardercolor; of 
course a table's bgcolor should not affect the boardercolor.

As for the 3D attribute, that was just a forsight for your consideration 
allowing for the possibility that it may be added to W3C in the future.

Re: [e-mail]
I guess you'll have to explain a little about what you mean by bgcolor 
shouldn't exist at all.
If you want to know what I meant by a "pure html standpoint", I was not 
speaking of WC3 standards.  In my view (and some will not agree) of html, a 
property applied to a tag should ecompass the entire scope of that tag.  As an 
example to explain what I mean, bgcolor and other things applied to the body 
apply to the whole body; properties applied to a layer affect that whole 
layer.  This does not mean that a table or some other tag should take on the 
properties of the body or layer it's in though.

Comment 20

[Mats Palmgren (inactive)]

*** Bug 61158 has been marked as a duplicate of this bug. ***

Comment 21

[Corey Puffalt]

What is the status of this bug...is it being fixed?  I don't understand why 
some people think this isn't a bug.  Look at it this way...from a side view as 
layers:

                         v---v-----cellpadding
cells:                --- --- ---    
enclosed by table:  ---------------
enclosed by body:  -----------------

cellpadding, according the the w3c spec is the spacing BETWEEN cells.  It makes 
no sense that the spacing between cells should be rendered with the body 
background when they are contained within a table (unless, of course, a table 
background has not been specified.)  As others have stated, doing it the 
current way forces developers to jump through hoops to get a desired background 
other than the body background.

Comment 22

[Corey Puffalt]

er...references to cellpadding should be replaced with cellspacing in my 
preceding post :/

Comment 23

[Kevin Ar18]

Attachment: Layering of tables

Comment 24

[Kevin Ar18]

Corey Puffalt,
If I understand your point correctly, that is the same argument I was trying to 
get accross.  I created an example of one way to look at how this thind may 
work.
Created an attachment (id=19723)
<a href="http://bugzilla.mozilla.org/showattachment.cgi?
attach_id=19723">http://bugzilla.mozilla.org/showattachment.cgi?
attach_id=19723</a>

Comment 25

[fantasai]

> I guess you'll have to explain a little about what you mean by bgcolor 
> shouldn't exist at all.

HTML was designed to be a structural, not a presentational, language.

> What is the status of this bug...is it being fixed?

The status of this bug is New--that is, confirmed, but not assigned.

> As others have stated, doing it the current way forces developers to
> jump through hoops to get a desired background other than the body 
> background.

Which is why this bug was filed, and why I confirmed it--to have the
bgcolor cover the entire table frame.


Kelvinar, I'm not entirely sure I understand your attachment correctly.
Would the code for the last example be
  <table bgcolor="lime">
    <tr>
      <td>..</td>
      <td bgcolor="red">..</td>
    </tr>
    <tr bgcolor="red">
      <td>..</td>
      <td>..</td>
    </tr>
  </table>
?
Or would the second row take it's color from bgcolor on its two <td>s?

Comment 26

[Kevin Ar18]

[e-mail],
At first I would have said the bgcolor would be on the td's or tr's,
but then I thought if it was on the tr would that mean the red would extend 
across the whole bottom row (including the spacing between cells) or just in 
all of the individual cells?

According to the layering in my example the tr probably wouldn't affect spacing 
between cells.

But, I don't know, what do you think?

Man this is gettin' complicated :)

Comment 27

[fantasai]

As bgcolor on <tr> doesn't affect cellspacing in any major browser, it shouldn't 
do so in Mozilla's Quirks mode, since Quirks tries to emulate what authors 
expect.

The behavior in Standard layout mode is, IMO, debatable. But that's outside the 
scope of this bug report. >:)

BTW, sorry for misspelling your name. m(_)m

Comment 28

[fantasai]

I think I can fix this. The code is in there already for Standard layout; only 
a few switches need adjustment. However, I am unable to build Mozilla. Is anyone 
willing to try building with patched source?

Comment 29

[Bernd]

fantasai, please attach the patch asap.

Comment 30

[fantasai]

Attachment: untested patch

Comment 31

[fantasai]

The attachment also removes Quirks behavior from background styles applied to 
row groups and column groups. (see bug 4510)

Please check the closing braces on nsTableRowFrame.cpp and 
nsTableRowGroupFrame.cpp before compiling; I edited the diff file directly a 
bit, as it didn't seem to reflect the changes properly. (Why? I don't know. I've 
never done a diff before.) The affected source is in the Paint function.

Comment 32

[fantasai]

Attachment: untested patch v2

Comment 33

[fantasai]

nsTableRowFrame.cpp should not have been changed.

Comment 34

[karnaze (gone)]

The 1st attachment invokes standard mode and paints the background of the table 
on the entire table, then the background of the rows on top of that, then the 
backgrounds of the cells on top of that. I guess this bug is not concerned with 
this behavior.

The 2nd attachment looks identical to Nav 4.7, because it is a quirk mode page, 
and that is the way it is supposed to work. If standard behavior is desired, 
then use a dtd that invokes it. 

So, why should we take this patch (which needs to be tested with the regression 
tests) and change quirks behavior to standard behavior? 

Comment 35

[fantasai]

This bug was opened to change Quirks behavior so that <table bgcolor="x"> paints 
the entire table frame. 
  - This is the behavior of all versions of MSIE and Opera. There is no need
    for a Quirk that is peculiar only to Navigator and on which few, if any,
    pages rely.
  - Most authors prefer this behavior.

The patch also enables Standard rendering for background styles applied to table 
row groups, and column groups. These are unsupported by Navigator; they take 
Quirks rendering in MSIE. (As it is not necessary to fixing this bug, I will 
remove that part of the patch should you so choose.)

However, Quirks behavior is _retained_ for table rows. 
This is the behavior of Navigator, MSIE, and Opera, and is thus the expected 
behavior.

Comment 36

[Bernd]

below follows a list of regression test's that fail due the proposed patch. I
support karnaze's view. I will not manual review all these testcases. I think
this bug should be held open in order to prevent dupes of the issue. I would
recommend to future the bug because for me it is a high risk -- low return
issue. With that big number of broken regression tests it seems to me that it is
very likely (near to 100% probability) that major sites will be broken.

regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\col_span.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\col_widths_fix_autoFix.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\col_widths_fix_fix.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\conflicts.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\misc.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\table_widths.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\table_widths.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\viewer_tests\verify\test4.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug1055-1.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug1055-2.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug1067-2.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug14929.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug1302.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug1430.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug14323.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug14929.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug16252.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug17548.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug17587.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug1800.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug18359.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug22122.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug23847.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug27038-2.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug27993-1.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug28928.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug2947.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug30332-1.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug33137.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug5188.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug5538.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug625.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug6304.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug7112-1.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug727.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug9123-1.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug9879.rgd failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\other\verify\empty_cells.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\other\verify\wa_table_thtd_rowspan.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\other\verify\wa_table_tr_align.rgd
failed
regression test
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\other\verify\wf_table_index.rgd
failed

Comment 37

[fantasai]

> I will not manual review all these testcases. 

I would not expect you to.

> With that big number of broken regression tests 

Most of them shouldn't have failed; this fix should only affect backgrounds, and 
none of those bugs deal with backgrounds (although several use backgrounds to 
illustrate other effects). I recognize, however that there may be a problem with 
the patch.

> it seems to me that it is very likely (near to 100% probability) that major 
> sites will be broken.

The intent is to emulate MSIE, whose behavior is very /unlikely/ to break major 
sites.

I can build Mozilla now, and I have run the regression tests. However, I did not 
get the same results you did. (Only one test failed and only the bug tests 
ran--no core, etc.) Are there any options I should be enabling? I'm running 
everything in default configuration on Linux.

Comment 38

[Bernd]

Table regression tests mean all tests in all directories listed in rtest.bat in
the layout/html/tests/table. Especially core and marvin are necessary. Every
bbox mismatch is a failure. Usually I ran the regression tests three times 1.
baseline without patch, 2. verify without patch in order to see that the
regression tests do not produce false warnings, 3. verify with the patch. I dump
the output of the tests into results file (with tee), in order to run grep over
it ('rgd failed' works great)afterwards. If a regression tests fail, I separate
these tests into a special directory, and try
to separate the table that causes the failure, this is sometimes very
cumbersome, for instance in core sometimes more than 10 tables are collected
into a single test. For me this bug is an RFE, which should be either futured or
marked as wontfix. As buster stated in a different bug: We have bigger fish to fry.

Comment 39

[fantasai]

Thanks for the tips~

Compiled source pulled on 2000-12-11, on Linux. All tests pass.

I ran the tests twice before and twice after applying changes and checked all 
the non-style data mismatches against the other verification results; everything 
seems accounted for. (Color & transparency were supposed to change.)

Is there anything else I should do?

> As buster stated in a different bug: We have bigger fish to fry.

Well, I can't fry any fish much bigger than this. :)

Comment 40

[Peter Henderson]

Can someone please tell me if I'm understanding the discussion/consensus properly?

table bgcolor: {standards: paint cellspacing} {quirks: paint cellspacing}
tr bgcolor: {standards: paint cellspacing} {quirks: don't paint cellspacing}

I believe this standards rendering is correct because AFAICS it is the only way
to control the color of the cellspacing. To use CSS to set all cells bgcolor
without painting the cellspacing, all you have to do is td {background-color:
#ffc} or whatever. Or if you want an alternative to the tr bgcolor:

<tr class="odd"><td>...
<tr class="even"><td>...
<tr class="odd"><td>...

and then do CSS like:

tr.odd td {background-color: #080}
tr.even td {background-color: #cfc}

You might have to do something fancy, like tr.odd > td, if you have nested
tables, but the problem is MSIE doesn't yet support child selectors. The point
of this comment is to try to summarize and clarify the discussion so far, and
hopefully I haven't misunderstood anything.

Comment 41

[fantasai]

Would it be possible to get this fixed for mozilla0.9?

Comment 42

[Amarendra Hanumanula]

QA contact update

Comment 43

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

I agree that we should try to get this in.  See my comments on bug 4510.

Just because regression tests show changes doesn't mean there are regressions. 
The change made a chance to the style contexts and a compensating change to
painting of backgrounds.  If we're moving closer to MSIE behavior it's more
likely that we're going to fix sites than break them.

Comment 44

[karnaze (gone)]

I don't have a problem with someone checking in this patch (since it just 
appears to change quirks mode into standard mode), if he/she will also manually 
review the regression test pages that Bernd cites. My fear is that since most of 
our tests are in quirks mode (shame on us) we may be exposing a standard mode 
bug to a wider number of pages. I will personally not be able to check this in 
for quite a while due the PDT approval process within Netscape. I'm marking this 
m1.0 just to get it off the untriaged radar.

Comment 45

[Bernd]

I will do the necessary work to get this in, but this will be after 0.9.2 as I
have already three other patches that should go into the tree....

Comment 46

[fantasai]

I just went through my regression test results for those tests Bernd cited: 
apparently, some of them did not run--there was no mention of them at all. But 
everything that did run (which was most of them) passed, and the only mismatches 
I got were in table background, background transparency, and color (see bug 
46480).

I'm afraid I won't be much help in manual reviewing those testcases, since they
wouldn't fail for me. (Well, one did fail, but it passed the second time.) 
However, if Bernd wants to send me the verification output (full text before and 
after), I'll go over that.

Comment 47

[Bernd]

Attachment: previous patch was slightly bit rotten

Comment 48

[Bernd]

Attachment: quirk test

Comment 49

[Bernd]

Attachment: strict test

Comment 50

[Bernd]

Attachment: quirk test with patch

Comment 51

[Bernd]

Chris wrote: My fear is that since most of 
our tests are in quirks mode (shame on us) we may be exposing a standard mode 
bug to a wider number of pages.

The good news is: you do not need fear it anymore, the bad one is, it is worse
than I imagined.

If you look at attachment 38472 [details], it becomes clear for me, before this patch can
go in, we need to file the bugs about background color in standard mode and make
these  bugs to block this bug, once the things are less ugly it should be show
time for this bug.

Comment 52

[Bernd]

*** Bug 82836 has been marked as a duplicate of this bug. ***

Comment 53

[Bernd]

Attachment: strict testcase rendering by IE

Comment 54

[fantasai]

> we need to file the bugs about background color in standard mode and make
> these  bugs to block this bug, once the things are less ugly it should be show
> time for this bug.

Few authors use or format HTML table columns or row groups, and table rows are 
not affected by this patch.

Comment 55

[Bernd]

fantasai: Could you comment, why we should exclude the rows and include the row
groups, I would prefer to get rid of the whole quirk stuff, that seems to me
more consistent.
Could you also comment about the attachment 38472 [details], is this the expected rendering?

Comment 56

[fantasai]

As I said in bug 4510:
  NavQuirks behavior should be kept for <tr bgcolor="x">, though, since all 
  versions of MSIE also render it that way. (And Opera, too.)

As David Baron stated, the purpose of Quirks mode is to emulate Quirks that page 
authors depend on and not those they don't. They generally don't use table rows 
groups or columns, and Nav4 didn't even support those. (If you look at the 
comments in nsHTMLStyleSheet.cpp, the main reason for having quirky behavior for 
table row groups was for simplicity of implementation.) However, setting 
backgrounds on table rows is common practice, and it behaves in a quirky manner 
in all the major browsers since MSIE introduced the capability in version 2.0. 
Therefore we need to emulate that behavior.

>Could you also comment about the attachment 38472 [details], is this the expected 
rendering?

Your testcase uses two green backgrounds on top of each other, so I can't tell. 
I'll attach another testcase in a moment.


BTW, that patch was even more rotten than either of us realized. Notice that the 
TableBackgroundRule no longer exists--that's what I was targetting when I 
modified the if statement in nsHTMLStyleSheet.cpp. I'm looking through Hyatt's 
changes (2001-05-31) now, and I recommend putting this fix on hold for a bit.

Comment 57

[fantasai]

Attachment: Extended Table Background Testcase (Quirks)

Comment 58

[fantasai]

Attachment: new patch

Comment 59

[fantasai]

I was not able to regression test the patch because Viewer kept locking up when 
I tried to run rtest.sh baseline. I'm going to try wiping out my mozilla 
directory again and downloading & compiling over the weekend.

I tested against the Extended testcase above and against David Baron's three 
table background tests in bug 4510--results were as expected.

Comment 60

[Bernd]

Just a short quote from the CSS2 spec:

17.6.1 The separated borders model

In this model, each cell has an individual border. The 'border-spacing' property
specifies the distance between the borders of adjacent cells. 


*This space is filled with the background of the table element*.


 Rows, columns, row groups, and column groups cannot have borders (i.e., user
agents must ignore the border properties for those elements).

I think what IE is doing is just right, they inherit the color into the table
cell. I think we should do the same. This would require from fantasai's patch
only the nsTableFrame.cpp part in order to paint the cellspacing with the table
backgrounds color.

I have to admit that the CSS2 spec is not conflict free in this matter and as
one can see by the David's posting
http://lists.w3.org/Archives/Public/www-style/1999Jul/0083.html 
and the answers, the W3C is not able create a clear specification over two years
now. So it is hard to believe that this will change very soon.

The honest interpretation of the conflict is that for the collapsed border model
 as  default the space between cell's should be covered by table- , row- , col-
etc background, while for the separated border model it deviates from the
default model and only the table background is used for the space between the
cells. 

In term's of testing I prefer now to check backgrounds.html file in
layout/tests/table/core against IE and the strict behaviour in order to get a
first impression on the fix and the remaining problems, and only to run the
regression tests when this looks fine.

Comment 61

[fantasai]

Bernd, please copy your comments into bug 4510. This bug is quirks-specific--
we're not changing standard mode rendering here.

> This would require from fantasai's patch only the nsTableFrame.cpp part in
> order to paint the cellspacing with the table backgrounds color.

Even if we decide to change standard mode to imitate quirks, the layout mode 
check still needs to go.

Comment 62

[fantasai]

*** Bug 84653 has been marked as a duplicate of this bug. ***

Comment 63

[Bernd]

fantasai: I dont think that the last bug is really a dupe of this bug it is more
a dupe of bug 41635. BTW IE renders this testcase fine and they do not need the
paint stuff.

Comment 64

[fantasai]

Attachment: final patch (I hope)

Comment 65

[fantasai]

Changes only to nsTableFrame.cpp and quirk.css, as requested.

And now, I request review. =)

Comment 66

[Bernd]

I'll do the review + testing

quote from the code reviewers guide:

'Remember, a code reviewer is not a friend. A good code reviewer can feel like
your worst enemy.' 

:-)

Comment 67

[Bernd]

here you go fantasai: r=bernd

Comment 68

[Marc Attinasi]

sr=attinasi

Comment 69

[Bernd]

fix checked in

Comment 70

[Pierre Saslawsky]

Reopening because I think it caused the regression at bug 91034.

Comment 71

[Pierre Saslawsky]

Closing as Fixed again.  After all, it's not related to bug 91034.

Comment 72

[karnaze (gone)]

Reopening to mark topembed.

Comment 73

[karnaze (gone)]

patch was checked into the m0.9.2 branch. 

Comment 74

[Jerry Baker]

Mass removing self from CC list.

Comment 75

[Jerry Baker]

Now I feel sumb because I have to add back. Sorry for the spam.