Bug 104794 – Unable to place the caret at the end of the first line, when followed by a block, in the vertical writing mode.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Arpita Bahuguna]

Created attachment 179021 [details]
Testcase

Fetch the attached test file and try placing the cursor at the end of the first line (in the vertical mode) by clicking at the bottom of that line.

Notice that we are not able to place the cursor at the end position of that line. However, no such issue occurs while trying to place the caret at the end of the second line.

Also, this issue is not observed in the horizontal writing mode.

[Arpita Bahuguna]

The issue is also reproducible in the case of some text followed by an empty (non-empty) block. Changing the bug title to reflect the same.

[Arpita Bahuguna]

Created attachment 179713 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 179713 [details]
Patch

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-vertical-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-vertical-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-vertical-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>.

[Ryosuke Niwa]

Comment on attachment 179713 [details]
Patch

Oh I guess you haven’t gotten committership yet so r- instead. Please upload a new patch. Also, you should probably set cq? flag so that your patch maybe landed.

[Arpita Bahuguna]

Created attachment 179927 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #3)
> (From update of attachment 179713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179713&action=review
> 
Hi rniwa, thanks for the 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-vertical-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.
> 
I agree, my bad. I was initially trying to make the test case in another way and forgot to change the name of this variable thereafter.

> > LayoutTests/editing/selection/caret-at-end-of-text-line-followed-by-block-in-vertical-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-vertical-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>.

Have made the suggested changes.

[Ryosuke Niwa]

Comment on attachment 179927 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179927&action=review

> LayoutTests/editing/selection/caret-at-end-of-text-line-followed-by-block-in-vertical-mode.html:30
> +    debug("The caret at the start of the text should be the same as anywhere else on that line (beyond the text), and should be drawn on the same line.")

This debug comment seems redundant.

> LayoutTests/editing/selection/caret-at-end-of-text-line-followed-by-block-in-vertical-mode.html:38
> +    debug("To manually verify the issue, try clicking on the empty region of the first vertical line. The caret should be drawn at the end of the text.");

You can put this in description() separated by "\n".

[Arpita Bahuguna]

Created attachment 180087 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #7)
> (From update of attachment 179927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179927&action=review
> 
Thanks for the review rniwa. Have uploaded another patch with the specified changes.

[Ryosuke Niwa]

Comment on attachment 180087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180087&action=review

> LayoutTests/editing/selection/caret-at-end-of-text-line-followed-by-block-in-vertical-mode.html:34
> +    isSuccessfullyParsed();

I think we define "var successfullyParsed = true;” at the end of the script element instead.

[WebKit Review Bot]

Comment on attachment 180087 [details]
Patch

Attachment 180087 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15401528

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Arpita Bahuguna]

Created attachment 180116 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #10)
> (From update of attachment 180087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180087&action=review
> 
> > LayoutTests/editing/selection/caret-at-end-of-text-line-followed-by-block-in-vertical-mode.html:34
> > +    isSuccessfullyParsed();
> 
> I think we define "var successfullyParsed = true;” at the end of the script element instead.

Have uploaded another patch by removing the call to isSuccessfullyParsed() altogether and included js-test-post.js instead.

Also, the test shown as failing on the cr-linux port: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html does not fail on my local builds.

[WebKit Review Bot]

Comment on attachment 180116 [details]
Patch

Clearing flags on attachment: 180116

Committed r138169: <http://trac.webkit.org/changeset/138169>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.