[Webkit-unassigned] [Bug 76246] Incorrect rendering of borders on <col> with span > 1

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76246


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #166250|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #15 from Julien Chaffraix <jchaffraix at webkit.org>  2012-10-01 12:20:32 PST ---
(From update of attachment 166250)
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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.