Bug 38087 – Clicking below last line of right-to-left editable text that puts caret in the wrong place

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

In the URL, click in the bottom of the box, below the line of text. The care appears immediately after the letter and before the trailing space. Compare with what happens when you click in the top of the box (anywhere to the left of the text).

Simple test case:

<div dir="rtl" style="-webkit-user-modify: read-write; height: 2em; border: solid"><span>&#x05d0</span><span>&#x05d1</span><span>&#x05d2</span></div>

The problem is RenderBlock::positionForPointWithInlineChildren()’s use of firstLeafChild() and lastLeafChild() where it should probably get the logically first or last child in those cases. Or perhaps it could just avoid using line boxes in those cases and return the first/last positions inside the block.

[Xiaomei Ji]

Created attachment 70295 [details]
test file

I tried both 'div's in the test file in Chrome 7.0.544.0 dev in Windows.
when click in the top of the box, the caret appears at the very left of the text,
when click in the bottom of the box, the caret appears at the very right of the text.

It is the same as Firefox.

What should be the expected result? or the bug is fixed?

[Uri Bernstein]

I can reproduce the behavior described in comment #0 in r70618 on OS X. The cursor appears between the first and second letters, which is clearly wrong.

[Ryosuke Niwa]

For the very simple case as in:
<div dir="rtl" contenteditable style="border: solid 1px red; width: 150px; padding: 20px;"><span>א</span><span>ב</span><span>ג</span></div>

The following change suffice to fix the bug.

--- WebCore/rendering/RenderBlock.cpp	(revision 73592)
+++ WebCore/rendering/RenderBlock.cpp	(working copy)
@@ -4016,7 +4019,8 @@
     if (lastRootBoxWithChildren) {
         // We hit this case for Mac behavior when the Y coordinate is below the last box.
         ASSERT(moveCaretToBoundary);
-        return VisiblePosition(positionForBox(lastRootBoxWithChildren->lastLeafChild(), false), DOWNSTREAM);
+        InlineBox* logicallyLastBox = style()->isLeftToRightDirection() ? lastRootBoxWithChildren->lastLeafChild() : lastRootBoxWithChildren->firstLeafChild();
+        return VisiblePosition(positionForBox(logicallyLastBox, false), DOWNSTREAM);
     }

However, when we have LTR / RTL mixed content as in below, things get a tricker.
<div style="border: solid 1px red; width: 150px; padding: 20px;">hello<span>א</span><span>ב</span><span>ג</span></div>
What should we expect to get for this case?  Should it be between hello and א, which is the logical end of the line?  Or should it be to the right of ג since we're in a LTR block?

[Xiaomei Ji]

