Bug 67877 - "border: collapse" + "display: none" rows in the tbody while having thead or tfoot doesn't render the opposite border
Summary: "border: collapse" + "display: none" rows in the tbody while having thead or ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://jsfiddle.net/gonchuki/8z6WF/show/
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-09 17:37 PDT by gonchuki
Modified: 2012-05-16 05:52 PDT (History)
5 users (show)

See Also:


Attachments
reduction (955 bytes, text/html)
2011-09-09 17:37 PDT, gonchuki
no flags Details
Patch (33.84 KB, patch)
2012-05-14 02:36 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2012-05-16 00:51 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description gonchuki 2011-09-09 17:37:22 PDT
Created attachment 106947 [details]
reduction

tables that have either a <thead> *or* <tfoot> + a <tbody> where all rows are hidden (display: none) are neglecting the render of the opposite side border: let's say, if you have a <thead>, then the bottom border is not rendered, while the layout still reserves the physical space where the border should be (it's just as if the border was transparent).

This only happens when collapse is used, using separate the borders are rendered correctly. In the reduction I created I also added a box-shadow to show how the table still takes its supposed height and that the issue is just with the table border.

Confirmed as a WebKit bug by testing on Safari 5.0.5, 5.1, Chrome 13 (latest stable), Chrome Canary 15.0.874.7 and Android 2.2.2 browser. The test case is correctly rendered by all the other non-WebKit browsers except for IE8, where at first glance same bug can be seen (didn't try much to debug with separate instead of collapse, but IE7 and IE9 render this correctly).

Let me know if you need any more info on this. I saw there are a bunch of issues with border: collapse but couldn't find anything that describes this scenario.
Comment 1 Julien Chaffraix 2011-09-14 18:26:27 PDT
Tested against IE9, Opera and FF and I confirm your findings: WebKit is the only engine not having a bottom border. Great test case and testing, thanks!
Comment 2 Arpita Bahuguna 2012-05-14 02:36:31 PDT
Created attachment 141673 [details]
Patch
Comment 3 Arpita Bahuguna 2012-05-14 02:45:35 PDT
Proposed patch added:

When calculating the collapsed before border, for obtaining the section above the current section, we call sectionAbove(). To this the second param should be passed as SkipEmptySections. Thus for the currSection being the top most section of the table, the returned result would be null which should be the expected output, thus enabling the before table border to be applied for the top most section.
Similarly, when calculating the collapsed after border, we call sectionBelow() for obtaining the section following the currSection. To this call as well we should pass the second param as SkipEmptySections so that for the bottom most section null is returned. This would thus apply the after table border to the bottom most section.

This fix causes an existing layout test case's - fast/table/empty-section-crash.html result to be changed. This is expected with the fix and same result is also observed on FF and Opera.
Modified expected files have been currently included only for the Qt platform.
Comment 4 Antti Koivisto 2012-05-14 03:09:10 PDT
Comment on attachment 141673 [details]
Patch

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

> LayoutTests/ChangeLog:23
> +        * platform/efl/Skipped:
> +        * platform/gtk/test_expectations.txt:
> +        * platform/mac-wk2/Skipped:
> +        * platform/mac/Skipped:
> +        * platform/wincairo/Skipped:
> +        Skipped fast/table/empty-section-crash.html since the expected result
> +        changes with the given fix.
> +
> +        * platform/qt/fast/table/empty-section-crash-expected.png:
> +        * platform/qt/fast/table/empty-section-crash-expected.txt:
> +        Added the new expected results for fast/table/empty-section-crash.html for
> +        Qt platform.

I think you should switch this test to use layoutTestController.dumpAsText() instead of skipping it all over the place. Your new test covers the rendering change.
Comment 5 Arpita Bahuguna 2012-05-16 00:51:33 PDT
Created attachment 142182 [details]
Patch
Comment 6 Arpita Bahuguna 2012-05-16 00:59:22 PDT
Have uploaded another patch.

Since the previously failing fast/table/empty-section-crash.html was modified to make its expected output platform independent, we no longer get differing results for this testcase after applying the patch.

fast/table/empty-section-crash.html verifies whether the page crashes or not and not the layout per se. For verifying the layout of table with collapsed borders, a reftest has been included in this patch : fast/css/table-collapsed-borders.html.
Comment 7 WebKit Review Bot 2012-05-16 05:52:44 PDT
Comment on attachment 142182 [details]
Patch

Clearing flags on attachment: 142182

Committed r117271: <http://trac.webkit.org/changeset/117271>
Comment 8 WebKit Review Bot 2012-05-16 05:52:49 PDT
All reviewed patches have been landed.  Closing bug.