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

mitz
Reported 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).
Attachments
test file (601 bytes, text/html)
2010-10-08 15:03 PDT, Xiaomei Ji
no flags
Patch (9.84 KB, patch)
2011-03-30 05:59 PDT, Levi Weintraub
no flags
Patch (9.92 KB, patch)
2011-03-30 06:16 PDT, Levi Weintraub
no flags
Patch (9.88 KB, patch)
2011-03-30 07:01 PDT, Levi Weintraub
no flags
Patch (9.86 KB, patch)
2011-03-30 07:24 PDT, Levi Weintraub
no flags
Patch (9.84 KB, patch)
2011-03-30 08:05 PDT, Levi Weintraub
no flags
mitz
Comment 1 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.
Xiaomei Ji
Comment 2 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?
Uri Bernstein
Comment 3 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.
Ryosuke Niwa
Comment 4 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?
Xiaomei Ji
Comment 5 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?
Xiaomei Ji
Comment 6 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?
Levi Weintraub
Comment 7 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?
Ryosuke Niwa
Comment 8 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.
Levi Weintraub
Comment 9 2011-03-30 05:59:40 PDT
Ryosuke Niwa
Comment 10 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?
Levi Weintraub
Comment 11 2011-03-30 06:16:20 PDT
Ryosuke Niwa
Comment 12 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.
WebKit Review Bot
Comment 13 2011-03-30 06:47:56 PDT
Early Warning System Bot
Comment 14 2011-03-30 06:56:12 PDT
Levi Weintraub
Comment 15 2011-03-30 07:01:12 PDT
WebKit Review Bot
Comment 16 2011-03-30 07:21:06 PDT
Levi Weintraub
Comment 17 2011-03-30 07:24:24 PDT
Levi Weintraub
Comment 18 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!
Early Warning System Bot
Comment 19 2011-03-30 07:29:57 PDT
WebKit Review Bot
Comment 20 2011-03-30 07:33:52 PDT
Early Warning System Bot
Comment 21 2011-03-30 07:43:38 PDT
Build Bot
Comment 22 2011-03-30 07:54:20 PDT
Levi Weintraub
Comment 23 2011-03-30 08:05:18 PDT
Levi Weintraub
Comment 24 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>
mitz
Comment 25 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?
Ryosuke Niwa
Comment 26 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.
mitz
Comment 27 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.
Ryosuke Niwa
Comment 28 2011-04-03 00:11:17 PDT
This bug has been fixed.
Note You need to log in before you can comment on or make changes to this bug.