(In reply to comment #4)
> For the very simple case as in:
> <div dir="rtl" contenteditable style="border: solid 1px red; width: 150px; padding: 20px;"><span>א</span><span>ב</span><span>ג</span></div>
> 
> The following change suffice to fix the bug.
> 
> --- WebCore/rendering/RenderBlock.cpp    (revision 73592)
> +++ WebCore/rendering/RenderBlock.cpp    (working copy)
> @@ -4016,7 +4019,8 @@
>      if (lastRootBoxWithChildren) {
>          // We hit this case for Mac behavior when the Y coordinate is below the last box.
>          ASSERT(moveCaretToBoundary);
> -        return VisiblePosition(positionForBox(lastRootBoxWithChildren->lastLeafChild(), false), DOWNSTREAM);
> +        InlineBox* logicallyLastBox = style()->isLeftToRightDirection() ? lastRootBoxWithChildren->lastLeafChild() : lastRootBoxWithChildren->firstLeafChild();


logically last box might not be the visually first box.
Following is Dan's example:
<div dir=rtl>abc <span>def OPQ </span>RST</div>.



> +        return VisiblePosition(positionForBox(logicallyLastBox, false), DOWNSTREAM);
>      }
> 
> However, when we have LTR / RTL mixed content as in below, things get a tricker.
> <div style="border: solid 1px red; width: 150px; padding: 20px;">hello<span>א</span><span>ב</span><span>ג</span></div>
> What should we expect to get for this case?  Should it be between hello and א, which is the logical end of the line?  Or should it be to the right of ג since we're in a LTR block?

[Xiaomei Ji]

getLogicalEndBoxAndNode() in visible_units.cpp gets the last logical node in a line.
BTW, can you reproduce it in Windows?

[Levi Weintraub]

I've confirmed that this doesn't reproduce on Safari 5 or Chrome 9 on Windows.

I think using getLogicalEndBoxAndNode() is a valid fix.

(In reply to comment #6)
> getLogicalEndBoxAndNode() in visible_units.cpp gets the last logical node in a line.
> BTW, can you reproduce it in Windows?

[Ryosuke Niwa]

(In reply to comment #7)
> I've confirmed that this doesn't reproduce on Safari 5 or Chrome 9 on Windows.
> 
> I think using getLogicalEndBoxAndNode() is a valid fix.
> 
> (In reply to comment #6)
> > getLogicalEndBoxAndNode() in visible_units.cpp gets the last logical node in a line.
> > BTW, can you reproduce it in Windows?

It's really odd for RenderBlock to rely on visible_units.cpp though.  Perhaps, dependency becomes less odd once we implement RenderedPosition I proposed on webkit-dev.

[Levi Weintraub]

Created attachment 87509 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 87509 [details]
Patch

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

> Source/WebCore/editing/visible_units.cpp:1062
> -    getLogicalStartBoxAndNode(rootBox, logicalStartBox, logicalStartNode);
> +    rootBox->getLogicalStartBoxAndNode(logicalStartBox, logicalStartNode);

Maybe we should return node instead of void?

[Levi Weintraub]

Created attachment 87518 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 87518 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:4157
>          // We hit this case for Mac behavior when the Y coordinate is below the last box.

So this bug only reproduces on Mac?

> Source/WebCore/rendering/RenderBlock.cpp:4161
> +        Node* endNode = lastRootBoxWithChildren->getLogicalEndBoxWithNode(logicallyLastBox);
> +        if (endNode)

We should declare endNode inside the if as in:
if (Node* endNode = lastRootBoxWithChildren->getLogicalEndBoxWithNode(logicallyLastBox))

> Source/WebCore/rendering/RootInlineBox.cpp:798
> +        startNode = startBox->renderer()->node();
> +        if (startNode)
> +            return startNode;

We should do
if (leafBoxesInLogicalOrder[i]->renderer()->node()) {
    startBox = leafBoxesInLogicalOrder[i];
    return startBox->renderer()->node();
}
so that you don't have to assign startBox every iteration.  It also eliminates the need for startNode local variable.

[WebKit Review Bot]

Attachment 87518 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8298301

[Early Warning System Bot]

Attachment 87518 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8154136

[Levi Weintraub]

Created attachment 87528 [details]
Patch

[WebKit Review Bot]

Attachment 87528 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8281883

[Levi Weintraub]

Created attachment 87530 [details]
Patch

[Levi Weintraub]

(In reply to comment #17)
> Created an attachment (id=87530) [details]
> Patch

Sorry for the blatant EWS abuse!

[Early Warning System Bot]

Attachment 87528 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8298312

[WebKit Review Bot]

Attachment 87530 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8298313

[Early Warning System Bot]

Attachment 87530 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8305043

[Build Bot]

Attachment 87530 [details] did not build on win:
Build output: http://queues.webkit.org/results/8281891

[Levi Weintraub]

Created attachment 87537 [details]
Patch

[Levi Weintraub]

Comment on attachment 87537 [details]
Patch

Clearing flags on attachment: 87537

Committed r82447: <http://trac.webkit.org/changeset/82447>

Comment on attachment 87537 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:55
> +#include "visible_units.h"

Is this necessary?

> Source/WebCore/rendering/RootInlineBox.h:146
> +    Node* getLogicalEndBoxWithNode(InlineBox*&);

Can these be const?

[Ryosuke Niwa]

Comment on attachment 87537 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.cpp:55
>> +#include "visible_units.h"
> 
> Is this necessary?

Oh, good catch. This should definitely be removed. Levi, can you do that?

>> Source/WebCore/rendering/RootInlineBox.h:146
>> +    Node* getLogicalEndBoxWithNode(InlineBox*&);
> 
> Can these be const?

You mean
Node* getLogicalStartBoxWithNode(InlineBox*&) const;
Node* getLogicalEndBoxWithNode(InlineBox*&) const;
?  It seems like we can.

(In reply to comment #26)
> >> Source/WebCore/rendering/RootInlineBox.h:146
> >> +    Node* getLogicalEndBoxWithNode(InlineBox*&);
> > 
> > Can these be const?
> 
> You mean
> Node* getLogicalStartBoxWithNode(InlineBox*&) const;
> Node* getLogicalEndBoxWithNode(InlineBox*&) const;

Yes.

[Ryosuke Niwa]

This bug has been fixed.