573960 - bordercolor for table is not used as grid color anymore

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Daniel Kirsch]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; Win64; x64; en-US; rv:1.9.3a6pre) Gecko/20100622 Minefield/3.7a6pre
Build Identifier: 

The latest nightly doesn't handle the bordercolor attribute of the table element for rendering the grid color anymore. However the border gets the defined color.

While this is not part of the html specification and the similar bug 46265 was marked as invalid this used to work in all versions I know and does so in all other major browsers (safari 5, chrome 5, ie 8) except opera (10.5).

Reproducible: Always



Expected Results:  
grid color should be identical to the border color defined by the bordercolor attribute.

Comment 1

[Daniel Kirsch]

Attachment: Example

Comment 2

[Henri Sivonen (:hsivonen)]

Confirmed on Linux 64, but this isn't an HTML parser bug.

Comment 3

[Daniel Kirsch]

Attachment: Example

Corrected the wrong example.

The old example shows the same behaviour in old versions too. So while this shows the different behaviour compared to other browsers (border is red, grid is black) it doesn't show any difference to other Gecko versions.

The difference seems to be caused when the "rules" attribute is used. The new example shows this. In old (FF 3.6 and before) versions and Chrome, Safari, IE all borders of the example are red. In latest versions (3.7) the borders are all black.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Would be great to get the regression range for this bug.

Comment 5

[Daniel Kirsch]

Works:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100109 Minefield/3.7a1pre
6a7294d0f305

Broken:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100110 Minefield/3.7a1pre
537a710037d4

See
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6a7294d0f305&tochange=537a710037d4

Must be a regression of bug 43178

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #5)
> Must be a regression of bug 43178

Looks like to be the only changeset in that area.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The HTML spec needs to define the behavior here.

Comment 8

[Daniel Kirsch]

