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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled 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 161966: Patch
https://bugs.webkit.org/attachment.cgi?id=161966&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.p
ng: Added.
> +	   *
fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.t
xt: 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-spa
n.html:26
> +<col span=3 style="border: 1px solid red;"></col>

red should be reserved for failure.