46480 - (Quirks) Color not initializing to 'inherit' within tables

Status: RESOLVED FIXED

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

Description

[fantasai]

Overview:

	In Quirks mode, the 'color' property is not inherited into <tbody>, 
	<tr>, or <td> unless "color: inherit" is explicitly specified. 
	Apparently, this is done to simulate Navigator 4.x, which does not 
	inherit color through tables. Although the quirk that blocks color 
	inheritance from the container into the table may be necessary, I don't 
	think that inheritance should be blocked in any part of the chain 
	linking <table>, <tbody>, <tr>, and <td>. Applying a color style to any
	of these elements has no effect in Navigator, so I don't think any sites 
	will depend on it not working; there's no point in specifying a color in 
	the first place. However, MSIE 5 supports intra-table color inheritance
	(I haven't tested IE4), and I'm sure sites take advantage of that.

Steps to Reproduce:

	Open up testcase (to be attached shortly) in Mozilla.

Actual Results:

	Color only inherits if /every single element/ between the assigned color 
	property and the table cell has 'style="color: inherit"'. This includes 
	<tbody>, whether it physically appears or not.

Expected Results:

	Color should inherit properly through the table tags.

Tested on Mozilla nightly build (id: 2000071810) on Windows 2000

Additional Information:

	Bug detected by Matt P <[e-mail]> on July 26th build.

Comment 1

[fantasai]

Attachment: testcase

Comment 2

[mpetrick]

Attachment: Test Case 2

Comment 3

[mpetrick]

css properties not being inherited:
color, background-repeat, .....im sure there are others

Comment 4

[fantasai]

Copying Matt P's comments from n.p.m.style post:
 | does not work on:
 | Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; m17) Gecko/20000725

'background-repeat' does not inherit. 

CSS1:5.3.4 - http://www.w3.org/TR/REC-CSS1#background-repeat
CSS2:14.2.1 - http://www.w3.org/TR/REC-CSS2/colors.html#background-properties

Comment 5

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

Would it be better to just make the backgrounds in quirks mode work the same
waythey do in strict mode?  That is, just paint the background on all table
elements...

There's another bug on this somewhere in the 4000's or 5000's.

Also, this is CSS2, not CSS1, since CSS1 did not describe tables.

Comment 6

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

Ignore my previous comment.  I saw the background-repeat stuff and thought the
bug was about background-color, not color.  Sorry.

Comment 7

[fantasai]

This form here needs an undo button...
Many apologies; I didn't mean to do that.

Comment 8

[karnaze (gone)]

Reassigning to pierre.

Comment 9

[fantasai]

This bug is caused by the line marked below, which should probably only be 
invoked on <table> and not any of the other tags.

(layout/html/style/src/nsHTMLStyleSheet.cpp)

      // add the quirks background compatibility rule if in quirks mode
      // and/or the Table TH rule for handling <TH> differently than <TD>
      else if ((tag == nsHTMLAtoms::td) ||
               (tag == nsHTMLAtoms::th) ||
               (tag == nsHTMLAtoms::tr) ||
               (tag == nsHTMLAtoms::thead) || // Nav4.X doesn't support row
                                              // groups, but it is a lot
               (tag == nsHTMLAtoms::tbody) || // easier passing from the table
                                              // to rows this way
               (tag == nsHTMLAtoms::tfoot)) {
        // add the rule to handle text-align for a <th>
        if (tag == nsHTMLAtoms::th) {
          aResults->AppendElement(mTableTHRule);
        }
        nsCompatibility mode;
        aPresContext->GetCompatibilityMode(&mode);
        if (eCompatibility_NavQuirks == mode) {
          if (mDocumentColorRule) {
!>>         aResults->AppendElement(mDocumentColorRule);
          }
          aResults->AppendElement(mTableBackgroundRule);
        }
      }

I believe it was added to fix bug 1044.

Comment 10

[Amarendra Hanumanula]

QA contact update

Comment 11

[Håkan Waara]

I investigated some and it seems like this isn't belonging to Quirks:

http://www.w3.org/TR/html4/struct/tables.html#h-11.2.6 - tells you that tables
doesn't have any implicit color= attribute, as someone suggested.

http://www.w3.org/TR/REC-CSS1#color - supports that all color attributes, should
be color: inherit;

Given this, I will attach a patch that fixes this bug, and adds the necessary
code to html.css

Need r= and sr=...

Comment 12

[Håkan Waara]

Attachment: fix v1

Comment 13

[fantasai]

Color should not be set to 'inherit' in the html stylesheet. It is and should be 
done internally by the style system. The inheritance within tables is merely 
overridden by nsHTMLStyleSheet (which also overrides the rules in your patch).

> I investigated some and it seems like this isn't belonging to Quirks:

Mozilla inherits color properly in Standard mode, so...
What do you mean by "this"?

Comment 14

[Bernd]

This bug seems to be related to bug 70831

Comment 15

[Håkan Waara]

Here comes a fix for it in nsHTMLStyleSheet.cpp

According to Bernd Mielke this patch also fixes bug 70831. But I feel that the
code change is quite risky, so I would appreciate if as many as possible looks
at the code and/or tries it on their build.