(In reply to comment #7)
> The HTML spec needs to define the behavior here.

This might be difficult as the HTML spec doesn't define a bordercolor attribute for a table element. AFAIK this was a MS invention which was adopted by other browsers.

Comment 9

[fantasai]

Should be easy to fix. Just need to set 'border-color: inherit' on all the internal table elements in html.css.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> This might be difficult as the HTML spec doesn't define a bordercolor attribute

Sure it does.  http://dev.w3.org/html5/spec/Overview.html#fonts-and-colors has this to say if you read it:

  When a table element has a bordercolor attribute, its value is expected to be
  parsed using the rules for parsing a legacy color value, and if that does not
  return an error, the user agent is expected to treat the attribute as a
  presentational hint setting the element's 'border-top-color',
  'border-right-color', 'border-bottom-color', and 'border-right-color'
  properties to the resulting color.

Which is what we do.  The current whatwg draft has the same language.

So in fact we implement what the spec says here (see the rest of the stylesheets in the "Punctuation and decorations" section).  Now maybe the spec needs changing...

Comment 11

[fantasai]

Attachment: bordercolor test scatter

So, behavior here is pretty inconsistent. Opera doesn't support bordercolor afaict. The two other issues are color inheritance and border style:
  - IE does inherit the color, but not when 'border-color' is set in CSS.
    Safari inherits in both cases.  Konqueror does not inherit the color.
    I think we should follow Safari here since it's easier and makes at
    least as much sense.
  - IE, Safari, and Konqueror all set the border style to solid when
    bordercolor is in effect. We (even in previous versions) do not.

I think the best thing to do here would be to implement border-color
inheritance through internal table elements, clean up the style rules
to be more CSS-friendly and to accommodate solidification, and submit
our UA style rules to the HTML5 spec.

Comment 12

[Daniel Kirsch]

Attachment: Screenshot of different browsers for test "bordercolor test scatter"

The screenshot shows the "bordercolor test scatter" test in different browsers.

Firefox 3.6.4
IE 9 Dev Preview (identical to IE 8)
Safari 5.0
Chrome 5.0
Opera 10.54

Comment 13

[fantasai]

Thanks very much for that screenshot!

Comment 14

[Daniel Kirsch]

You're welcome :-)

Interesting to see not only the different behaviour in Opera but also the huge space between each table in Firefox compared to other browsers.

Total height of all tables in px:
Firefox: 320
Chrome/Safari: 288
IE: 245
Opera: 250

Comment 15

[philippe (part-time)]

(In reply to comment #14)
 
> Interesting to see not only the different behaviour in Opera but also the huge
> space between each table in Firefox compared to other browsers.
Vertical margins don't collapse in Gecko - that is bug 87277 I believe

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving to betaN - requires beta coverage, no specific beta milestone targetted at this time.

Comment 17

[Daniel Kirsch]

(In reply to comment #9)
> Should be easy to fix. Just need to set 'border-color: inherit' on all the
> internal table elements in html.css.

I've tested it using the following:

td,tr,tbody,thead {
  border-color:inherit;
}

at least for my case this seems to fix the issue.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK now all you have to do is roll that into a patch to html.css so dbaron can review it :-)

Comment 19

[Daniel Kirsch]

OK, I will try that. :-)

Comment 20

[Daniel Kirsch]

Attachment: Example including the CSS fix

The table should contains all visible elements of a table. It seems to be necessary to define a CSS definition for all of these elements to let the fix work.

I will now create the patch adding the following lines to html.css:

tr,td,th,thead,tbody,tfoot {
  border-color:inherit;
}

Comment 21

[Daniel Kirsch]

Attachment: Patch

Patch as described.

Comment 22

[fantasai]

Comment on attachment 501870 [details] [diff] [review]
Patch

Given that the other rules that apply to internal table elements are grouped by selector rather than grouped by declaration, I think you should assign border-color: inherit; to the existing style rule blocks in that section rather than adding a new style rule. r- due to that reason, unless dbaron disagrees.

(Also note that this rule will not work on tables with the rules attribute due to the way existing style rules are written for [rules], so while correct, it is not a complete fix.)

Comment 23

[Daniel Kirsch]

I thought it would be much clearer to have that fix at one single place only; to have less code (otherwise I need to add it for each declaration) and better separation between bug/workaround and feature. Do you think my solution might cause slower performance or is it a coding style or another reason?

What about the comment. Should I place a comment on each additional assignment about the bugfix? 

My latest testcase includes a "rules" attribute and it seems to work. May you provide a testcase to show what you mean with "not a complete fix"?

Comment 24

[Daniel Kirsch]

Attachment: Patch using existing declarations

@fantasai
I've included border-color: inherit; for the existing definitions. I don't like the hint to the bug on each assignment. Maybe one single hint is enough? I would definitely mention it at least once to avoid confusion.
Please check this new patch and give me some feedback. Thanks.

Comment 25

[fantasai]

(Sorry, thought I'd replied already. Must've forgotten to hit Submit.)

Yeah, one comment should be enough. I'd make it /* inherit bordercolor attribute */ instead of /* Fix bug 573960 */, though, since that directly explains why it's there instead of requiring investigation into what bug 573960 is. (If the bug number is needed for some reason, it can be retrieved from the Mercurial commit logs.)

Your latest testcase places the inherit rule at the author level, which allows it to override the UA-level rules in html.css. When the rules move to html.css, they will no longer override the 'rules' attribute style rules. Specifically, the 'border' shorthand used in the [rules] style rules resets 'border-color' at a higher specificity than your inherit rules, which means on such tables they will not work.

Comment 26

[Daniel Kirsch]

Attachment: Patch

I've now included a border-color:inherit definition to all relevant places. The "bordercolor test scatter" file now looks fine with it.

Topics to look at:
- The bordercolor for td elements doesn't inherit from the color attribute anymore when using this patch. Firefox used to inherit it (see the blue border in attachment 453683 [details]). IMHO this change is ok as it is what a developer expects and no other browser showed the color value as attribute when a bordercolor attribute was given.
- Do we also need a definition in the general "table[rules]" rule? I don't think so as the following definitions should already handle it.
- In my tests I've seen that the borderstyle for the table element is outset when the bordercolor attribute is used. This wasn't the case in previous releases (was solid) and isn't the case for webkit and IE (solid as well). I will file a separate bug.

Comment 27

[fantasai]

Comment on attachment 504563 [details] [diff] [review]
Patch

>   /* Put this first so 'border' and 'frame' rules can override it. */
> table[rules] { 
>-  border: thin hidden; 
>+  border: thin hidden;
> }
...
> table:-moz-table-border-nonzero { 
>   border: thin outset; 
>+  border-color: inherit; /* inherit bordercolor attribute */
> }
...
> table[frame] {
>   border: thin hidden;
>+  border-color: inherit;
> }

The table element shouldn't be inheriting border-color from its parent.

...
> table:-moz-table-border-nonzero > th
> {
>   border: thin inset;
>+  border-color: inherit;
> }
...
> table[rules]:not([rules=""])> th
> {
>   border: thin none;
>+  border-color: inherit;
> }
...
> table[rules][rules="none"] > th
> {
>   border: thin hidden;
>+  border-color: inherit;
> }
...
> {
>-  border: thin solid ;
>+  border: thin solid;
>+  border-color: inherit;
> }
> 
> table[rules][rules="rows"] > tr,
> table[rules][rules="rows"] > * > tr {
>   border-top: thin solid;
>   border-bottom: thin solid;
>+  border-color: inherit;
> }
...
>   border-left: thin solid;
>   border-right: thin solid;
>+  border-color: inherit;
...
> table[rules][rules="groups"] > tfoot,
> table[rules][rules="groups"] > thead,
> table[rules][rules="groups"] > tbody {
>   border-top: thin solid;
>   border-bottom: thin solid;
>+  border-color: inherit;
> }

Instead of adding border-color: inherit everywhere, I think it makes more sense not to use the 'border' shorthand -- to replace it with 'border-width' and 'border-style' statements, which don't reset the color.

So I would suggest
  a) adding 'border-width: thin' to the general declarations for thead, tfoot, tbody, tr, td, and th.
  b) switching the [rules]-related declarations to use 'border-style' in place of 'border'.

