Bug 83978 – CSS 2.1 failure: table-columns-example-001 fails

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Robert Hogan]

rules=cols needs to be fixed.

[Robert Hogan]

Created attachment 137241 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 137241 [details]
Patch

Attachment 137241 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12410257

[Build Bot]

Comment on attachment 137241 [details]
Patch

Attachment 137241 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12400993

[Robert Hogan]

Created attachment 137243 [details]
Patch

[Julien Chaffraix]

Comment on attachment 137243 [details]
Patch

My big question here: what does other browsers do if there is both the 'rules' attribute and some explicit border on the cells? Do they also disable or display the border?

The spec is pretty silent on a lot of those cases and I really wonder if you are not just working around the default cell's style for all borders that enables some borders you don't want.

[Robert Hogan]

(In reply to comment #5)
> (From update of attachment 137243 [details])
> My big question here: what does other browsers do if there is both the 'rules' attribute and some explicit border on the cells? Do they also disable or display the border?

They disable it too.

> 
> The spec is pretty silent on a lot of those cases and I really wonder if you are not just working around the default cell's style for all borders that enables some borders you don't want.

Could you elaborate on this? I don't quite get you.

[Julien Chaffraix]

>> My big question here: what does other browsers do if there is both the 'rules' attribute and some explicit border on the cells? Do they also disable or display the border?

> They disable it too.

To be frank, your change makes our code way worse and this behavior doesn't strike me as obvious. Is this behavior standardized anywhere? (AFAICT HTML4 is silent about the interaction and HTML5 doesn't even mention the attribute "rules").

> > The spec is pretty silent on a lot of those cases and I really wonder if you are not just working around the default cell's style for all borders that enables some borders you don't want.
> 
> Could you elaborate on this? I don't quite get you.

Sure, HTMLTableElement has a default shared style for all cells (see HTMLTableElement::createSharedCellStyle). It is setting the right borders for a cell inside the table but doesn't do the right thing for the cells on the rim (or frame to use HTML4's wording). I was just wondering if this change wasn't going against that.

[Robert Hogan]

Created attachment 137570 [details]
Patch

[Robert Hogan]

Comment on attachment 137570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137570&action=review

