[webkit-reviews] review granted: [Bug 104794] Unable to place the caret at the end of the first line, when followed by a block, in the vertical writing mode. : [Attachment 179713] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 104794: Unable to place the caret at the end of the first line, when
followed by a block, in the vertical writing mode.
https://bugs.webkit.org/show_bug.cgi?id=104794

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179713&action=review


r=me for the code change but please fix the test before landing it.

>
LayoutTests/editing/selection/caret-at-end-of-text-line-followed-by-block-in-ve
rtical-mode.html:18
> +	   var textNode = document.getElementById('textDiv');

It’s very misleading to call a div "textNode". It’s better to call it testDiv
instead.

>
LayoutTests/editing/selection/caret-at-end-of-text-line-followed-by-block-in-ve
rtical-mode.html:19
> +	   eventSender.mouseMoveTo(textNode.offsetWidth - textNode.offsetLeft,
textNode.offsetTop);

Why are you subtracting left from width?? If anything, we should have
testDiv.offsetLeft + testDiv.offsetWidth - 5;
The last time is to place the caret on the left of the edge of the containing
div.

>
LayoutTests/editing/selection/caret-at-end-of-text-line-followed-by-block-in-ve
rtical-mode.html:24
> +	   eventSender.mouseMoveTo(textNode.offsetWidth - textNode.offsetLeft,
textNode.offsetHeight - textNode.offsetTop);

Ditto. Here, we should have textNode.offsetTop + textNode.offsetHeight - 5; the
last term (-5) could be any number you’d like as long as it’s not longer than
100 - <height of text>.