Summary: | Clicking below last line of right-to-left editable text that puts caret in the wrong place | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||||||||||
Component: | HTML Editing | Assignee: | 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">א </div> | ||||||||||||||||
Attachments: |
|
Description
mitz
2010-04-24 18:40:10 PDT
Simple test case: <div dir="rtl" style="-webkit-user-modify: read-write; height: 2em; border: solid"><span>א</span><span>ב</span><span>ג</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. 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?
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. 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? (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? getLogicalEndBoxAndNode() in visible_units.cpp gets the last logical node in a line. BTW, can you reproduce it in Windows? 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? (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. Created attachment 87509 [details]
Patch
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? Created attachment 87518 [details]
Patch
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. Attachment 87518 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8298301 Attachment 87518 [details] did not build on qt: Build output: http://queues.webkit.org/results/8154136 Created attachment 87528 [details]
Patch
Attachment 87528 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8281883 Created attachment 87530 [details]
Patch
(In reply to comment #17) > Created an attachment (id=87530) [details] > Patch Sorry for the blatant EWS abuse! Attachment 87528 [details] did not build on qt: Build output: http://queues.webkit.org/results/8298312 Attachment 87530 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8298313 Attachment 87530 [details] did not build on qt: Build output: http://queues.webkit.org/results/8305043 Attachment 87530 [details] did not build on win: Build output: http://queues.webkit.org/results/8281891 Created attachment 87537 [details]
Patch
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? 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. This bug has been fixed. |