[Webkit-unassigned] [Bug 5515] Border collapse problem with rowspan/colspan cells

[bugzilla-daemon at webkit.org]

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


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #161153|review?                     |review-
               Flag|                            |




--- Comment #33 from Julien Chaffraix <jchaffraix at webkit.org>  2012-09-06 16:55:46 PST ---
(From update of attachment 161153)
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.

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