[webkit-reviews] review granted: [Bug 74864] There is additional space not belonged to a table between the table cells : [Attachment 157438] 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 74864: There is additional space not belonged to a table between the table
cells
https://bugs.webkit.org/show_bug.cgi?id=74864

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

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


> Source/WebCore/ChangeLog:17
> +		  fast/dom/hittest-tablecell-with-borders-right-edge.html

I didn't notice that those tests were in fast/dom. It doesn't make sense to put
them there - you are not testing a dom API - and they would be better with the
other table tests in fast/tables. If you prefer you could even put them in a
sub-directory (fast/table/testing).

> Source/WebCore/rendering/RenderTableSection.cpp:1129
> +    // of other browsers.

Woua, that's a long explanation! It's also half true as spanningRows /
spanningColumns are used for hit testing but also painting.

Let's try to shorten the comment (feel free to adapt):
// lower_bound doesn't handle the edge between 2 cells properly as it would
wrongly return the cell on the logical top / left.
// upper_bound properly return the cell on the logical bottom / right, which
matches other browsers.

> LayoutTests/fast/dom/hittest-tablecell-bottom-edge-expected.txt:41
> +PASS document.elementFromPoint(middleX, edge - 1).id is ''

This result is interesting. Note that this means that there is a 1px gap (or
more) around an empty rows. This matches the empty column case too. As this
patch makes us only progress, I am in favor of filing a follow-up bug about
that.

>
LayoutTests/fast/dom/hittest-tablecell-with-borders-right-edge-expected.txt:47
> +PASS document.elementFromPoint(edge - 1, middleY).id is ''
> +PASS document.elementFromPoint(edge, middleY).id is ''
> +PASS document.elementFromPoint(edge + 1, middleY).id is ''

I don't think this is fine to return nothing in all those cases but you are
just validating our existing behavior here.