Bug 5515 - Border collapse problem with rowspan/colspan cells
Summary: Border collapse problem with rowspan/colspan cells
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://newgeo.com/web/css/safaritd.html
Keywords: HasReduction
: 21208 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-10-26 14:35 PDT by scott
Modified: 2014-03-21 07:38 PDT (History)
12 users (show)

See Also:


Attachments
Reduction to show collapsed border bug with rowspan (291 bytes, text/html)
2007-07-09 14:26 PDT, Aaron Rosenzweig
no flags Details
Patch (46.67 KB, patch)
2012-08-20 06:36 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (379.08 KB, application/zip)
2012-08-20 07:31 PDT, WebKit Review Bot
no flags Details
Patch (81.90 KB, patch)
2012-08-21 04:59 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (701.79 KB, application/zip)
2012-08-21 09:10 PDT, WebKit Review Bot
no flags Details
Patch (81.90 KB, patch)
2012-08-21 22:21 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (67.38 KB, text/plain)
2012-08-26 23:36 PDT, Arpita Bahuguna
no flags Details
Archive of layout-test-results from gce-cr-linux-04 (518.48 KB, application/zip)
2012-08-27 05:30 PDT, WebKit Review Bot
no flags Details
Patch (108.12 KB, text/plain)
2012-08-27 23:22 PDT, Arpita Bahuguna
no flags Details
Archive of layout-test-results from gce-cr-linux-05 (460.68 KB, application/zip)
2012-08-28 00:07 PDT, WebKit Review Bot
no flags Details
Patch (120.59 KB, patch)
2012-08-28 02:08 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (120.51 KB, patch)
2012-08-29 00:11 PDT, Arpita Bahuguna
jchaffraix: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description scott 2005-10-26 14:35:44 PDT
As seen in the example link, the td with the row/colspan does not have a class/style applied to it, yet it 
still gets a border.  The bordering is inconsistent.

Adding this trick to it
td[colspan]{
     border: 1px solid transparent !important;
}

