Bug 76246 – Incorrect rendering of borders on <col> with span > 1

• Component: Tables
• Status: RESOLVED FIXED

[Boris Zbarsky]

The testcase at https://bug717854.bugzilla.mozilla.org/attachment.cgi?id=588310 should show a red border between the A and B and likewise between the D and F.

The relevant part of the HTML5 spec is in http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#tables and says:

  For the purposes of the CSS table model, the col element is expected to be treated as if
  it was present as many times as its span attribute specifies.

[Alexey Proskuryakov]

Bugzilla returns a ton of results for "border span". Is this bug new? I suspect that no one attempted to clean up those (I certainly didn't, due to limited understanding of how these work).

[Boris Zbarsky]

I did search before filing.  All the bugs I saw had to do with colspan/rowspan on cells.

[Arpita Bahuguna]

Created attachment 161890 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 161890 [details]
Patch

Attachment 161890 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13740120

[Arpita Bahuguna]

Created attachment 161966 [details]
Patch

[Julien Chaffraix]

Comment on attachment 161966 [details]
Patch

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

> Source/WebCore/rendering/RenderTableCell.cpp:458
> +            // Next, irrespective of whether it is a spanned col or not, we apply its start border.

It would be nice to quote HTML5 here to know why we apply the col every time.

> Source/WebCore/rendering/RenderTableCell.cpp:459
> +            result = chooseBorder(result, CollapsedBorderValue(colElt->style()->borderStart(), includeColor ? colElt->style()->visitedDependentColor(startColorProperty) : Color(), BCOL));

The order in which you apply the borders here is the opposite of what CSS uses (column should win over column-group per 17.6.2.1 point 4). You should apply |colElt| first and then find its enclosingColumnGroup to apply the border. This should also be tested.

> Source/WebCore/rendering/RenderTableCell.cpp:475
> +                result = chooseBorder(CollapsedBorderValue(colElt->style()->borderEnd(), includeColor ? colElt->style()->visitedDependentColor(endColorProperty) : Color(), BCOL), result);

result should be first as we return the first parameter in case of equivalent borders. The code is not really consistent here unfortunately and we should try to avoid adding more latent bugs. It should be easily testable using equivalent borders of different color.

> Source/WebCore/rendering/RenderTableCell.cpp:573
> +                if (RenderTableCol* enclosingColumnGroup = colElt->enclosingColumnGroupIfAdjacentBefore()) {

Same remark about the ordering here too.

> LayoutTests/ChangeLog:11
> +        Modified the result of the existing test case which consists of a table with
> +        col and colgroup specified.

I have a hard time understand what changed. None of the borders have changed, yet your element is bigger. Could you explain what the change is and why it occurs?

> LayoutTests/ChangeLog:15
> +        * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.png: Added.
> +        * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.txt: Added.
> +        * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html: Added.

Could it be possible to change this into a ref-test?

> LayoutTests/fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html:26
> +<col span=3 style="border: 1px solid red;"></col>

red should be reserved for failure.

[Arpita Bahuguna]

Created attachment 165260 [details]
Patch

[Build Bot]

Comment on attachment 165260 [details]
Patch

Attachment 165260 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13986044

New failing tests:
fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html

[WebKit Review Bot]

Comment on attachment 165260 [details]
Patch

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

New failing tests:
fast/table/border-collapsing/cached-change-colgroup-border-color.html
WebFilterOperationsTest.saveAndRestore
fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html
fast/table/border-collapsing/cached-change-colgroup-border-width.html

[Arpita Bahuguna]

Created attachment 165350 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #6)
> (From update of attachment 161966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161966&action=review

Hi Julien, thank-you for the review. Have made amends to handle most of the review comments except for the following two:

> 
> > Source/WebCore/rendering/RenderTableCell.cpp:475
> > +                result = chooseBorder(CollapsedBorderValue(colElt->style()->borderEnd(), includeColor ? colElt->style()->visitedDependentColor(endColorProperty) : Color(), BCOL), result);
> 
> result should be first as we return the first parameter in case of equivalent borders. The code is not really consistent here unfortunately and we should try to avoid adding more latent bugs. It should be easily testable using equivalent borders of different color.

'result' should be the second param here as when considering the preceeding column's end border, as per the specification, the left most should be given higher priority.
Also, have added a new test (last table in the added testcase) which highlights this issue. If result were to be given higher priority, the borders for the last table come as red, light brown, green and black instead of transparent/invisible, light green, dark green and black.

> 
> > LayoutTests/ChangeLog:15
> > +        * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.png: Added.
> > +        * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.txt: Added.
> > +        * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html: Added.
> 
> Could it be possible to change this into a ref-test?
I tried, but found it difficult to achieve the same results w/o using collapsed borders.

[Build Bot]

Comment on attachment 165350 [details]
Patch

Attachment 165350 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13992662

New failing tests:
fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html

[Arpita Bahuguna]

Created attachment 166250 [details]
Patch

[Arpita Bahuguna]

Added another patch, making the layout testcase image only and platform specific.

[Julien Chaffraix]

Comment on attachment 166250 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        As per the specifications we should apply a col element's border as if

Please say which specification as it is important: tables are defined by several specifications: HTML (4 and 5) and CSS 2.1.

Ideally, you should quote it somewhere (the code is usually the best place but not if we have to duplicate it).

> Source/WebCore/ChangeLog:19
> +        Borders from col and it's enclosing colgroup element should be handled

typo: its enclosing colgroup.

> Source/WebCore/rendering/RenderTableCell.cpp:477
> +            // The |colElt| is a column group, in which case we apply it's start border but only if it

typo: its start border (this error occurs in other comments but I didn't repeat it for each mistake)

> Source/WebCore/rendering/RenderTableCell.cpp:501
> +        if (RenderTableCol* colElt = table->colElement(col() -1, &startColEdge, &endColEdge)) {

Let's fix the style when we move code (there should be a space after the '-').

> Source/WebCore/rendering/RenderTableCell.cpp:603
> +            // If the current element is a colgroup then we apply it's start border only if it is the first
> +            // colgroup (considering spanned colgroups).

I really wonder how we could make those comments more digest as they are pretty repetitive (yet a bit different different for each function) and could be condensed & add more value. My take below.

> Source/WebCore/rendering/RenderTableCell.cpp:605
> +                result = chooseBorder(result, CollapsedBorderValue(colElt->borderAdjoiningCellBefore(this), includeColor ? colElt->style()->visitedDependentColor(startColorProperty) : Color(), BCOLGROUP));

// This case is a colgroup without any col, we only compute it if it is adjacent to the cell's edge.

> Source/WebCore/rendering/RenderTableCell.cpp:608
> +            } else if (!colElt->isTableColumnGroup()) {

You probably want to use colElt->isTableColumn() instead of the negative (not isTableColumnGroup).

> Source/WebCore/rendering/RenderTableCell.cpp:609
> +                // First, apply the start border of the |colElt|, irrespective of whether it is spanned or not.

// Resolve the collapsing border against the col's border ignoring any 'span' per HTML5.

> Source/WebCore/rendering/RenderTableCell.cpp:614
> +                // Next, if the element has a parent colgroup then it's start border should be applied
> +                // but only if it is the first col in that colgroup (i.e. considering spanned cols within a colgroup).

// If we have a parent colgroup, resolve the border only if it is adjacent to the cell.

> LayoutTests/ChangeLog:19
> +        Existing tests modified. This is because previously, while computing collpased
> +        start border, we did not take the preceeding col's enclosing colgroup's end border
> +        into consideration. While computing the collapsed start border, only the preceeding
> +        col element's end border was considered.
> +
> +        With this fix, for the above two tests, the last col's width now changes due to
> +        the border being applied to it (the preceeding col's enclosing colgroup's end border)
> +        which causes the table's width to change.

Good catch. Worth mentioning that the cell's are growing by half the border's width, which is expected.

> LayoutTests/ChangeLog:25
> +        New image only test added for verifying the behavior of collapsed borders with col
> +        and colgroup span.

Bad wrapping.

> LayoutTests/fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html:27
> +<!-- This table has a col with span. Individual borders for the spanned cells should be painted. -->

The descriptions here are really difficult to make sense of. If you don't know the collapsing border algorithm (which isn't the case for most people), you have a hard time saying if the output is expected.

> LayoutTests/fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html:40
> +<div id="space"></div>

This case could probably use some red for anything that shouldn't be shown (like .bordered's border-right as it should be disabled by the next column's border, with the exception of the last col that would need an extra style).

[Arpita Bahuguna]

Created attachment 167480 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #15)
> (From update of attachment 166250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166250&action=review
> 
Thanks for the review Julien. Have made the changes as per your comments and uploaded another patch.
Apologize for the silly "it's" mistake (among others). Have nailed it in my head now :) and thank-you for your help with re-framing the comments.

[Eric Seidel (no email)]

Comment on attachment 167480 [details]
Patch

We seems to be repeating much of this code 4 times.  Is there no way to share it?

[Julien Chaffraix]

Comment on attachment 167480 [details]
Patch

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

I will fix the patch for landing.

> We seems to be repeating much of this code 4 times.  Is there no way to share it?

That would be neat but I don't see a good way to do that: each repetition is indeed similar but also slightly different from the other ones. This makes sharing difficult. The code is also confusingly complex due to equal border resolution (all other parameters being equals, the start-most border wins)

> Source/WebCore/rendering/RenderTableCell.cpp:532
> +            // we apply its start border. This is as per the specification which states that

HTML5 instead of "the specification" here. This has an importance as CSS tables don't have the concept of spanning cell.

> Source/WebCore/rendering/RenderTableCell.cpp:539
> +            // Next, apply the start border of the enclosing colgroup but only if it is
> +            // adjacent to the cell's edge.

This could be on one line.

> Source/WebCore/rendering/RenderTableCell.cpp:632
> +            // This is as per the specification which states that "For the purposes of the CSS table model,

Same comment here.

> Source/WebCore/rendering/RenderTableCell.cpp:639
> +            // Next, if it has a parent colgroup then we apply its end border but only if it is
> +            // adjacent to the cell.

Ditto.

> LayoutTests/ChangeLog:12
> +        Existing tests modified. This is because previously, while computing collpased

Typo: collpased.

[Julien Chaffraix]

Created attachment 169288 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 169288 [details]
Patch for landing

Clearing flags on attachment: 169288

Committed r131671: <http://trac.webkit.org/changeset/131671>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arpita Bahuguna]

(In reply to comment #19)
> (From update of attachment 167480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167480&action=review
> 
> I will fix the patch for landing.
> 
Thanks for the review Julien and also for landing the patch.