Created attachment 61095 [details] test case See test case
Created attachment 61098 [details] testcase2 it's not clear (to me) how your testcase works because there is no second line (which is mentioned in the instructions) so i'll also add my own:
In Chromium bug: http://code.google.com/p/chromium/issues/detail?id=46239
Comment on attachment 61095 [details] test case Obsoleting since Yair's test case is clearer.
IMHO the desired behavior here with the up/down arrow keys on multiline text match the LTR case. There may be corner cases I'm not aware of, but for this testcase I think things are pretty clear cut.
This seems to be a bug in RenderText::positionForPoint. The following change fixes the issue: Index: WebCore/rendering/RenderText.cpp =================================================================== --- WebCore/rendering/RenderText.cpp (revision 70891) +++ WebCore/rendering/RenderText.cpp (working copy) @@ -423,7 +423,7 @@ // at the y coordinate of the first line or above // and the x coordinate is to the left of the first text box left edge offset = firstTextBox()->offsetForPosition(point.x()); - return createVisiblePosition(offset + firstTextBox()->start(), DOWNSTREAM); + return createVisiblePosition(offset + firstTextBox()->start(), offset > 0 ? VP_UPSTREAM_IF_POSSIBLE : DOWNSTREAM); } if (lastTextBox() && point.y() >= lastTextBox()->root()->lineTop() && point.x() >= lastTextBox()->m_x + lastTextBox()->logicalWidth()) { // at the y coordinate of the last line or below I'll write some tests and post a patch tomorrow.
My suggested change fixes the testcase but not the testcase2: <div dir="rtl" contenteditable style="width: 5ex;">גכ יגכעי ג</div> In this editable region, the problem still reproduces even with my change. The problem seems to be in startPositionForLine in visible_units.cpp in the following stack trace: #0 0x10213da38 in WebCore::rootBoxForLine at visible_units.cpp:358 #1 0x10213f100 in WebCore::startPositionForLine at visible_units.cpp:376 #2 0x10213f295 in WebCore::startOfLine at visible_units.cpp:419 #3 0x10213f4c4 in WebCore::inSameLine at visible_units.cpp:508 #4 0x102143749 in WebCore::VisiblePosition::init at VisiblePosition.cpp:64 #5 0x102143803 in WebCore::VisiblePosition::VisiblePosition at VisiblePosition.cpp:54 #6 0x101eedbe0 in WebCore::RenderObject::createVisiblePosition at RenderObject.cpp:2636 #7 0x101f4c74d in WebCore::RenderText::positionForPoint at RenderText.cpp:426 We're trying to decide whether or not we should keep UPSTREAM in VisiblePosition's constructor. To do that, we check whether or not DOWNSTREAM and UPSTREAM belong to the same line. However, we're obtaining a wrong visible position for the DOWNSTREAM. In startPositionForLine, we call rootBoxForLine(c) and retrieve the first leaf child of the root box. However, this the first leaf child (offset 8) is different from the box we had in rootBoxForLine (offset 3). +Justin, Harrison, Eric, & Darin who seem to have worked on startPositionForLine according to http://trac.webkit.org/browser/trunk/WebCore/editing/visible_units.cpp?annotate=blame&rev=70932
note: the first test case seems to only work under Linux (in Windows the lines doesn't wrap -at least in my computers)
(In reply to comment #7) > note: the first test case seems to only work under Linux (in Windows the lines doesn't wrap -at least in my computers) That's quite irrelevant here since I can always adjust the width and reproduce the issue but yes, we shouldn't be specifying the width of div by px. we should be using ex instead.
Xiaomei is right, removing the following code after adding my suggested change will fix the issue: if (visPos.isNotNull()) { // Make sure the start of line is not greater than the given input position. Else use the previous position to // obtain start of line. This condition happens when the input position is before the space character at the end // of a soft-wrapped non-editable line. In this scenario, startPositionForLine would incorrectly hand back a position // greater than the input position. This fix is to account for the discrepancy between lines with webkit-line-break:after-white-space // style versus lines without that style, which would break before a space by default. Position p = visPos.deepEquivalent(); if (p.deprecatedEditingOffset() > c.deepEquivalent().deprecatedEditingOffset() && p.node()->isSameNode(c.deepEquivalent().node())) { visPos = c.previous(); if (visPos.isNull()) return VisiblePosition(); visPos = startPositionForLine(visPos); } } This change was added in http://trac.webkit.org/changeset/23608 but I have no idea what it's doing since it doesn't have a test or bug. Could someone from Apple elaborate on what rdar://problem/5237325 is?
From the conversion I had with Enrica: "The real problem is due to how inline text boxes are laid out in RenderBlock, not accounting for the space character at the end of a wrapped line if the line lacks a webkit-line-break:after-white-space style. So an inline text box without webkit-line-break:after-white-space style will have a length one less than that of an inline text box containing webkit-line-break:after-white-space style. This discrepancy, in turn, affects the result of RenderText's isAtLineWrap routine, which then affect which inlineTextBox is fetched by startOfLine/endOfLine."
Created attachment 74013 [details] failed attempt to write a test for r23608 I tried to make a test case per Enrica's comment & comment in the code but the problem doesn't reproduce anymore.
Created attachment 74016 [details] test case(?) for r23608 Oops, my previous test had a typo. It was missing leading "-". However, now the problem seems to reproduce on TOT as well regardless of the code's presence.
Created attachment 74022 [details] Patch
Could someone review my patch?
Comment on attachment 74022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74022&action=review > WebCore/rendering/RenderText.cpp:432 > + return createVisiblePosition(offset + firstTextBox()->start(), offset > 0 && firstTextBox()->start() ? VP_UPSTREAM_IF_POSSIBLE : DOWNSTREAM); I don’t understand why an offset of 0 is a special case. What are the conditions where the offset is 0?
Comment on attachment 74022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74022&action=review >> WebCore/rendering/RenderText.cpp:432 >> + return createVisiblePosition(offset + firstTextBox()->start(), offset > 0 && firstTextBox()->start() ? VP_UPSTREAM_IF_POSSIBLE : DOWNSTREAM); > > I don’t understand why an offset of 0 is a special case. What are the conditions where the offset is 0? We check that condition because if offset was 0, then we're at the beginning of LTR text, and we don't want to move to the upstream in that case. See http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderText.cpp#L421.
Comment on attachment 74022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74022&action=review >>> WebCore/rendering/RenderText.cpp:432 >>> + return createVisiblePosition(offset + firstTextBox()->start(), offset > 0 && firstTextBox()->start() ? VP_UPSTREAM_IF_POSSIBLE : DOWNSTREAM); >> >> I don’t understand why an offset of 0 is a special case. What are the conditions where the offset is 0? > > We check that condition because if offset was 0, then we're at the beginning of LTR text, and we don't want to move to the upstream in that case. See http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderText.cpp#L421. So offset of 0 can’t mean anything about collapsed spaces or visibility:hidden text or the like?
Comment on attachment 74022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74022&action=review >>>> WebCore/rendering/RenderText.cpp:432 >>>> + return createVisiblePosition(offset + firstTextBox()->start(), offset > 0 && firstTextBox()->start() ? VP_UPSTREAM_IF_POSSIBLE : DOWNSTREAM); >>> >>> I don’t understand why an offset of 0 is a special case. What are the conditions where the offset is 0? >> >> We check that condition because if offset was 0, then we're at the beginning of LTR text, and we don't want to move to the upstream in that case. See http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderText.cpp#L421. > > So offset of 0 can’t mean anything about collapsed spaces or visibility:hidden text or the like? Ah, that's a good point. I think the condition for firstTextBox()->start() is wrong. It should just be checking offset. But "offset" shouldn't be affected by collapsed spaces or visibility:hidden because it's the offset inside the inline text box.
Created attachment 74434 [details] removed the condition that firstTextBox()->start() > 0
Comment on attachment 74434 [details] removed the condition that firstTextBox()->start() > 0 r=me
Committed r72861: <http://trac.webkit.org/changeset/72861>
http://trac.webkit.org/changeset/72861 might have broken Qt Linux Release The following tests are not passing: editing/selection/click-left-of-rtl-wrapping-text.html editing/selection/extend-selection-home-end.html editing/selection/modify-up-on-rtl-wrapping-text.html