Bug 18305 – Border drawing incorrect when using both border-collapse: collapse and overflow: hidden on a table

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jasper Boot]

When WebKit renders a table for which both border-collapse: collapse; and overflow: hidden; are set via CSS the rendering of my tables with 1px border looked strange: the bottom and right border were not displayed at all.
After some investigation I found that these borders do get rendered somehow, but in another way than usual; this effect can be seen by enlarging the border-width.

Please check the website or the attached test case reduction file to see an example of both cases

[Jasper Boot]

Created attachment 20323 [details]
Test case reduction file (HTML): Border drawing issues

[Jasper Boot]

Created attachment 20324 [details]
Screenshots of both Firefox and WebKit rendering the tables

[David Kilzer (:ddkilzer)]

Confirmed with Safari 3.0.4 (523.12.2) on Mac OS X 10.4.11 (8S165).

[Marek Stepien]

I can also reproduce this bug with the following browsers:

Mozilla/5.0 (Windows; U; Windows NT 5.1; pl-PL) AppleWebKit/531.9 (KHTML, like Gecko) Version/4.0.3 Safari/531.9.1 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/532.0 (KHTML, like Gecko) Chrome/3.0.195.33 Safari/532.0

[Robert Hogan]

Created attachment 203510 [details]
Patch

[Build Bot]

Comment on attachment 203510 [details]
Patch

Attachment 203510 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/668255

New failing tests:
css2.1/20110323/overflow-applies-to-014.htm
tables/mozilla_expected_failures/marvin/table_overflow_hidden_table.html
tables/mozilla_expected_failures/marvin/table_overflow_caption_hidden_table.html
tables/mozilla/bugs/bug44505.html
css2.1/20110323/overflow-applies-to-013.htm
tables/mozilla_expected_failures/bugs/bug106966.html

[Build Bot]

Created attachment 203513 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 203510 [details]
Patch

Attachment 203510 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/656645

New failing tests:
css2.1/20110323/overflow-applies-to-014.htm
tables/mozilla_expected_failures/marvin/table_overflow_hidden_table.html
tables/mozilla_expected_failures/marvin/table_overflow_caption_hidden_table.html
tables/mozilla/bugs/bug44505.html
css2.1/20110323/overflow-applies-to-013.htm
tables/mozilla_expected_failures/bugs/bug106966.html

[Build Bot]

Created attachment 203514 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 203510 [details]
Patch

Attachment 203510 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/708246

New failing tests:
css2.1/20110323/overflow-applies-to-014.htm
tables/mozilla_expected_failures/marvin/table_overflow_hidden_table.html
tables/mozilla_expected_failures/marvin/table_overflow_caption_hidden_table.html
tables/mozilla/bugs/bug44505.html
css2.1/20110323/overflow-applies-to-013.htm
tables/mozilla_expected_failures/bugs/bug106966.html

[Build Bot]

Created attachment 203515 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Robert Hogan]

Created attachment 203523 [details]
Patch

[Dave Hyatt]

Comment on attachment 203523 [details]
Patch

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

r-

> Source/WebCore/rendering/RenderTable.cpp:1363
> +    if (phase == PaintPhaseChildBlockBackgrounds) {
> +        rect = borderBoxRectInRegion(region);
> +        rect.setLocation(location + rect.location());
> +        clipScrollBars(rect, relevancy);

This code doesn't make much sense to me. Isn't the clipScrollbars rectangle going to clip scrollbars in the wrong place?

[Robert Hogan]

Created attachment 204187 [details]
Patch

[Dave Hyatt]

Comment on attachment 204187 [details]
Patch

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

r-

> Source/WebCore/rendering/RenderTable.cpp:1364
> +    LayoutRect rect;
> +    if (phase == PaintPhaseChildBlockBackgrounds) {
> +        rect = borderBoxRectInRegion(region);
> +        rect.setLocation(location + rect.location());
> +    } else
> +        rect = RenderBox::overflowClipRect(location, region, relevancy);

I just don't think you can make this phase-dependent. Make the table into a self-painting layer, e.g., by making it position:absolute, and suddenly the overflowClipRect is going to get cached by the RenderLayer clipRects code, and it's not going to use the phase you want.

Basically overflowClipRect can't be giving different answers based off phases.

[Robert Hogan]

Created attachment 204533 [details]
Patch

[Build Bot]

Comment on attachment 204533 [details]
Patch

Attachment 204533 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/869038

New failing tests:
fast/table/table-overflow.html
fast/repaint/table-cell-collapsed-border-scroll.html

[Build Bot]

Created attachment 204538 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 204533 [details]
Patch

Attachment 204533 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/869053

New failing tests:
fast/table/table-overflow.html
fast/repaint/table-cell-collapsed-border-scroll.html

[Build Bot]

Created attachment 204542 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Robert Hogan]