Comment 28

[Daniel Kirsch]

Attachment: Screenshot including the patch

This is a screenshot of the "bordercolor test scatter" attachment including the patch. See the border-style set to "outset". (This is actually not caused by the patch but was included before already).

For the border-style outset change I filled bug 626524.

Comment 29

[Daniel Kirsch]

Attachment: Patch

New patch including the suggested changes.

Please also take a look at comment 26 and the "Topics to look at".

Comment 30

[fantasai]

Comment on attachment 504574 [details] [diff] [review]
Patch

   /* Put this first so 'border' and 'frame' rules can override it. */
 table[rules] { 
-  border: thin hidden; 
+  border: thin hidden;
 }

This looks like the stray removal of a space. Undo that change (yes, we don't like trailing spaces, but no, we should not cause unrelated diff churn to remove them), and r=fantasai!

Thanks very much for working on this.

Comment 31

[Daniel Kirsch]

Attachment: Patch

Reverted trailing space.
Added space between "border-color:" and "inherit".

Thanks for your patience and your feedback. The first patch is probably the hardest ;-)

Comment 32

[Daniel Kirsch]

Another difference to previous versions:
When using "frame=none" on the table element, the table doesn't have a border any more where in previous versions it had one. See example G in the "bordercolor test scatter" attachment. IMHO this change is intended and what I expect as a developer.

Comment 33

[fantasai]

Comment on attachment 504713 [details] [diff] [review]
Patch

Congrats! This is ready for checkin.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW can we have a reftest here?

Comment 35

[Boris Zbarsky [:bzbarsky]]

I tried checking this in, and had to back it out for test failures:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/bugs/320979-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-anonymous-boxes/156888-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-anonymous-boxes/338735-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-5.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-6.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-7.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-8.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-frame-border.html | image comparison (==)

My guess is that the border colors are slightly different between testcases and references there, for the most part.  Certainly the differing pixels are in the borders.

Comment 36

[Daniel Kirsch]

