[webkit-reviews] review canceled: [Bug 69723] Visual glitch in webkit html table respect both firefox and IE : [Attachment 137149] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Shezan Baig
<shezbaig.wk at gmail.com>'s request for review:
Bug 69723: Visual glitch in webkit html table respect both firefox and IE
https://bugs.webkit.org/show_bug.cgi?id=69723

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137149&action=review


This bug is a duplicate of bug 20840 for which I posted a patch some time ago
but never followed up. Please check for duplicates when fixing bugs as this is
a super old bug.

> Source/WebCore/rendering/RenderTableCell.cpp:417
> +bool RenderTableCell::isHorizontallyWithin(const RenderTableCell* other)
const
> +{
> +    return col() >= other->col()
> +	   && col() + colSpan() <= other->col() + other->colSpan();
> +}
> +
> +bool RenderTableCell::isVerticallyWithin(const RenderTableCell* other) const

> +{
> +    return row() >= other->row()
> +	   && row() + rowSpan() <= other->row() + other->rowSpan();
> +}

I don't really think that matches other browsers. Also I really think it's not
a good change: it completely ignores the fact that the 2 cells can be in any
configurations (above / under, left / right) and thus only checks for some of
the cases.

It's also not specified in CSS 2.1 so we should keep our behavior simple and
not tied to a complex algorithm. The best way would be to get this behavior
specified (likely matching FF, which means resolving collapsed borders by
comparing adjacent cells even if that means cutting the border for colspan or
rowspan) and implement it properly.

>
LayoutTests/fast/table/border-collapsing/col-row-span-collapsing-expected.txt:1

> +CONSOLE MESSAGE: line 67: TypeError: 'undefined' is not an object
(evaluating 'divs[i].style.display = "none"')

We prefer tests that don't have exceptions.