Fixes it, however, it also breaks it badly in other browsers with that workaround.
Comment 1 Dave Hyatt 2005-10-26 15:09:49 PDT
This is a problem with border collapsing.
Comment 2 scott 2005-10-26 15:11:42 PDT
So are you saying it is an issue with my css, or just the border-collapse in general?
Comment 3 Dave Hyatt 2005-10-26 15:22:44 PDT
Nothing wrong with your CSS. It's a bug in Safari's border-collapse handling when cells have spans.
Comment 4 mitz 2006-12-19 11:12:24 PST
The linked test case is no longer available. A lot of border collapsing issues have been fixed since this bug was filed. Scott, can you attach the original test case to this bug?
Comment 5 Kurt Moore 2006-12-19 13:40:48 PST
I dont have the file anymore, sorry :-(
Seems to be resolved in my tests.
Comment 6 Aaron Rosenzweig 2007-07-09 14:26:10 PDT
Created attachment 15455 [details]
Reduction to show collapsed border bug with rowspan

When two table cells share a border and the borders are "collapsed" then the shared border effectively becomes one. What happens if the 2nd cell defines the shared wall as red? The wall probably should turn red if that rule is more specific. What happens if the 1st cell has a rowspan? Firefox says that the redness should stop at the bottom of the 2nd cell. Safari says that the redness should continue all the way down all the rowspans of the 1st cell. Please try this attached reduction in both Firefox and Safari to see the difference. I'm testing in the Safari 3 Beta on Mac and still see this issue.
Comment 7 Aaron Rosenzweig 2007-07-19 13:24:16 PDT
Can someone comment about the reduction I posted to help solve this issue? Please let me know if it is clear. It should be but I don't want to assume.
Comment 8 mitz 2007-07-19 13:30:33 PDT
(In reply to comment #7)
> Can someone comment about the reduction I posted to help solve this issue?
> Please let me know if it is clear. It should be but I don't want to assume.

Good reduction! The issue is well understood. Thanks.
Comment 9 mitz 2008-09-29 09:49:25 PDT
*** Bug 21208 has been marked as a duplicate of this bug. ***
Comment 10 mitz 2008-09-29 09:50:26 PDT
See also bug 14274.
Comment 11 David Kilzer (:ddkilzer) 2008-09-29 10:58:51 PDT
Reassigning to webkit-unassigned for wider exposure.
Comment 12 Arpita Bahuguna 2012-08-20 06:36:20 PDT
Created attachment 159412 [details]
Patch
Comment 13 WebKit Review Bot 2012-08-20 07:31:53 PDT
Comment on attachment 159412 [details]
Patch

Attachment 159412 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13529965

New failing tests:
fast/table/border-collapsing/collapsed-border-with-colspan.html
fast/table/border-collapsing/collapsed-border-with-rowspan.html
Comment 14 WebKit Review Bot 2012-08-20 07:31:56 PDT
Created attachment 159422 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 15 Arpita Bahuguna 2012-08-21 04:59:41 PDT
Created attachment 159659 [details]
Patch
Comment 16 Arpita Bahuguna 2012-08-21 07:24:45 PDT
Added the expected files from the previous patch for cr-linux.
Comment 17 WebKit Review Bot 2012-08-21 09:10:55 PDT
Comment on attachment 159659 [details]
Patch

Attachment 159659 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13550405

New failing tests:
fast/table/border-collapsing/collapsed-border-with-colspan.html
fast/table/border-collapsing/collapsed-border-with-rowspan.html
Comment 18 WebKit Review Bot 2012-08-21 09:10:59 PDT
Created attachment 159701 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 19 Arpita Bahuguna 2012-08-21 22:21:33 PDT
Created attachment 159860 [details]
Patch
Comment 20 Eric Seidel (no email) 2012-08-22 12:01:16 PDT
Comment on attachment 159860 [details]
Patch

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

> LayoutTests/fast/table/border-collapsing/collapsed-border-with-rowspan.html:9
> +<p>Test for Bugzilla <a href="https://bugs.webkit.org/show_bug.cgi?id=5515">Bug 5515</a>:  Border collapse problem with rowspan/colspan cells.</p>
> +<p>The following tests verify the border-collapse behavior on tables with rowspan.</p>
> +<table border="1" style="border-collapse : collapse;">

Please remove the text so that we can share pixel results between platforms.  Just make it an HTML comment instead of rendered text.
Comment 21 Julien Chaffraix 2012-08-22 12:55:45 PDT
Comment on attachment 159860 [details]
Patch

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

> Source/WebCore/rendering/RenderTableCell.cpp:498
> +    if (!isEndColumn && !(rowSpan() > 1)) {

This fix is not right. First you are only touching half of the collapsing border functions which means that it is still broken in half of the cases. Secondly, after this change, you are ignoring adjacent rowSpan / colspan > 1 cells that still have a full common border which I think is unfortunate. The spec is silent on what we are supposed to do with rowspan / colspan but it doesn't make much sense to disallow this case, especially since we handle it properly already and other browsers seems to allow it too. I posted something similar some time ago on bug 20840 (only for the colspan case) but never followed up on it due to lack of time.

Note that your fix is a hack around the fact that we should support border segmenting per bug 20260 and you should say it, not hide this fact. Adding this would be the best but it would make our collapsing border code more complex and we would probably run this change through the standards for inter-operability.
Comment 22 Arpita Bahuguna 2012-08-26 23:36:51 PDT
Created attachment 160650 [details]
Patch
Comment 23 WebKit Review Bot 2012-08-27 05:30:34 PDT
Comment on attachment 160650 [details]
Patch

Attachment 160650 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13619115

New failing tests:
fast/forms/range/slider-delete-while-dragging-thumb.html
tables/mozilla/marvin/backgr_layers-opacity.html
inspector/debugger/source-frame.html
fast/loader/unload-form-post-about-blank.html
tables/mozilla/marvin/backgr_simple-table-cell.html
fast/canvas/webgl/shader-precision-format.html
animations/suspend-resume-animation-events.html
tables/mozilla_expected_failures/marvin/backgr_border-table-quirks.html
http/tests/xmlhttprequest/zero-length-response.html
tables/mozilla_expected_failures/marvin/backgr_border-table-column-group.html
tables/mozilla_expected_failures/marvin/backgr_border-table-row.html
fast/table/border-collapsing/collapsed-border-with-colspan.html
platform/chromium/virtual/gpu/fast/canvas/webgl/shader-precision-format.html
fast/forms/range/slider-mouse-events.html
fast/frames/cached-frame-counter.html
tables/mozilla/marvin/backgr_simple-table-column-group.html
tables/mozilla/marvin/backgr_simple-table-row.html
tables/mozilla/marvin/backgr_simple-table.html
tables/mozilla_expected_failures/marvin/backgr_border-table-column.html
fast/table/border-collapsing/collapsed-border-with-rowspan.html
tables/mozilla_expected_failures/marvin/backgr_border-table-row-group.html
tables/mozilla/marvin/backgr_position-table.html
tables/mozilla/marvin/backgr_simple-table-row-group.html
tables/mozilla/marvin/backgr_simple-table-column.html
tables/mozilla_expected_failures/marvin/backgr_border-table-cell.html
fast/forms/range/slider-onchange-event.html
fast/loader/local-CSS-from-local.html
Comment 24 WebKit Review Bot 2012-08-27 05:30:39 PDT
Created attachment 160699 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 25 Arpita Bahuguna 2012-08-27 23:22:09 PDT
Created attachment 160909 [details]
Patch
Comment 26 WebKit Review Bot 2012-08-28 00:07:55 PDT
Comment on attachment 160909 [details]
Patch

Attachment 160909 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13664073

New failing tests:
tables/mozilla_expected_failures/marvin/backgr_position-table-row.html
tables/mozilla_expected_failures/marvin/backgr_position-table-column.html
tables/mozilla_expected_failures/marvin/backgr_fixed-bg.html
tables/mozilla_expected_failures/marvin/backgr_position-table-row-group.html
tables/mozilla_expected_failures/marvin/backgr_position-table-column-group.html
tables/mozilla_expected_failures/marvin/backgr_layers-hide.html
tables/mozilla_expected_failures/marvin/backgr_position-table-cell.html
tables/mozilla_expected_failures/marvin/backgr_layers-show.html
tables/mozilla_expected_failures/marvin/backgr_border-table.html
Comment 27 WebKit Review Bot 2012-08-28 00:07:59 PDT
Created attachment 160919 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 28 Arpita Bahuguna 2012-08-28 02:08:43 PDT
Created attachment 160931 [details]
Patch
Comment 29 Arpita Bahuguna 2012-08-28 04:43:45 PDT
(In reply to comment #21)
> (From update of attachment 159860 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159860&action=review
> 
> > Source/WebCore/rendering/RenderTableCell.cpp:498
> > +    if (!isEndColumn && !(rowSpan() > 1)) {
> 
> This fix is not right. First you are only touching half of the collapsing border functions which means that it is still broken in half of the cases. Secondly, after this change, you are ignoring adjacent rowSpan / colspan > 1 cells that still have a full common border which I think is unfortunate. The spec is silent on what we are supposed to do with rowspan / colspan but it doesn't make much sense to disallow this case, especially since we handle it properly already and other browsers seems to allow it too. I posted something similar some time ago on bug 20840 (only for the colspan case) but never followed up on it due to lack of time.
> 
> Note that your fix is a hack around the fact that we should support border segmenting per bug 20260 and you should say it, not hide this fact. Adding this would be the best but it would make our collapsing border code more complex and we would probably run this change through the standards for inter-operability.

Hi Julien, thanks for the review.
I agree that my previous patch was a workaround; only addressing the problem at hand. 
Have tried to incorporate most cases with this patch (adjacent colspan/rowspan, patching in all areas etc.).
Have also added some more cases in the layout testcase added along with this patch.
Comment 30 Robert Hogan 2012-08-28 12:17:52 PDT
Comment on attachment 160931 [details]
Patch

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

> Source/WebCore/rendering/RenderTableCell.cpp:438
> +        if (isValidCellForColRowSpanBorderResolution(true, prevCell)) {
>          CollapsedBorderValue prevCellBorder = CollapsedBorderValue(prevCell->style()->borderEnd(), includeColor ? prevCell->style()->visitedDependentColor(endColorProperty) : Color(), BCELL);

You're missing your indentation here.

> Source/WebCore/rendering/RenderTableCell.h:214
> +    bool isValidCellForColRowSpanBorderResolution(bool isStartOrEndBorder, RenderTableCell* adjacentCell = 0) const;

Couldn't this helper function be static?

> LayoutTests/ChangeLog:67
> +        Modified existing results for qt and chromium-win ports.

It's not obvious why these new results are correct - can you provide some commentary?
Comment 31 Arpita Bahuguna 2012-08-29 00:11:41 PDT
Created attachment 161153 [details]
Patch
Comment 32 Arpita Bahuguna 2012-08-29 08:59:42 PDT
(In reply to comment #30)

Thanks for the review comments Robert. Have uploaded another patch with the suggested changes.

> (From update of attachment 160931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160931&action=review
> 
> > Source/WebCore/rendering/RenderTableCell.cpp:438
> > +        if (isValidCellForColRowSpanBorderResolution(true, prevCell)) {
> >          CollapsedBorderValue prevCellBorder = CollapsedBorderValue(prevCell->style()->borderEnd(), includeColor ? prevCell->style()->visitedDependentColor(endColorProperty) : Color(), BCELL);
> 
> You're missing your indentation here.
Missed out on this.
> 
> > Source/WebCore/rendering/RenderTableCell.h:214
> > +    bool isValidCellForColRowSpanBorderResolution(bool isStartOrEndBorder, RenderTableCell* adjacentCell = 0) const;
> 
> Couldn't this helper function be static?
Done.
> 
> > LayoutTests/ChangeLog:67
> > +        Modified existing results for qt and chromium-win ports.
> 
> It's not obvious why these new results are correct - can you provide some commentary?
Added an explanation for the changes in the results.
Comment 33 Julien Chaffraix 2012-09-06 16:55:46 PDT
Comment on attachment 161153 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Our handling of colspans and rowspans while resolving collapsed borders
> +        on a table is not proper.

Globally this sentence doesn't add anything. How does this sentence help someone unfamiliar with your change understand what you did?

Adding a one line short / TL;DR description is totally welcome but think how you can pack *relevant* information in one sentence.

> Source/WebCore/ChangeLog:17
> +        The left border (or the start border) specified for the first cell
> +        adjacent to a spanned cell (rowspan) was being applied for the entire length
> +        of the spanned cell's right (end border) border.
> +
> +        Similarly, the top border (or the before border) specified for the first
> +        cell below a spanned cell (colspan) was being applied for the entire
> +        length of the spanned cell's bottom (after border) border.

Be careful how you phrase the change. start / before are not strictly equal to left / top but this ChangeLog mixes the 2 and makes people wonder if you know the difference.

> Source/WebCore/ChangeLog:21
> +        collapsed borders.

Nowhere do you mention in your ChangeLog that this change is a work-around, please do and mention the bug with the right fix.

> Source/WebCore/rendering/RenderTableCell.cpp:342
> +static bool isValidCellForColRowSpanBorderResolution(const RenderTableCell* currCell, bool isStartOrEndBorder, const RenderTableCell* adjacentCell = 0)

Please NO BOOLEAN parameters. They make the call sites difficult to read and every time you add one you should ask yourself if it's needed. See http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html among other resources.

On top of that, here they are unnecessary and confusing. You should have 2 function dedicated to each logical direction.

> Source/WebCore/rendering/RenderTableCell.cpp:346
> +        if (currCell->rowSpan() > 1)
> +            return adjacentCell ?  adjacentCell->rowSpan() > 1 : false;

First unrelated but this could be written: return adjacentCell && adjacentCell->rowSpan() > 1.

Second, I don't see the point of doing this comparison with an empty |adjacentCell|.

Finally, I really don't understand what the point of this check is. I would rather keep the alignment case as our heuristic here. Both choices are arbitrary but it matches other browsers on more cases and AFAICT yours requires a lot more rebaselining.

> Source/WebCore/rendering/RenderTableCell.cpp:509
> +    if (!isEndColumn && isValidCellForColRowSpanBorderResolution(this, true)) {

NO! Changing this condition means that you now wrongly fall back to checking the |row|'s end border for middle rows if the cells has a rowspan.

> LayoutTests/fast/table/border-collapsing/collapsed-border-with-colspan.html:51
> +<!-- Red, green and blue colored borders should be displayed per cell. -->

Red should be reserved for failures. See our wiki on how to write good layout tests: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

Preferably this should be a ref-test.