Bug 41987 – up arrow doesn't work with RTL text with word wrapping

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ojan Vafai]

Created attachment 61095 [details]
test case

See test case

[Yair Yogev]

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:

[Xiaomei Ji]

In Chromium bug: 
http://code.google.com/p/chromium/issues/detail?id=46239

[Jeremy Moskovich]

Comment on attachment 61095 [details]
test case

Obsoleting since Yair's test case is clearer.

[Jeremy Moskovich]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

My suggested change fixes the testcase but not the testcase2:
<div dir="rtl" contenteditable style="width: 5ex;">&#1490;&#1499; &#1497;&#1490;&#1499;&#1506;&#1497; &#1490;</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

[Yair Yogev]

note: the first test case seems to only work under Linux (in Windows the lines doesn't wrap -at least in my computers)

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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."

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 74022 [details]
Patch

[Ryosuke Niwa]

Could someone review my patch?

[Darin Adler]

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?

[Ryosuke Niwa]

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.

[Darin Adler]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 74434 [details]
removed the condition that firstTextBox()->start() > 0

[Dave Hyatt]

Comment on attachment 74434 [details]
removed the condition that firstTextBox()->start() > 0

r=me

[Ryosuke Niwa]

Committed r72861: <http://trac.webkit.org/changeset/72861>

[WebKit Review Bot]

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