Bug 18305 - Border drawing incorrect when using both border-collapse: collapse and overflow: hidden on a table
Summary: Border drawing incorrect when using both border-collapse: collapse and overfl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Robert Hogan
URL: http://www.willibrordusgroep.nl/test/...
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-04-03 18:39 PDT by Jasper Boot
Modified: 2013-07-24 11:01 PDT (History)
15 users (show)

See Also:


Attachments
Test case reduction file (HTML): Border drawing issues (1.71 KB, text/html)
2008-04-03 18:40 PDT, Jasper Boot
no flags Details
Screenshots of both Firefox and WebKit rendering the tables (68.62 KB, image/png)
2008-04-03 18:46 PDT, Jasper Boot
no flags Details
Patch (11.39 KB, patch)
2013-06-02 01:59 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (544.17 KB, application/zip)
2013-06-02 04:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (567.78 KB, application/zip)
2013-06-02 06:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (576.21 KB, application/zip)
2013-06-02 07:07 PDT, Build Bot
no flags Details
Patch (9.39 KB, patch)
2013-06-02 08:40 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (8.11 KB, patch)
2013-06-10 13:58 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2013-06-12 14:14 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (562.95 KB, application/zip)
2013-06-12 15:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (562.90 KB, application/zip)
2013-06-12 16:19 PDT, Build Bot
no flags Details
Patch (5.17 KB, patch)
2013-06-24 13:06 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (11.04 KB, patch)
2013-07-02 12:12 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (20.32 KB, patch)
2013-07-03 13:57 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (601.18 KB, application/zip)
2013-07-03 14:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (524.13 KB, application/zip)
2013-07-03 18:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from APPLE-EWS-4 for win-future (896.85 KB, application/zip)
2013-07-05 08:16 PDT, Build Bot
no flags Details
Patch (86.11 KB, patch)
2013-07-13 04:03 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (86.10 KB, patch)
2013-07-13 05:59 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (517.95 KB, application/zip)
2013-07-13 06:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (515.74 KB, application/zip)
2013-07-13 08:07 PDT, Build Bot
no flags Details
Patch (82.12 KB, patch)
2013-07-19 10:35 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jasper Boot 2008-04-03 18:39:05 PDT
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
Comment 1 Jasper Boot 2008-04-03 18:40:13 PDT
Created attachment 20323 [details]
Test case reduction file (HTML): Border drawing issues
Comment 2 Jasper Boot 2008-04-03 18:46:53 PDT
Created attachment 20324 [details]
Screenshots of both Firefox and WebKit rendering the tables
Comment 3 David Kilzer (:ddkilzer) 2008-04-27 17:30:09 PDT
Confirmed with Safari 3.0.4 (523.12.2) on Mac OS X 10.4.11 (8S165).

Comment 4 Marek Stepien 2009-12-02 07:17:51 PST
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
Comment 5 Robert Hogan 2013-06-02 01:59:59 PDT
Created attachment 203510 [details]
Patch
Comment 6 Build Bot 2013-06-02 04:41:47 PDT
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
Comment 7 Build Bot 2013-06-02 04:41:49 PDT
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
Comment 8 Build Bot 2013-06-02 06:40:17 PDT
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
Comment 9 Build Bot 2013-06-02 06:40:18 PDT
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
Comment 10 Build Bot 2013-06-02 07:07:31 PDT
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
Comment 11 Build Bot 2013-06-02 07:07:35 PDT
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
Comment 12 Robert Hogan 2013-06-02 08:40:17 PDT
Created attachment 203523 [details]
Patch
Comment 13 Dave Hyatt 2013-06-06 10:41:55 PDT
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?
Comment 14 Robert Hogan 2013-06-10 13:58:03 PDT
Created attachment 204187 [details]
Patch
Comment 15 Dave Hyatt 2013-06-11 13:19:14 PDT
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.
Comment 16 Robert Hogan 2013-06-12 14:14:08 PDT
Created attachment 204533 [details]
Patch
Comment 17 Build Bot 2013-06-12 15:15:23 PDT
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
Comment 18 Build Bot 2013-06-12 15:15:25 PDT
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
Comment 19 Build Bot 2013-06-12 16:18:59 PDT
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
Comment 20 Build Bot 2013-06-12 16:19:02 PDT
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
Comment 21 Robert Hogan 2013-06-24 13:06:35 PDT
Created attachment 205318 [details]
Patch
Comment 22 Dave Hyatt 2013-07-01 12:30:07 PDT
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.
Comment 23 Robert Hogan 2013-07-02 12:12:37 PDT
Created attachment 205934 [details]
Patch
Comment 24 Robert Hogan 2013-07-03 13:57:47 PDT
Created attachment 206023 [details]
Patch
Comment 25 Build Bot 2013-07-03 14:45:19 PDT
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
Comment 26 Build Bot 2013-07-03 14:45:22 PDT
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
Comment 27 Build Bot 2013-07-03 18:49:29 PDT
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
Comment 28 Build Bot 2013-07-03 18:49:32 PDT
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
Comment 29 Build Bot 2013-07-05 08:15:57 PDT
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
Comment 30 Build Bot 2013-07-05 08:16:01 PDT
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
Comment 31 Robert Hogan 2013-07-13 04:03:05 PDT
Created attachment 206601 [details]
Patch
Comment 32 EFL EWS Bot 2013-07-13 04:17:48 PDT
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
Comment 33 Build Bot 2013-07-13 04:28:54 PDT
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
Comment 34 Build Bot 2013-07-13 04:39:00 PDT
Comment on attachment 206601 [details]
Patch

Attachment 206601 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1063339
Comment 35 kov's GTK+ EWS bot 2013-07-13 04:40:15 PDT
Comment on attachment 206601 [details]
Patch

Attachment 206601 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1069026
Comment 36 Robert Hogan 2013-07-13 05:59:08 PDT
Created attachment 206603 [details]
Patch
Comment 37 Build Bot 2013-07-13 06:48:17 PDT
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
Comment 38 Build Bot 2013-07-13 06:48:21 PDT
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
Comment 39 Build Bot 2013-07-13 08:06:56 PDT
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
Comment 40 Build Bot 2013-07-13 08:07:01 PDT
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
Comment 41 Dave Hyatt 2013-07-16 12:02:19 PDT
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.
Comment 42 Robert Hogan 2013-07-19 10:35:58 PDT
Created attachment 207122 [details]
Patch
Comment 43 Dave Hyatt 2013-07-23 12:09:53 PDT
Comment on attachment 207122 [details]
Patch

r=me. Look at how nice that patch ended up being!
Comment 44 WebKit Commit Bot 2013-07-24 11:00:57 PDT
Comment on attachment 207122 [details]
Patch

Clearing flags on attachment: 207122

Committed r153089: <http://trac.webkit.org/changeset/153089>
Comment 45 WebKit Commit Bot 2013-07-24 11:01:04 PDT
All reviewed patches have been landed.  Closing bug.