Bug 83978

Created attachment 137241 [details] Patch

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

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

Created attachment 137243 [details] Patch

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.

(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.

>> 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.

Created attachment 137570 [details] Patch

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.

Created attachment 137604 [details] Patch

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 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".

Created attachment 137949 [details] Patch

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.

(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.

Created attachment 138233 [details] Patch

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

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

Created attachment 138239 [details] Patch

Comment on attachment 138239 [details] Patch r=me

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!

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

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