Created attachment 205318 [details]
Patch

[Dave Hyatt]

Comment on attachment 205318 [details]
Patch

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

r-

> Source/WebCore/rendering/RenderTable.cpp:672
> +            info.context->restore();
> +            info.context->save();

This code seems to be making the assumption that the last save() you did pushed only the overflow clip? That seems like a bogus assumption.

[Robert Hogan]

Created attachment 205934 [details]
Patch

[Robert Hogan]

Created attachment 206023 [details]
Patch

[Build Bot]

Comment on attachment 206023 [details]
Patch

Attachment 206023 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1015808

New failing tests:
fast/table/overflowHidden.html

[Build Bot]

Created attachment 206027 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 206023 [details]
Patch

Attachment 206023 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1019548

New failing tests:
fast/table/overflowHidden.html

[Build Bot]

Created attachment 206041 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 206023 [details]
Patch

Attachment 206023 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/975044

New failing tests:
fast/table/overflowHidden.html

[Build Bot]

Created attachment 206154 [details]
Archive of layout-test-results from APPLE-EWS-4 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-4  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Robert Hogan]

Created attachment 206601 [details]
Patch

[EFL EWS Bot]

Comment on attachment 206601 [details]
Patch

Attachment 206601 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1066261

[Build Bot]

Comment on attachment 206601 [details]
Patch

Attachment 206601 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1070023

[Build Bot]

Comment on attachment 206601 [details]
Patch

Attachment 206601 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1063339

[kov's GTK+ EWS bot]

Comment on attachment 206601 [details]
Patch

Attachment 206601 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1069026

[Robert Hogan]

Created attachment 206603 [details]
Patch

[Build Bot]

Comment on attachment 206603 [details]
Patch

Attachment 206603 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1054576

New failing tests:
fast/table/overflowHidden.html

[Build Bot]

Created attachment 206605 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 206603 [details]
Patch

Attachment 206603 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1069057

New failing tests:
fast/table/overflowHidden.html

[Build Bot]

Created attachment 206606 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Dave Hyatt]

Comment on attachment 206603 [details]
Patch

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

r-

> Source/WebCore/rendering/RenderTableCell.cpp:1004
> +LayoutRect RenderTableCell::overflowClipRect(const LayoutPoint& location, RenderRegion* region, OverlayScrollbarSizeRelevancy relevancy)

I don't understand any of these table cell changes really.  In the self-painting layers case, the cell gets clipped properly already by overflowClipRectForChildLayers. In the case where the table cell has no layer, why not just simulate the same thing and re-use overflowClipRectForChildLayers. Make the table code actually push the overflowClipRectForChildLayers clip before painting kids.

I really think done right you don't have to touch RenderTableCell at all.

[Robert Hogan]

Created attachment 207122 [details]
Patch

[Dave Hyatt]

Comment on attachment 207122 [details]
Patch

r=me. Look at how nice that patch ended up being!

[WebKit Commit Bot]

Comment on attachment 207122 [details]
Patch

Clearing flags on attachment: 207122

Committed r153089: <http://trac.webkit.org/changeset/153089>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.