Bug 83978

Summary: CSS 2.1 failure: table-columns-example-001 fails
Product: WebKit Reporter: Robert Hogan <robert>
Component: CSSAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, gns, hyatt, jchaffraix, robert, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 47141    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch hyatt: review+

Description Robert Hogan 2012-04-14 05:49:01 PDT
rules=cols needs to be fixed.
Comment 1 Robert Hogan 2012-04-15 12:02:10 PDT
Created attachment 137241 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2012-04-15 12:11:59 PDT
Comment on attachment 137241 [details]
Patch

Attachment 137241 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12410257
Comment 3 Build Bot 2012-04-15 12:23:55 PDT
Comment on attachment 137241 [details]
Patch

Attachment 137241 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12400993
Comment 4 Robert Hogan 2012-04-15 13:18:46 PDT
Created attachment 137243 [details]
Patch
Comment 5 Julien Chaffraix 2012-04-16 11:28:58 PDT
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.
Comment 6 Robert Hogan 2012-04-16 12:11:30 PDT
(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.
Comment 7 Julien Chaffraix 2012-04-16 15:18:10 PDT
>> 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.
Comment 8 Robert Hogan 2012-04-17 11:47:56 PDT
Created attachment 137570 [details]
Patch
Comment 9 Robert Hogan 2012-04-17 14:09:39 PDT
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.
Comment 10 Robert Hogan 2012-04-17 14:32:32 PDT
Created attachment 137604 [details]
Patch
Comment 11 Julien Chaffraix 2012-04-18 09:14:07 PDT
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.
Comment 12 Julien Chaffraix 2012-04-18 21:32:09 PDT
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".
Comment 13 Robert Hogan 2012-04-19 12:25:29 PDT
Created attachment 137949 [details]
Patch
Comment 14 Julien Chaffraix 2012-04-19 16:55:03 PDT
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.
Comment 15 Robert Hogan 2012-04-20 12:22:59 PDT
(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.
Comment 16 Robert Hogan 2012-04-21 07:53:44 PDT
Created attachment 138233 [details]
Patch
Comment 17 WebKit Review Bot 2012-04-21 08:23:57 PDT
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
Comment 18 WebKit Review Bot 2012-04-21 08:24:03 PDT
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
Comment 19 Robert Hogan 2012-04-21 09:48:01 PDT
Created attachment 138239 [details]
Patch
Comment 20 Dave Hyatt 2012-04-21 14:21:50 PDT
Comment on attachment 138239 [details]
Patch

r=me
Comment 21 Julien Chaffraix 2012-04-24 11:50:09 PDT
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!
Comment 22 Robert Hogan 2012-04-24 12:05:04 PDT
Committed r115091: <http://trac.webkit.org/changeset/115091>
Comment 23 Robert Hogan 2012-04-24 12:43:04 PDT
For rebaselines see: https://bugs.webkit.org/show_bug.cgi?id=84748