Bug 38087

Summary: Clicking below last line of right-to-left editable text that puts caret in the wrong place
Product: WebKit Reporter: mitz
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dglazkov, enrica, leviw, leviw, rniwa, uriber, webkit-ews, webkit.review.bot, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
URL: data:text/html,<div dir="rtl" style="-webkit-nbsp-mode: space; -webkit-line-break: after-white-space; -webkit-user-modify: read-write; height: 2em; border: solid">&#x05d0;&nbsp;</div>
Attachments:
Description Flags
test file
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description mitz 2010-04-24 18:40:10 PDT
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).
Comment 1 mitz 2010-04-24 18:54:54 PDT
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.
Comment 2 Xiaomei Ji 2010-10-08 15:03:36 PDT
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?
Comment 3 Uri Bernstein 2010-10-27 03:50:33 PDT
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.
Comment 4 Ryosuke Niwa 2010-12-08 22:43:58 PST
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?
Comment 5 Xiaomei Ji 2010-12-09 12:22:26 PST
(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?
Comment 6 Xiaomei Ji 2010-12-09 15:03:36 PST
getLogicalEndBoxAndNode() in visible_units.cpp gets the last logical node in a line.
BTW, can you reproduce it in Windows?
Comment 7 Levi Weintraub 2011-01-11 11:08:08 PST
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?
Comment 8 Ryosuke Niwa 2011-02-08 01:46:38 PST
(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.
Comment 9 Levi Weintraub 2011-03-30 05:59:40 PDT
Created attachment 87509 [details]
Patch
Comment 10 Ryosuke Niwa 2011-03-30 06:11:28 PDT
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?
Comment 11 Levi Weintraub 2011-03-30 06:16:20 PDT
Created attachment 87518 [details]
Patch
Comment 12 Ryosuke Niwa 2011-03-30 06:34:28 PDT
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.
Comment 13 WebKit Review Bot 2011-03-30 06:47:56 PDT
Attachment 87518 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8298301
Comment 14 Early Warning System Bot 2011-03-30 06:56:12 PDT
Attachment 87518 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8154136
Comment 15 Levi Weintraub 2011-03-30 07:01:12 PDT
Created attachment 87528 [details]
Patch
Comment 16 WebKit Review Bot 2011-03-30 07:21:06 PDT
Attachment 87528 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8281883
Comment 17 Levi Weintraub 2011-03-30 07:24:24 PDT
Created attachment 87530 [details]
Patch
Comment 18 Levi Weintraub 2011-03-30 07:24:48 PDT
(In reply to comment #17)
> Created an attachment (id=87530) [details]
> Patch

Sorry for the blatant EWS abuse!
Comment 19 Early Warning System Bot 2011-03-30 07:29:57 PDT
Attachment 87528 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8298312
Comment 20 WebKit Review Bot 2011-03-30 07:33:52 PDT
Attachment 87530 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8298313
Comment 21 Early Warning System Bot 2011-03-30 07:43:38 PDT
Attachment 87530 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8305043
Comment 22 Build Bot 2011-03-30 07:54:20 PDT
Attachment 87530 [details] did not build on win:
Build output: http://queues.webkit.org/results/8281891
Comment 23 Levi Weintraub 2011-03-30 08:05:18 PDT
Created attachment 87537 [details]
Patch
Comment 24 Levi Weintraub 2011-03-30 08:21:51 PDT
Comment on attachment 87537 [details]
Patch

Clearing flags on attachment: 87537

Committed r82447: <http://trac.webkit.org/changeset/82447>
Comment 25 mitz 2011-03-30 08:41:51 PDT
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?
Comment 26 Ryosuke Niwa 2011-03-30 12:55:52 PDT
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.
Comment 27 mitz 2011-03-30 13:31:50 PDT
(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.
Comment 28 Ryosuke Niwa 2011-04-03 00:11:17 PDT
This bug has been fixed.