Bug 83978

Summary: CSS 2.1 failure: table-columns-example-001 fails
Product: WebKit Reporter: Robert Hogan <robert@webkit.org>
Component: CSSAssignee: Robert Hogan <robert@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: darin@apple.com, dglazkov@chromium.org, gns@gnome.org, hyatt@apple.com, jchaffraix@webkit.org, robert@webkit.org, webkit.review.bot@gmail.com, xan.lopez@gmail.com
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 From 2012-04-14 05:49:01 PST
rules=cols needs to be fixed.
------- Comment #1 From 2012-04-15 12:02:10 PST -------
Created an attachment (id=137241) [details]
Patch
------- Comment #2 From 2012-04-15 12:11:59 PST -------
(From update of attachment 137241 [details])
Attachment 137241 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12410257
------- Comment #3 From 2012-04-15 12:23:55 PST -------
(From update of attachment 137241 [details])
Attachment 137241 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12400993
------- Comment #4 From 2012-04-15 13:18:46 PST -------
Created an attachment (id=137243) [details]
Patch
------- Comment #5 From 2012-04-16 11:28:58 PST -------
(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?

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 From 2012-04-16 12:11:30 PST -------
(In reply to comment #5)
> (From update of attachment 137243 [details] [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 From 2012-04-16 15:18:10 PST -------
>> 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 From 2012-04-17 11:47:56 PST -------
Created an attachment (id=137570) [details]
Patch
------- Comment #9 From 2012-04-17 14:09:39 PST -------
(From update of attachment 137570 [details])
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 From 2012-04-17 14:32:32 PST -------
Created an attachment (id=137604) [details]
Patch
------- Comment #11 From 2012-04-18 09:14:07 PST -------
(From update of attachment 137604 [details])
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 From 2012-04-18 21:32:09 PST -------
(From update of attachment 137604 [details])
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 From 2012-04-19 12:25:29 PST -------
Created an attachment (id=137949) [details]
Patch
------- Comment #14 From 2012-04-19 16:55:03 PST -------
(From update of attachment 137949 [details])
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 From 2012-04-20 12:22:59 PST -------
(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 From 2012-04-21 07:53:44 PST -------
Created an attachment (id=138233) [details]
Patch
------- Comment #17 From 2012-04-21 08:23:57 PST -------
(From update of attachment 138233 [details])
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 From 2012-04-21 08:24:03 PST -------
Created an attachment (id=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 From 2012-04-21 09:48:01 PST -------
Created an attachment (id=138239) [details]
Patch
------- Comment #20 From 2012-04-21 14:21:50 PST -------
(From update of attachment 138239 [details])
r=me
------- Comment #21 From 2012-04-24 11:50:09 PST -------
(From update of attachment 138239 [details])
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 From 2012-04-24 12:05:04 PST -------
Committed r115091: <http://trac.webkit.org/changeset/115091>
------- Comment #23 From 2012-04-24 12:43:04 PST -------
For rebaselines see: https://bugs.webkit.org/show_bug.cgi?id=84748