fantasai, does this do what you want?

Comment 16

[Håkan Waara]

Attachment: fix v2

Comment 17

[fantasai]

> fantasai, does this do what you want?

No, the mTableBackgroundRule is for something else.

I have a patch that fixes this bug; it just doesn't pass the regression tests. 
(I haven't gotten around to sorting through the results.) If you want, I'll send 
it to you. There's no reason for you to waste time guessing how I expect to fix 
this. :)

BTW, I'm getting the impression that you are not testing your fixes. 
Are you able to compile Mozilla?

Comment 18

[Håkan Waara]

Yes, you can send it to me and yes (of course!) I compile and run my fixes.

Comment 19

[fantasai]

> and yes (of course!) I compile and run my fixes.

And test them, too, I hope.
That CSS fix had no effect on my copy of Mozilla, so I'd like
to know what changed in the rendering of the testcase to make 
it seem that the bug had been fixed on your computer.

Comment 20

[Bernd]

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=28736 creates a problem
when
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug6933.html
is viewed with the viewer and the standard DTD is selected. -> no go for this
patch :-(

Comment 21

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

How could attachment 28736 [details] [diff] [review], which moves a line out of an "if (quirks mode)", fix
bug 70831, which is quirks-mode only?

Comment 22

[fantasai]

Attachment: patch

Comment 23

[fantasai]

Attachment: table regression test results [edited]

Comment 24

[fantasai]

Attachment: quirk test [text inside both tables should be blue]

Comment 25

[fantasai]

> WONTFIX ? -- minor quirks mode unquirk

This is not unquirking a quirk. It's fixing a bug in the implementation of a 
quirk.

Comment 26

[fantasai]

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

Comment 27

[Bernd]

Attachment: wfm WIN98

Comment 28

[fantasai]

Attachment: Testcase with workaround for quirk regression

Comment 29

[fantasai]

There is a regression in the behavior of the table color quirk (I haven't filed 
it yet), but this bug still exists. Please use the testcase I have just 
attached.

The patch needs to be updated to account for Hyatt's changes. I'll try to do 
that in the next few days.

Comment 30

[fantasai]

Attachment: new patch

Comment 31

[fantasai]

Requesting reveiw.

Comment 32

[Marc Attinasi]

So, the patch just limits the application of the DocumentColorRule to the table
element, right? (wanna make sure I'm not missing something subtle here)

Comment 33

[fantasai]

Precisely. Unless I've somehow mangled the if statements to do otherwise.. :)

Comment 34

[fantasai]

Bernd asked me to explain exactly what this patch does. So I'm first going to 
explain what the affected code does, and then what I've changed.

mDocumentColorRule holds the color of the <body> element. There is a table 
NavQuirk that causes tables to use the color of the <body> element, so to 
imitate that in Mozilla, the mDocumentColorRule is applied to the table in 
quirks mode.

Specifically, the mDocumentColorRule was applied to <tbody>, <thead>, <tfoot>, 
<tr>, <td>, and <th> instead of just to the <table>. This was probably because 
it was a convenient place to put it since another rule for handling table 
backgrounds was applied to all these elements.

Setting a table color on <table> did not inherit into the table cell because the 
color got reset in the table row group and again in the table row and again in 
the table cell. Notice that in the testcase, the color does inherit properly if 
one explicitly specifies inherit for all these table elements.

The first patch I attached moved the mDocumentColor rule out of the table 
elements if check into a separate <table> check. When Hyatt checked in his style 
 performance fix on 2001-05-31, along with other changes he removed the table 
background rule from nsHTMLStyleSheet.cpp and put it in quirk.css. Since the 
table elements if check is no longer needed for the table background rule or the 
Quirk color rule, I have removed it. The <th> rule is moved out to its own 'else 
if', and the Quirk <table> color rule is likewise moved out.

The only other thing I changed was the order of if statements for the Quirk 
rule. It checks for the mDocumentColorRule before making calls to find the 
layout mode. That is all.

Any questions?

Comment 35

[Marc Attinasi]

[s]r=attinasi - thanks!

Comment 36

[Bernd]

Attachment: ie rendering of the testcase

Comment 37

[Bernd]

Attachment: mozilla rendering of the inheritance testcase

Comment 38

[Bernd]

Attachment: ooops now comes the ie rendering

Comment 39

[Bernd]

I dont know how 'nörgeln'  translates into english, but I know how to do it.
In the first table the border is black instead of green. I think it should be
green as the div specifies color:green. 
from css2:
If an element's border color is not specified with a border property, user
agents must use the value of the element's 'color' property as the computed
value for the border color.
 

Comment 40

[fantasai]

The table's border is black because the table's 'color' is black. As I've
explained above, Quirks mode breaks color inheritance on the table, using the
<body>'s color. Since the <body>'s 'color' is black, the table's color is black.

If I specify "table {color: inherit}", the border will be green, and the color
will inherit through the table as described by CSS (if you have the patch
applied).

Comment 41

[Bernd]

r = bernd

Comment 42

[Bernd]

fix checked in