(In reply to comment #35)
> REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/bugs/320979-1.html
> | image comparison (==)

bug 320979 attachment 206421 [details]
No bordercolor attribute is given but the CSS border-color for the table. The grid color should then be inherited from the CSS color value of the td element.

Suggestions? Maybe use inherit only when the bordercolor attribute is given?

> REFTEST TEST-UNEXPECTED-PASS |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-anonymous-boxes/156888-1.html
> | image comparison (==)

I'm not sure about this test. Looking at the source, it uses a style "display:inline" on a td element. Changing that to "display:table-cell" looks fine for me.
What does TEST-UNEXPECTED-PASS really mean?
Not clear what to do here as the whole behaviour (changing the whole table layout because of a single td element with display:inline) is pretty weird for me.


> REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-anonymous-boxes/338735-1.html
> | image comparison (==)

Similar case here. The testcase uses display="block" and display="" on a td element. The behaviour changed due to my patch.


> REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-5.xhtml
> | image comparison (==)

I can not see any visual difference.


> REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-6.xhtml
> | image comparison (==)

Visually indentical with borderhandling-5.xhtml (previous test). I can not see any visual difference.
 

> REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-7.xhtml
> | image comparison (==)

Visually indentical with borderhandling-5.xhtml. I can not see any visual difference.


> REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-8.html
> | image comparison (==)

Visually indentical with borderhandling-5.xhtml. I can not see any visual difference.


> REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-bordercollapse/borderhandling-frame-border.html
> | image comparison (==)

I can not see any visual difference.

I'm going to check the changes for the first 3 cases and try to find a solution, however I could need some support on this. All other cases doesn't show any change for me.

Comment 37

[Daniel Kirsch]

(In reply to comment #36)
> bug 320979 attachment 206421 [details]
> No bordercolor attribute is given but the CSS border-color for the table. The
> grid color should then be inherited from the CSS color value of the td element.

BTW: webkit shows the same behaviour in that case and has a red grid too.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> No bordercolor attribute is given but the CSS border-color for the table. 

Perhaps the test needs changing, if we think the behavior it's testing for is not desirable?

> Looking at the source, it uses a style "display:inline" on a td element.

Yes, that's the point of the test.

> What does TEST-UNEXPECTED-PASS really mean?

That the test is currently marked as failing, but after this patch starts passing.  This is generally a good thing, if you expected it to happen.... if not, worth figuring why the behavior changed in a case where you didn't expect it to change.  One thing worth looking into is why the test is marked as failing to start with.

> The behaviour changed due to my patch.

OK, was that expected and desired?

> I can not see any visual difference.

But the test harness can?  Can you examine the pixel values in the resulting images in an image editor to see what's going on?

Comment 39

[Daniel Kirsch]

Attachment: Color inheritance testcase

Testcase to look at the color inheritance for border-color.

Comment 40

[Daniel Kirsch]

Attachment: Screenshot for color inheritance testcase

The screenshot shows the different browser behavior. Minefield 4b10 trunk, Chrome 8 (identical to Safari), IE 9 beta, Opera 10.63.

Comment 41

[Daniel Kirsch]

(In reply to comment #38)
> Perhaps the test needs changing, if we think the behavior it's testing for is
> not desirable?

IMHO the test is OK in regard of the referenced bug. It raises a simple question: Do we really want to inherit border-color by default inside a table?
If yes it would also cause a change in color inheritance when only a CSS "color" value is given. For usual HTML elements the "color" value also affects the border-color if no explicit border-color is given. However for the table elements the "color" value would no longer be used as border-color.

The new "Color inheritance testcase" (attachment 506267 [details]) shows the current situation. Attachment 506268 [details] shows a comparison to other browsers. IE 9 shows the same behaviour as the current implementation. Chrome shows the behaviour identical to the suggested patch (at least for the color inheritance).

What shall we do? And if we want to keep the current inheritance, how can we fix this bug then?


> One thing worth looking into is why the test is marked as
> failing to start with.

I'm trying...


> OK, was that expected and desired?

No. But the whole behaviour there seems to be buggy and unclear and fragile. (But that's actually beyond this bug.)


> But the test harness can?  Can you examine the pixel values in the resulting
> images in an image editor to see what's going on?

I will.

Comment 42

[Daniel Kirsch]

Just looked at the latest IE release and they seemed to have changed that as well. The grid color now is identical to what we have.

When looking at the "bordercolor test scatter" testcase and thinking about color inheritance the longer I test the issue the more I think the current behavior is correct.

And: It only happens for a very special case and it's unclear how we should fix this without causing other changes. We might mark this "INVALID" or "WONTFIX". What do you think?

BTW: I've compared the images from the borderhandling-5.xhtml doc in Photoshop and they are absolutely identical as far as I can see and my color picker can tell. Is there any app to show differences in two images? Might be a nice task for a HTMLCanvas app :-)

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See layout/tools/reftest/reftest-analyzer.xhtml

Comment 44

[fantasai]

To inherit only when bordercolor is set, you could factor out the border-color: inherit rules as

table[bordercolor] > tbody,
table[bordercolor] > thead,
table[bordercolor] > tfoot,
table[bordercolor] > tr,
table[bordercolor] > td,
table[bordercolor] > th {
  border-color: inherit;
}

That might make the most sense.

Comment 45

[Daniel Kirsch]

(In reply to comment #44)
> To inherit only when bordercolor is set, you could factor out the border-color:

I thought about that too, but it would always inherit the border color and would not use a CSS "color" value if given for a td element. Do we want this? And what, if the CSS border-color is given as well?

Given the following example

<table border=1 bordercolor=red>
<tr>
  <td style="color:blue">Cell1</td>
</tr>
</table>

What color should the border of the <td> element have? Red, because of the bordercolor attribute, or blue because of the color value?

Do we want a different behavior if we use the bordercolor attribute or the border-color CSS value?

Comment 46

[Daniel Kirsch]

(In reply to comment #43)
> See layout/tools/reftest/reftest-analyzer.xhtml

May you give me some more info on this? I don't know how to use it and what for.

Comment 47

[Boris Zbarsky [:bzbarsky]]

You open that page, then select the reftest log file in the file input.  It gives you a list of tests, with the failed ones linkified.  You click the link, and can examine both of the images involved, as well as have it circle the pixels that are different between the two.

Comment 48

[Daniel Kirsch]

I don't have a reftest log file.
As far as I understand, I need a test build and run the reftest. Right? Well, I cannot create a custom build (tried it several times with no success). I've found http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/firefox-4.0b10pre.en-US.win32.tests.zip but don't know what to do with it.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> I don't have a reftest log file.

http://tinderbox.mozilla.org/Firefox/1295412270.1295414022.19171.gz should be a log of the Windows XP reftest run with your failures in it.

http://tinderbox.mozilla.org/Firefox/1295405508.1295406417.13491.gz should be the equivalent Linux run.

http://tinderbox.mozilla.org/Firefox/1295409222.1295409977.32071.gz the Mac run.

Hope that helps!

Comment 50

[Daniel Kirsch]

Attachment: Color inheritance testcase 2

Comment 51

[Daniel Kirsch]

Attachment: Patch

Patch based on comment 44. Just to test the behavior and define a basis for further discussion.
I had to change some of the rules definitions to get it working.

Comment 52

[Daniel Kirsch]

Attachment: Screenshots of "Color inheritance testcase 2"

These screenshots show the "Color inheritance testcase 2" in FF 3.6, current nighlty (4b10) and the 4b10 including the patch.

Comment 53

[Daniel Kirsch]

(In reply to comment #49)
> Hope that helps!

Yes, thanks. However I still don't see the differences in the images. :-/

Comment 54

[Boris Zbarsky [:bzbarsky]]

Well.  But the analyzer tells you which pixels to look at, right?  Does your image editor also not see a difference between those pixels?

Comment 55

[Daniel Kirsch]

Comparing both images from the reftest in the editor, the gray color of the outer and inner border is slightly different.
Comparing the images taken at my machine with and without the patch, I cannot see any difference.
Both my images are slightly different in brightness compared to the images from the reftest.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Sounds exciting.  ;)

Comment 57

[Daniel Kirsch]

You cannot imagine the fun I have. "Should be easy to fix. Just need to set 'border-color: inherit'". Well...

Comment 58

[fantasai]

(In reply to comment #45)
> What color should the border of the <td> element have? Red, because of the
> bordercolor attribute, or blue because of the color value?

Red. The author is expecting bordercolor to set the colors of the table borders, and color to set the color of the text. (If he'd set 'border-color' rather than 'color', then he would expect the CSS to set the border color.)

Comment 59

[Daniel Kirsch]

OK, this sounds comprehensible.

If that's what we want, the latest patch might be worth your review.

I've checked my patch against the failed tests (see comment 35) and they seem to work now. I still don't have any idea for the "borderhandling" documents and the strange border color change. I cannot reproduce it locally.

Please check the new testcase (attachment 506391 [details]) and the screenshots (attachment 506397 [details]). If the result is what you expect, we might check it.

Comment 60

[fantasai]

Comment on attachment 506393 [details] [diff] [review]
Patch

I'd leave the border-width rules factored out as before, since I don't think the default width should fluctuate depending on our exact ua.css selectors, but otherwise it looks good to me.

Comment 61

[Daniel Kirsch]

(In reply to comment #35)
> REFTEST TEST-UNEXPECTED-PASS |
> file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/table-anonymous-boxes/156888-1.html
> | image comparison (==)

I think I know why this passed. The reference test creates a border with a td element using style="display:inline". This causes all td elements to change their border style to inset. The testcase itself sets the initial style to "display:none" and changes the style by script. This doesn't cause the border style to change. However my first patch caused a change of the border style resulting in the layout as defined in the reference. 
This was not intended.

Comment 62

[Daniel Kirsch]

(In reply to comment #60)
> I'd leave the border-width rules factored out as before

Do you mean using "border: thin none;" instead of "border-width:thin;border-style:none" aso?
I've tried it using only the table[bordercolor] definitions, but the [rules] definitions prevented border color inheritance. I've tried a couple of different things and found this to be a working solution with as little changes as necessary. What do you think?

Comment 63

[Boris Zbarsky [:bzbarsky]]

Hmm.  Your patch doesn't change the border-style, right?  The bug covering that failing test is bug 484825, fwiw.

Comment 64

[fantasai]

I mean just border-width: thin; to td, th, tr, etc. without qualification, as you did in your previous patch.

Comment 65

[Daniel Kirsch]

(In reply to comment #63)
> Hmm.

What do you mean?

> Your patch doesn't change the border-style, right?  

Right.

> The bug covering that failing test is bug 484825, fwiw.

Thanks

Comment 66

[Daniel Kirsch]

(In reply to comment #64)
> I mean just border-width: thin; to td, th, tr, etc. without qualification, as
> you did in your previous patch.

Ah well, that caused some of the unwanted behavior. So I would better leave the current logic as is. It seems to be a bit fragile.

But we might remove some of the border-width definitions from the patch IMHO.

Comment 67

[Boris Zbarsky [:bzbarsky]]

> What do you mean?

Mostly that I have no idea offhand what's going on there.  ;)

Comment 68

[Daniel Kirsch]

@fantasai
My suggestion:
Remove the border-width for the table[rules][rules="XXX"] definitions (only for "none", "all", "rows" and "cols"). The border-width should be inherited by the table[rules]:not([rules=""]) definition.

What do you think?

Comment 69

[Daniel Kirsch]

Just for my understanding:
In which circumstance will the following rule be evaluated:

table > tr {
  border-color:inherit;
}

I think a tbody element will be created automatically so this must read:

"table > tbody > tr" or "table > * > tr".

My tests seem to second that but the source is full of definitions using both. Why is the first case necessary? 
(We could remove a couple of lines in html.css if this is not necessary.)

Comment 70

[fantasai]

(In reply to comment #68)
border-width doesn't inherit, so that wouldn't work. Your patch as it stands isn't wrong, btw. If it passes reftests, it can be checked in. I *think* it would make more sense if our UA rules just assigned a thin border-width to all table elements, just because it's a more predictable behavior, but that's enough of an edge case that it doesn't really matter right now.

(In reply to comment #69)

The TBODY element is added during HTML parsing, but not during XHTML parsing (because XHTML can't have parse-time magic). So the table > tr rule will be evaluated in an XHTML <table><tr><td>...</td></tr></table>.

Comment 71

[Daniel Kirsch]

(In reply to comment #70)
> border-width doesn't inherit, so that wouldn't work.

Sorry, it doesn't inherit but is already defined by previous definitions. eg:

 table[rules]:not([rules=""])> tr > td,
 table[rules]:not([rules=""])> * > tr > td,
 table[rules]:not([rules=""])> tr > th,
 table[rules]:not([rules=""])> * > tr > th,
 table[rules]:not([rules=""])> td,
 table[rules]:not([rules=""])> th
 {
-  border: thin none;
+  border-width: thin;
+  border-style: none;
 }
 
 
 table[rules][rules="none"]  > tr > td,
 table[rules][rules="none"] > * > tr > td,
 table[rules][rules="none"] > tr > th,
 table[rules][rules="none"] > * > tr > th,
 table[rules][rules="none"] > td,
 table[rules][rules="none"] > th
 {
-  border: thin hidden;
+  border-style: hidden;
 }

border-width is defined by the previous definition so IMO it's not necessary here anymore.

btw. Do we need additional definitions for "table[bordercolor] > td" and "table[bordercolor] > th"? The above definition has it and so do others. But what for? Is it valid (X)HTML or just an automated correction for wrong (X)HTML?


> If it passes reftests, it can be checked in.

As I am currently not able to do the reftest, may someone please check this? 


> I *think* it
> would make more sense if our UA rules just assigned a thin border-width to all
> table elements, just because it's a more predictable behavior

Yes. You may file a separate bug for this.


> because XHTML can't have parse-time magic

Thanks for the explanation.

Comment 72

[(no longer active)]

Temporarily assigning to myself as I'm taking care of landing this.

Comment 73

[(no longer active)]

So, some reftests are still failing here.  <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1296277307.1296278827.18690.gz>.  Investigating...

Comment 74

[(no longer active)]

The reftest failures are caused because of this quirks mode style: <http://mxr.mozilla.org/mozilla-central/source/layout/style/quirk.css?force=1#100>.  This causes the default border color to be gray in quirks mode, and black in standards compliance mode.

I'll submit a new patch.

Comment 75

[(no longer active)]

Attachment: Patch (v2)

I fixed the failing reftest, and actually made us use the reftest which we had half written to make sure that this doesn't regress in the future.

Comment 76

[Bernd]

Sorry for late kicking in, but real life does take its toll.
The half written test was checked in by me by accident without any review sorry for this. 
I believe that the patch should not have got r+ without reftests. IMHO the recovery of those 2 tests is clearly not sufficient. There are plenty of good test cases in this bug already accumulated they should be converted into reftests to nail down the behavior. As a side effect the reftests are tested by the patch author and not as just before checkin.

Comment 77

[Bernd]

Comment on attachment 508161 [details] [diff] [review]
Patch (v2)

Further  attachment 508161 [details] [diff] [review] makes borderhandling-rules-border.html to fail consistently rather than very seldom (due to slowness on test machines).

The borderhandling-rules-border.html testcase reveals that this patch does not color the border of the columns and colsgroups which is used by the rules="col".

r- as it will regress the rules="all" and rules="col" cases.

Comment 78

[(no longer active)]

(In reply to comment #77)
> Further  attachment 508161 [details] [diff] [review] makes borderhandling-rules-border.html to fail
> consistently rather than very seldom (due to slowness on test machines).

I'm not really sure how this patch would have any effect on how often borderhandling-rules-border.html fails.  Do you mean it has a performance impact?  Right now the test is marked as random, so I don't think we have any good measure of how often it fails...

Thanks for catching the @rules problems.  I'll fix them and will submit a new patch for you to review.

Comment 79

[Bernd]

Attachment: patch (v2) revised

>I am not sure how this patch would have any effect....
just remove the random in the reftest.list file and watch for pixel differences

see http://mxr.mozilla.org/mozilla-central/source/layout/reftests/table-bordercollapse/borderhandling-rules-border.html
it makes the rules grid partially gray. Especially the rules="rows" case makes it obvious that the patch has a problem

The addition of the black color diverts the tests from what they should test the default colors, the strict tests should compare against a strict ref.

Comment 80

[Bernd]

>Right now the test is marked as random, so I don't think we have any
>good measure of how often it fails...
It fails so very seldom that I never got it on my machine, so that I could fix it. I know the test "personally", I wrote it.

Comment 81

[(no longer active)]

(In reply to comment #80)
> >Right now the test is marked as random, so I don't think we have any
> >good measure of how often it fails...
> It fails so very seldom that I never got it on my machine, so that I could fix
> it. I know the test "personally", I wrote it.

OK, thanks for explaining the problem.  The cause of the problem was that borderhandling-rules-border-ref.html used the border shorthand property, which overrides the border-color: gray property coming from quirk.css.  I switched both files to standards mode to fix this problem.  This change will be included in my next version of the patch, soon to be submitted here.

Comment 82

[Bernd]

>I switched both files to standards mode to fix this problem
I doubt that this is correct, the patch should work both in quirks and in standards mode. So the correct solution would be to add a standards mode testcase. The rendering in quirks mode of the rules=rows case with black and gray color is not acceptable.

Comment 83

[(no longer active)]

(In reply to comment #77)
> The borderhandling-rules-border.html testcase reveals that this patch does not
> color the border of the columns and colsgroups which is used by the
> rules="col".
> 
> r- as it will regress the rules="all" and rules="col" cases.

Actually, I don't think that you were right here.  I opened the testcase manually in a patched build, and I can't see anything broken there.  You might have been looking at the wrong patch or something?

I also added a bunch of new reftests.  I'll attach my updated patch soon.

Comment 84

[(no longer active)]

(In reply to comment #82)
> >I switched both files to standards mode to fix this problem
> I doubt that this is correct, the patch should work both in quirks and in
> standards mode. So the correct solution would be to add a standards mode
> testcase.

There is nothing wrong with the rendering in quirks mode.  It's just a difference between the colors.  In standards mode, we default the table border color to black.  In the quirks mode, we default it to gray.  Therefore, a reftest which tests a quirks page to a standards page will always fail *unless* we explicitly declare all border colors (which is what I had done in attachment 508161 [details] [diff] [review]).  What you did in attachment 508330 [details] [diff] [review] was putting the pages in standards mode.

> The rendering in quirks mode of the rules=rows case with black and
> gray color is not acceptable.

I'm not sure what you mean by unacceptable.

The reason that the rules used to be rendered in black was this rule: <http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#347> which explicitly set the border color to black.  Because the rule was more specific, it won over the rule in quirk.css.  Do you mean that you want to preserve this behavior?  If that's the case, then we can just revert your change to that rule in the patch, but I'm not sure that's what we really want...

Comment 85

[(no longer active)]

Attachment: Patch (v3)

Comment 86

[Bernd]

Ehsan, I am concerned about the quirks mode situation, making the test strict does not solve the problem there it rather hides it. I have split up the border - rules test case to remove the randomness and will attach it soon.

Comment 87

[(no longer active)]

(In reply to comment #86)
> Ehsan, I am concerned about the quirks mode situation, making the test strict
> does not solve the problem there it rather hides it. I have split up the border
> - rules test case to remove the randomness and will attach it soon.

I re-read comment 82, and I'm still not sure what the desired behavior should be.  Let me summarize what happens right now with the patch:

If there is a bordercolor attribute on the table element, we end up using that as the "default color".  Otherwise, we set the "default color" to gray in quirks mode, and to the computed CSS color property in standards mode.  This "default color" is inherited to td and th elements inside tables in both standards and quirks mode, and is possible to override by using a CSS border-color property on the aforementioned td/th element.

Please let me know what should change in this behavior.  But as long as we have a quirks mode default color difference, test cases such as data:text/html,<table border><tr><td>foo</table> and data:text/html,<!DOCTYPE html><table border><tr><td>foo</table> are not going to be rendered the same...

Comment 88

[Bernd]

Attachment: quirks rendering

Comment 89

[Bernd]

I attached a split up of the disabled (random) test to bug 540360 which tests both scenarios, quirks and strict (the patch there is against trunk not on top of this bug)

Comment 90

[(no longer active)]

So, I have a patch which makes the quirks mode rendering of all rules=xxx types similar to regular borders.  I did this by setting border-color on all table related elements in quirks mode, and inheriting the border-color on colgroup elements in standards mode (in addition to the rest of the elements).

Here are the changes compared to my current patch (for ease in review): http://pastebin.mozilla.org/1019536

I'll clean up the patch and will submit it for review shortly.

Comment 91

[(no longer active)]

Spoke too soon.  Need to style col elements too, because they can be styled by content CSS to have borders!  Crrrazy!  :-)

With that, borderhandling-rules-border passes cleanly!

Comment 92

[(no longer active)]

Attachment: Patch (v4)

This should address everything, hopefully.

Comment 93

[(no longer active)]

I splitted borderhandling-rules-border on top of this patch in bug 540360.  I think that we need to take that patch together with this one for test coverage.  Adding a dependency based on that.

Comment 94

[(no longer active)]

bernd: ping?

Comment 95

[Bernd]

Comment on attachment 509547 [details] [diff] [review]
Patch (v4)

pong, that is what I want, reftests in abundance, no tweaking away of the quirks testcases, looks good to me.

Comment 96

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/152fe0de3c82