Bug 5515 – Border collapse problem with rowspan/colspan cells

• Component: Tables
• Status: NEW

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.

[Dave Hyatt]

This is a problem with border collapsing.

So are you saying it is an issue with my css, or just the border-collapse in general?

[Dave Hyatt]

Nothing wrong with your CSS. It's a bug in Safari's border-collapse handling when cells have spans.

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?

[Kurt Moore]

I dont have the file anymore, sorry :-(
Seems to be resolved in my tests.

[Aaron Rosenzweig]

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.

[Aaron Rosenzweig]

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.

(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.

*** Bug 21208 has been marked as a duplicate of this bug. ***

See also bug 14274.

[David Kilzer (:ddkilzer)]

Reassigning to webkit-unassigned for wider exposure.

[Arpita Bahuguna]

Created attachment 159412 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Arpita Bahuguna]

Created attachment 159659 [details]
Patch

[Arpita Bahuguna]

Added the expected files from the previous patch for cr-linux.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Arpita Bahuguna]

Created attachment 159860 [details]
Patch

[Eric Seidel (no email)]

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.

[Julien Chaffraix]

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.

[Arpita Bahuguna]

Created attachment 160650 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Arpita Bahuguna]

Created attachment 160909 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Arpita Bahuguna]

Created attachment 160931 [details]
Patch

[Arpita Bahuguna]

(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.

[Robert Hogan]

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?

[Arpita Bahuguna]

Created attachment 161153 [details]
Patch

[Arpita Bahuguna]

(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.

[Julien Chaffraix]

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.

[Ahmad Saleem]

This bug is reproducible using attached "reduced test case" on Safari 15.5 on macOS 12.4. Chrome Canary 104 and Firefox Nightly 102 behaves same. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/94163960>