> Source/WebCore/html/HTMLTableElement.cpp:429
> +    if ((!m_borderAttr && !m_borderColorAttr) || m_frameAttr) {

Oh, just realized this isn't right if the frame attribute is set.

[Robert Hogan]

Created attachment 137604 [details]
Patch

[Julien Chaffraix]

Comment on attachment 137604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137604&action=review

It looks really much better, thanks!

> Source/WebCore/html/HTMLTableElement.cpp:429
> +    if ((!m_borderAttr && !m_borderColorAttr) || m_frameAttr) {

I think the m_frameAttr case should be singled out as you are basically doing:

if (m_frameAttr)
   return 0;

> Source/WebCore/html/HTMLTableElement.cpp:430
> +        if (!m_frameAttr && (m_rulesAttr == ColsRules || m_rulesAttr == RowsRules || m_rulesAttr == AllRules)) {

Shouldn't you also check for m_rulesAttr == GroupsRules? I don't see this tested and would bet our testing is lacking...

If I am right here, I think the check should be moved to an helper methods: like rulesIsSetAndNotNone().

> LayoutTests/ChangeLog:18
> +        * fast/css/table-rules-attribute-with-frame1-expected.png: Added.
> +        * fast/css/table-rules-attribute-with-frame1-expected.txt: Added.
> +        * fast/css/table-rules-attribute-with-frame1.html: Added.
> +        * fast/css/table-rules-attribute-with-frame2-expected.png: Added.
> +        * fast/css/table-rules-attribute-with-frame2-expected.txt: Added.
> +        * fast/css/table-rules-attribute-with-frame2.html: Added.

I really looks like those could be ref-tests as we don't care about the DRT dump. Could it be possible to compare to the equivalent border-collapse: separate case?

> LayoutTests/css2.1/20110323/table-columns-example-001.htm:9
> +        <meta name="assert" content="CSS can be used to emmulate the HTML 'rules = cols' attribute (example from section 17.3).">

Typo 'emmulate', was that in the original test case?

> LayoutTests/fast/css/table-rules-attribute-with-frame1.html:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

We usually prefer the HTML5 doctype for new tests:

<!DOCTYPE html>

> LayoutTests/fast/css/table-rules-attribute-with-frame1.html:10
> +        <meta name="assert" content="The first table has four vertical lines capped and bottomed by two horizontal lines.">
> +        <meta name="assert" content="The second table has 2 vertical lines.">
> +        <meta name="assert" content="The third table has 2 vertical lines capped by a horizontal line.">
> +        <meta name="assert" content="The fourth table has 2 vertical lines bottomed by a horizontal line.">
> +        <meta name="assert" content="The fifth table has 3 vertical lines.">
> +        <meta name="assert" content="The sixth table has 3 vertical lines.">

That's quite a neat way to embed the expected result.

[Julien Chaffraix]

Comment on attachment 137604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137604&action=review

Per discussion, more comments on the test cases. I checked the output and agrees with Robert that they are what is expected.

> LayoutTests/fast/css/table-rules-attribute-with-frame1.html:15
> +                height: 2em;
> +                width: 2em;

em is dependent on the font size, doesn't that make your test platform-dependent?

> LayoutTests/fast/css/table-rules-attribute-with-frame2.html:12
> +                height: 2em;
> +                width: 2em;

Same question here.

> LayoutTests/fast/css/table-rules-attribute.html:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

DOCTYPE.

> LayoutTests/fast/css/table-rules-attribute.html:12
> +            #id1

This should likely be a class as you now have several elements with the same id. Also .greenBorder would be way more precise to me.

> LayoutTests/fast/css/table-rules-attribute.html:16
> +            #id2

Same here.

> LayoutTests/fast/css/table-rules-attribute.html:77
> +            </tr>

Would be neat to test rules="groups".

[Robert Hogan]

Created attachment 137949 [details]
Patch

[Julien Chaffraix]

Comment on attachment 137949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137949&action=review

The code is fine. But talking the whole ref tests with another reviewer, we have the same feeling that the tests you are adding should be ref-tests. For a few reasons:
* it is simple to have a reference (using the border-collapse: separate equivalent of what you expect)
* you don't care about the tree dump
* it's smaller and we get less potential baselines.

> Source/WebCore/html/HTMLTableElement.cpp:435
> +        if (m_rulesAttr) {

I prefer to do the full comparison: m_rulesAttr != UnsetRules.

> LayoutTests/css2.1/20110323/table-columns-example-001.htm:23
> +                height: 2em;
> +                width: 2em;

Again, why do those needs to be ems? I fail to see the need but it may just be me being thick.

[Robert Hogan]

(In reply to comment #14)
> > LayoutTests/css2.1/20110323/table-columns-example-001.htm:23
> > +                height: 2em;
> > +                width: 2em;
> 
> Again, why do those needs to be ems? I fail to see the need but it may just be me being thick.

Nope - that's just me missing your comment - also this particular example is a suite test. So I can fix it in the others but not here.

[Robert Hogan]

Created attachment 138233 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138233 [details]
Patch

Attachment 138233 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12467500

New failing tests:
css2.1/20110323/table-columns-example-001.htm

[WebKit Review Bot]

Created attachment 138235 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Robert Hogan]

Created attachment 138239 [details]
Patch

[Dave Hyatt]

Comment on attachment 138239 [details]
Patch

r=me

[Julien Chaffraix]

Comment on attachment 138239 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138239&action=review

In case you were waiting on me, the patch is cool and should go in.

> LayoutTests/ChangeLog:27
> +          These test the correct interaction of a rules attribute with a frame attribute. 'cols' is used
> +          for the test, but each of cols, rows, and all have the same effect on the table's borders. It wasn't
> +          possible to create reftests for all of them because of the different behaviour of the frame attr to 
> +          a simple border, probably due to painting order.

Too bad all tests cannot be reftests. Thanks for patching some!

[Robert Hogan]

Committed r115091: <http://trac.webkit.org/changeset/115091>

[Robert Hogan]

For rebaselines see: https://bugs.webkit.org/show_bug.cgi?id=84748