| Summary: | CSS 2.1 failure: border-conflict-element-021a | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Robert Hogan <robert> | ||||||||||||||||
| Component: | CSS | Assignee: | Arpita Bahuguna <arpitabahuguna> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | arpitabahuguna, darin, eric, gustavo, jchaffraix, philn, pravind.2k4, robert, vijayan.bits, webkit.review.bot, xan.lopez | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| URL: | http://www.gtalbot.org/BrowserBugsSection/css21testsuite/border-conflict-element-021a.html | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Robert Hogan
2012-05-18 12:49:29 PDT
Hi Arpita, You've shown some interest in border-collapsing so if you want to take this please assign it to yourself so we know you're working on it! Otherwise I'll have a look myself later.. (In reply to comment #1) > Hi Arpita, > > You've shown some interest in border-collapsing so if you want to take this please assign it to yourself so we know you're working on it! Otherwise I'll have a look myself later.. Sure Robert.. will definitely work on this issue but am afraid I don't have "editbugs" bit set for my ID to assign this issue to myself. Would appreciate if you could assign this issue to me. Even otherwise I shall look into this bug and try to come up with a patch as soon as possible. This issue is not as much to do with the collapsed before border (with a section above) being computed incorrectly than to do with the sequence in which the collapsed borders are painted. As is visible, the before border for body does have the blue color (head's or the previous row group's after border given precedence), it's only at the corners wherein the diff occurs which is due to the left and the right borders being drawn after the top and bottom borders thereby causing the overlap. The reason why this same does not occur with the tbody and tfoot is because in the test page tfoot has been specified before tbody and thus is drawn before tbody. If in the test page the tfoot were to be moved after the tbody, same issue would arise between these two row-groups as well. Keeping the order in which the collapsed borders paint over each other in the same cell intact probably we need to look at the order in which the row-groups/sections are painted; starting from the bottom to the top?? Created attachment 143781 [details]
Patch
Uploaded patch as per the aforementioned comment. Comment on attachment 143781 [details]
Patch
Very nice. Looks good to me. You should ping jchaffraix as he is a reviewer.
Comment on attachment 143781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143781&action=review Nice change, some comments. > Source/WebCore/rendering/RenderTable.cpp:595 > + for (RenderObject* child = lastChild(); child; child = child->previousSibling()) While this change is good, I would really love for us to iterate directly over RenderTableSection here. You would need to add the following function though: RenderTableSection* lastSection() const; and the iterate using sectionAbove(). > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:2 > +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> Please, let's use the HTML5 doctype: <!DOCTYPE html> > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:7 > +<title>Bugzilla Bug: 86885. CSS Test: Border conflict resolution - adjacent table row groups with same border-style and border-width. Expected result.</title> Just remove the title (see below why). > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:20 > +<p>When two adjacent table row groups (thead, tbody, tfoot) have the same border-width and the same border-style in a 'border-collapse: collapse' table, then the color of the border from the topmost table row group wins: so, a thead wins over a tbody which wins over tfoot.</p> > + The long explanation is nice but the following more basic information are useful and should also be present (in the output as this is a ref-test, not in a non-dumped <title> tag): * bug id, bonus point for having a clickable form. * bug title * what are the condition for the test to pass. > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:23 > + > + Please remove those unneeded empty lines (not repeated). Also let's add the missing section. > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:25 > + <td class="t1"> </td> Nit: You don't strictly need any text here and could just specify the size of your cells in CSS: td { width: 100px; height: 100px; } (text has several issues in test cases that makes me avoid it as much as I can) > LayoutTests/fast/table/border-collapsing/adjacent-row-groups.html:16 > + thead {border-color: blue;} > + tbody {border-color: yellow;} > + tfoot {border-color: orange;} A good reading on test case is the trac page about that: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Generaltips I usually prefer a red / green test: everything that shouldn't be shown should be red and everything that should be shown should be green. Here it would give the following rules: thead, tbody, tfoot { border: 1px solid green; border-top: 1px solid red; } thead { border-top: 1px solid green; } Created attachment 144341 [details]
Patch
(In reply to comment #7) Thanks for the review Julien. Apologize for the delay in uploading the patch though. Have incorporated most of the changes suggested by you in this patch. Just a few variances: > (From update of attachment 143781 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143781&action=review > > Nice change, some comments. > > > Source/WebCore/rendering/RenderTable.cpp:595 > > + for (RenderObject* child = lastChild(); child; child = child->previousSibling()) > > While this change is good, I would really love for us to iterate directly over RenderTableSection here. You would need to add the following function though: > > RenderTableSection* lastSection() const; > > and the iterate using sectionAbove(). > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:2 > > +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> > > Please, let's use the HTML5 doctype: > > <!DOCTYPE html> > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:7 > > +<title>Bugzilla Bug: 86885. CSS Test: Border conflict resolution - adjacent table row groups with same border-style and border-width. Expected result.</title> > > Just remove the title (see below why). > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:20 > > +<p>When two adjacent table row groups (thead, tbody, tfoot) have the same border-width and the same border-style in a 'border-collapse: collapse' table, then the color of the border from the topmost table row group wins: so, a thead wins over a tbody which wins over tfoot.</p> > > + > > The long explanation is nice but the following more basic information are useful and should also be present (in the output as this is a ref-test, not in a non-dumped <title> tag): > * bug id, bonus point for having a clickable form. > * bug title > * what are the condition for the test to pass. > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:23 > > + > > + > > Please remove those unneeded empty lines (not repeated). Have incorporated the aforementioned changes. > > Also let's add the missing section. > If I add the sections i.e. thead etc. to the expected testcase then it starts behaving in the exact same way as the actual test case. Since my expected testcase should show the desired result even without the fix, have desisted from adding the sections to the expected page. > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:25 > > + <td class="t1"> </td> > > Nit: You don't strictly need any text here and could just specify the size of your cells in CSS: > > td { > width: 100px; > height: 100px; > } > > (text has several issues in test cases that makes me avoid it as much as I can) > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups.html:16 > > + thead {border-color: blue;} > > + tbody {border-color: yellow;} > > + tfoot {border-color: orange;} > > A good reading on test case is the trac page about that: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Generaltips > > I usually prefer a red / green test: everything that shouldn't be shown should be red and everything that should be shown should be green. Here it would give the following rules: > > thead, tbody, tfoot { > border: 1px solid green; > border-top: 1px solid red; > } Setting the border-top as red does not have any effect as the "before" border of the section above gets higher priority thereby painting all the borders in green color. > thead { > border-top: 1px solid green; > } Since the actual differing regions are the overlapping corners of the borders alone it was difficult to obtain the desired output (i.e. only to have the corners paint in red). Have thus made the border-color of the middle section red which brings out the difference in the corners. Between the tbody and thead sections the red border-color of the tbody section overlaps the green of thead and between the tfoot and the tbody sections the green border-color of the tfoot overlaps the red of tbody. Comment on attachment 144341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144341&action=review I want to say r=me but I am troubled by the naming. > Source/WebCore/rendering/RenderTable.cpp:595 > + for (RenderTableSection* section = lastSection(); section; section = sectionAbove(section)) { The mix of names here, lastSection with sectionAbove, seems wrong. The new function should be named bottomSection if it works with sectionAbove, or lastSection if it works with sectionBefore. > Source/WebCore/rendering/RenderTable.cpp:1117 > + if (m_firstBody) { Is the null check of m_firstBody an important optimization? > Source/WebCore/rendering/RenderTable.cpp:1120 > + for (RenderObject* section = lastChild(); section; section = section->previousSibling()) > + if (section->isTableSection() && section != m_head) > + return toRenderTableSection(section); In WebKit coding style, this for statement should have braces. Comment on attachment 144341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144341&action=review r- due to the multiple headers / footers issue. > Source/WebCore/ChangeLog:3 > + CSS 2.1 failure: border-conflict-element-021a Btw, I would expect to see this test added or unskipped as part of this change. > Source/WebCore/rendering/RenderTable.cpp:597 > + static_cast<RenderObject*>(section)->paint(info, childPoint); No static_cast please, if you can't call paint because paint() is private, you should make it public (don't forget to annotate the function with OVERRIDE as you move it). > Source/WebCore/rendering/RenderTable.cpp:1122 > + if (m_foot) > + return m_foot; > + if (m_firstBody) { > + for (RenderObject* section = lastChild(); section; section = section->previousSibling()) > + if (section->isTableSection() && section != m_head) > + return toRenderTableSection(section); > + } > + return m_head; m_foot, m_firstBody, m_head are the top (ie first) sections of this type. We allow several footers and headers to be inserted in our tree so this means this function will be totally broken in this case. The fact that it is making no test fail means that our testing is lacking and should be improved. > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:12 > + .red { border-color: red; } As said, I would rather reserve 'red' for failures. In this case, use another color as this makes the output confusing for someone unfamiliar with the test. Created attachment 144782 [details]
Patch
Comment on attachment 144782 [details] Patch Attachment 144782 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12841970 Created attachment 144785 [details]
Patch
(In reply to comment #10) Thanks for the review Darin. Have uploaded another patch with the specified changes. > (From update of attachment 144341 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144341&action=review > > I want to say r=me but I am troubled by the naming. > > > Source/WebCore/rendering/RenderTable.cpp:595 > > + for (RenderTableSection* section = lastSection(); section; section = sectionAbove(section)) { > > The mix of names here, lastSection with sectionAbove, seems wrong. The new function should be named bottomSection if it works with sectionAbove, or lastSection if it works with sectionBefore. Have changed the name to bottomSection(). > > > Source/WebCore/rendering/RenderTable.cpp:1117 > > + if (m_firstBody) { > > Is the null check of m_firstBody an important optimization? > > > Source/WebCore/rendering/RenderTable.cpp:1120 > > + for (RenderObject* section = lastChild(); section; section = section->previousSibling()) > > + if (section->isTableSection() && section != m_head) > > + return toRenderTableSection(section); > > In WebKit coding style, this for statement should have braces. Have modified this function. (In reply to comment #11) Julien, have uploaded another patch with changes as suggested by you. Please review the same. > (From update of attachment 144341 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144341&action=review > > r- due to the multiple headers / footers issue. > > > Source/WebCore/ChangeLog:3 > > + CSS 2.1 failure: border-conflict-element-021a > > Btw, I would expect to see this test added or unskipped as part of this change. border-conflict-element-021a currently does not exist in our css2.1 testsuite. So in all probability shall have to add the same to the testsuite. Am sorry but am unaware on how to go about doing this since the only place where I found the test is at http://www.gtalbot.org/BrowserBugsSection/css21testsuite/border-conflict-element-021a.html Is it okay to copy the test-case directly from this site? > > > Source/WebCore/rendering/RenderTable.cpp:597 > > + static_cast<RenderObject*>(section)->paint(info, childPoint); > > No static_cast please, if you can't call paint because paint() is private, you should make it public (don't forget to annotate the function with OVERRIDE as you move it). Have taken care of this; paint() has been made public for RenderTableSection. > > > Source/WebCore/rendering/RenderTable.cpp:1122 > > + if (m_foot) > > + return m_foot; > > + if (m_firstBody) { > > + for (RenderObject* section = lastChild(); section; section = section->previousSibling()) > > + if (section->isTableSection() && section != m_head) > > + return toRenderTableSection(section); > > + } > > + return m_head; > > m_foot, m_firstBody, m_head are the top (ie first) sections of this type. We allow several footers and headers to be inserted in our tree so this means this function will be totally broken in this case. > > The fact that it is making no test fail means that our testing is lacking and should be improved. > Have modified this function. If m_foot is present, return that else iterate using lastChild() and return the first RenderTableSection (from the bottom). The multiple tfoot problem will not arise with this change since the first tfoot encountered in the table is marked as m_foot. Following tfoot(s) (if any) are simply added as sibling(s) to the previous section. Now we always render this m_foot as the bottom-most section irrespective of whether any other tfoot was specified in the table after this. Thus for tfoot (if specified), m_foot would point to the last section of the table. > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:12 > > + .red { border-color: red; } > > As said, I would rather reserve 'red' for failures. In this case, use another color as this makes the output confusing for someone unfamiliar with the test. Comment on attachment 144785 [details] Patch Attachment 144785 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12842863 Comment on attachment 144785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144785&action=review Please add the requested testing, this would make me confident that you are not breaking already working code. Without those tests, I don't think we should accept the patch as it's too easy to break painting. > Source/WebCore/ChangeLog:3 > + CSS 2.1 failure: border-conflict-element-021a My take would be to add the test as part of this change. You should grab the change from the official test suite, not one of the test author's website. Robert knows where it is if you can't find it. > Source/WebCore/rendering/RenderTable.cpp:1118 > + if (m_foot) > + bottomSection = m_foot; This is still wrong. All the pointers are to the *first* section of its kind, including m_foot. Note that sectionBelow seems to have the same logical flaw. If you had added some testing as requested previous for the multiple sections of the same type you would have seen an issue here. > Source/WebCore/rendering/RenderTable.h:195 > + RenderTableSection* bottomSection() const; Note that bottom / top are in device coordinates and the naming don't play well with writing-mode (which is supported on table). As we are not consistent in our naming here, this shouldn't be a show-stopper. > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:6 > +<title>Bugzilla Bug 86885: CSS 2.1 failure: border-conflict-element-021a. Expected result.</title> Unneeded title. > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:7 > +<style type="text/css"> Unneeded type attribute. > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:27 > + Can you remove the unneeded space? (In reply to comment #6) > (From update of attachment 143781 [details]) > Very nice. Looks good to me. You should ping jchaffraix as he is a reviewer. Hi Robert. Would really appreciate if you could help me get the border-conflict-element-021a testcase or point me in the right direction. I did search in the official test suite: http://test.csswg.org/suites/css2.1/20110323/ but was only able to get border-conflict-element-021. Created attachment 145295 [details]
Patch
(In reply to comment #18) Thanks for the review Julien, have uploaded another patch. > (From update of attachment 144785 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144785&action=review > > Please add the requested testing, this would make me confident that you are not breaking already working code. Without those tests, I don't think we should accept the patch as it's too easy to break painting. > > > Source/WebCore/ChangeLog:3 > > + CSS 2.1 failure: border-conflict-element-021a > > My take would be to add the test as part of this change. You should grab the change from the official test suite, not one of the test author's website. Robert knows where it is if you can't find it. > I was unable to locate border-conflict-element-021a on the official test suite: http://test.csswg.org/suites/css2.1/20110323/. Have requested Robert to help me with the same. > > Source/WebCore/rendering/RenderTable.cpp:1118 > > + if (m_foot) > > + bottomSection = m_foot; > > This is still wrong. All the pointers are to the *first* section of its kind, including m_foot. Note that sectionBelow seems to have the same logical flaw. > > If you had added some testing as requested previous for the multiple sections of the same type you would have seen an issue here. > Have added a new ref test adjacent-row-groups-multi.html which consists of a border-collapsed table with 2 theads and 2 tfoots. WebKit's rendering in this scenario is the same as is on other browsers such as FF, Opera and I.E. The first tfoot encountered is the last section to be rendered. Thus going by that logic m_foot which points to the first tfoot specified within the table would still be the last section. While iterating over the DOM nodes the order would be like head1, head2, body, foot1, foot2 but if you see in terms of sections then the ordering is like: head1, head2, body, foot2, foot1. This behavior seems to be in accordance with other browsers as well. > > Source/WebCore/rendering/RenderTable.h:195 > > + RenderTableSection* bottomSection() const; > > Note that bottom / top are in device coordinates and the naming don't play well with writing-mode (which is supported on table). As we are not consistent in our naming here, this shouldn't be a show-stopper. > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:6 > > +<title>Bugzilla Bug 86885: CSS 2.1 failure: border-conflict-element-021a. Expected result.</title> > > Unneeded title. > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:7 > > +<style type="text/css"> > > Unneeded type attribute. > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:27 > > + > > Can you remove the unneeded space? Have made the changes as suggested in the layout testcases. Also, since I currently do not have a mac at my disposal I was unable to figure out the reason for the build failure on mac for my previous patch. Would really appreciate it if you could help me out with that. I grabbed the testcase from here: http://lists.w3.org/Archives/Public/public-css-testsuite/2012May/0022.html It's now in the nightly unstable branch of the test suite: http://test.csswg.org/suites/css2.1/nightly-unstable/html4/border-conflict-element-021a.htm (In reply to comment #21) > > Also, since I currently do not have a mac at my disposal I was unable to figure out the reason for the build failure on mac for my previous patch. Would really appreciate it if you could help me out with that. It builds cleanly now. You can click through to the output from the build button - in this case there's nothing revealing there so looks unrelated to your patch. Comment on attachment 145295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145295&action=review Sorry for the drive-by comments - I hope these iterations aren't doing your head in, they are a fact of life with rendering code unfortunately. > Source/WebCore/rendering/RenderTable.cpp:1118 > + if (m_foot) > + bottomSection = m_foot; You should return early here instead of branching. > Source/WebCore/rendering/RenderTable.cpp:1121 > + for (child = lastChild(); child && !child->isTableSection(); child = child->previousSibling()) { } This looks like it should be a 'while' clause that returns when it finds a TableSection. Comment on attachment 145295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145295&action=review >> Source/WebCore/rendering/RenderTable.cpp:1121 >> + for (child = lastChild(); child && !child->isTableSection(); child = child->previousSibling()) { } > > This looks like it should be a 'while' clause that returns when it finds a TableSection. It doesn't have to be a 'while' clause btw - but an empty 'for' clause definitely looks wrong here. (In reply to comment #22) > I grabbed the testcase from here: > > http://lists.w3.org/Archives/Public/public-css-testsuite/2012May/0022.html > > It's now in the nightly unstable branch of the test suite: > > http://test.csswg.org/suites/css2.1/nightly-unstable/html4/border-conflict-element-021a.htm Thanks so much for this! I'll get the testcase from here.. (In reply to comment #24) > (From update of attachment 145295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145295&action=review > > Sorry for the drive-by comments - I hope these iterations aren't doing your head in, they are a fact of life with rendering code unfortunately. > > > Source/WebCore/rendering/RenderTable.cpp:1118 > > + if (m_foot) > > + bottomSection = m_foot; > > You should return early here instead of branching. > > > Source/WebCore/rendering/RenderTable.cpp:1121 > > + for (child = lastChild(); child && !child->isTableSection(); child = child->previousSibling()) { } > > This looks like it should be a 'while' clause that returns when it finds a TableSection. Robert, the comments/reviews are most welcome. I wanted to avoid multiple returns from bottomSection() and hence the implementation. I shall incorporate your suggestions in the next patch. Created attachment 145552 [details]
Patch
Comment on attachment 145552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145552&action=review Thanks for fixing all the issues. r=me with one comment and one nit. Btw, if you want your patch to be landed make sure you check the cq? flag as part of your change (under webkit-patch, you can use --request-commit). > Source/WebCore/rendering/RenderTable.cpp:1119 > + if (m_foot) > + return m_foot; I am still surprised at that working but thanks the new test is passing so it must be me. > Source/WebCore/rendering/RenderTable.h:195 > + RenderTableSection* bottomSection() const; This should go next to the topSection() getter. > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:4 > + > +<html> > + Nit: I really don't think this spaces add much. Not repeated on the other files. Created attachment 145750 [details]
Patch
(In reply to comment #29) > (From update of attachment 145552 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145552&action=review > > Thanks for fixing all the issues. r=me with one comment and one nit. Btw, if you want your patch to be landed make sure you check the cq? flag as part of your change (under webkit-patch, you can use --request-commit). > > > Source/WebCore/rendering/RenderTable.cpp:1119 > > + if (m_foot) > > + return m_foot; > > I am still surprised at that working but thanks the new test is passing so it must be me. > > > Source/WebCore/rendering/RenderTable.h:195 > > + RenderTableSection* bottomSection() const; > > This should go next to the topSection() getter. > > > LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:4 > > + > > +<html> > > + > > Nit: I really don't think this spaces add much. Not repeated on the other files. Thanks for the r+ Julien. Have uploaded another patch with the additional changes as suggested by you. Removed the extra line space from adjacent-row-groups-expected.html and moved bottomSection() next to topSection() in RenderTable.h Comment on attachment 145750 [details] Patch Clearing flags on attachment: 145750 Committed r119492: <http://trac.webkit.org/changeset/119492> All reviewed patches have been landed. Closing bug. |