[webkit-reviews] review granted: [Bug 76246] Incorrect rendering of borders on <col> with span > 1 : [Attachment 166250] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 76246: Incorrect rendering of borders on <col> with span > 1
https://bugs.webkit.org/show_bug.cgi?id=76246

Attachment 166250: Patch
https://bugs.webkit.org/attachment.cgi?id=166250&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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-spa
n.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-spa
n.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).