Bug 33435 – selection.modify("move", "left", "lineboundary") in RTL text moves to the right lineboundary

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Justin Lebar]

Created attachment 46224 [details]
Testcase #3 from Mozilla bug 496275

This was noticed in https://bugzilla.mozilla.org/show_bug.cgi?id=496275, where Gecko is adding DOMSelection.modify to match WebKit.

If I call selection.modify("move", "left", "lineboundary") from inside a contenteditable div with dir="rtl", the cursor moves to the right line boundary, while I expect it to move to the left.

Similarly, selection.modify("move", "right", "lineboundary") moves the cursor to the left.

[Justin Lebar]

Created attachment 46225 [details]
Testcase #3 from Mozilla bug 496275 (better encoding?)

Trying to fix encoding of attachment.

[Hajime Morrita]

Created attachment 50484 [details]
reduction

[Xiaomei Ji]

Created attachment 75845 [details]
patch w/ layout test

[WebKit Review Bot]

Attachment 75845 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 75845 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=75845&action=review

> WebCore/editing/SelectionController.cpp:432
> +    case LineBoundary:
> +        if (directionOfEnclosingBlock() == LTR)
> +            pos = visualEndOfLine(startForPlatform());
> +        else
> +            pos = visualStartOfLine(startForPlatform());
> +        break;

We should just do:
if (directionOfEnclosingBlock() == LTR)
    return visualEndOfLine(startForPlatform());
return visualStartOfLine(startForPlatform());

and modify
pos = modifyMovingForward(granularity);
to
return modifyMovingForward(granularity);

I don't know why we have a VisiblePosition local variable in the first place.

> WebCore/editing/visible_units.cpp:1247
> +    // Current implementation takes the 1st alternative. The caret will be
> +    // displayed on the leftmost visual position for logical start of line in
> +    // LTR paragraph and on the right most visual position for the RTL paragraphs.
> +    return visualStartOfLine(c);
> +}
> +
> +VisiblePosition logicalEndOfLine(const VisiblePosition& c)
> +{
> +    // TODO: this is the current behavior that might need to be fixed.
> +    return visualEndOfLine(c);
> +}

Does this mean that logical/visual behaviors are the same, and we're not changing the behavior at all this in patch?

[Ryosuke Niwa]

Comment on attachment 75845 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=75845&action=review

>> WebCore/editing/SelectionController.cpp:432
>> +        break;
> 
> We should just do:
> if (directionOfEnclosingBlock() == LTR)
>     return visualEndOfLine(startForPlatform());
> return visualStartOfLine(startForPlatform());
> 
> and modify
> pos = modifyMovingForward(granularity);
> to
> return modifyMovingForward(granularity);
> 
> I don't know why we have a VisiblePosition local variable in the first place.

I'm confused about this change now.  Why are we calling different visual* functions depending on the directionality of text block?  Shouldn't visualStartOfLine always return the visually left-most offset?  When we say the start of line in RTL text, is it the right edge of the text?  Because your change seems to indicate that.

[Xiaomei Ji]

Comment on attachment 75845 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=75845&action=review

>>> WebCore/editing/SelectionController.cpp:432
>>> +        break;
>> 
>> We should just do:
>> if (directionOfEnclosingBlock() == LTR)
>>     return visualEndOfLine(startForPlatform());
>> return visualStartOfLine(startForPlatform());
>> 
>> and modify
>> pos = modifyMovingForward(granularity);
>> to
>> return modifyMovingForward(granularity);
>> 
>> I don't know why we have a VisiblePosition local variable in the first place.
> 
> I'm confused about this change now.  Why are we calling different visual* functions depending on the directionality of text block?  Shouldn't visualStartOfLine always return the visually left-most offset?  When we say the start of line in RTL text, is it the right edge of the text?  Because your change seems to indicate that.

The "pos = ..." style is in all modifyMoving.... and modifyExtending.... functions. I feel it is more easier to read and to debug. But I do not have strong opinions on it.

I agree the visualStart/EndOfLine names are confusing. In the current patch,  visual start of line in RTL text is the right edge of text. I think using left/right will be unambiguous, and I will change to that.

>> WebCore/editing/visible_units.cpp:1247
>> +}
> 
> Does this mean that logical/visual behaviors are the same, and we're not changing the behavior at all this in patch?

I am not sure what should be the correct behavior of logical start/end of line.

I just read the draft specs for bidi editing by Matitiahu Allouche of IBM, the chairman of the Standards Institute of Israel's committee on IT aspects of Hebrew.
On "HOME/END" issue it says:
"Home and End keys are logical functions which move the current logical position before the first logical character (whatever its Bidi level) and after the last logical character (of the line, sentence, paragraph etc... according to whatever unit of text the non-bidi Home and End functions relate to).
The caret will be displayed on the leftmost visual position for Home in LTR paragraphs and End in RTL paragraphs, on the rightmost visual position for End in LTR paragraphs and Home in RTL paragraphs. Beside moving the caret position, Home and End must set the caret direction in accordance with the base direction of the the paragraph."

Which matches our current implementation.

In the current patch, MovingLeft/Right on line boundary always moves caret to the left/right edge of the line regardless of containing block's directionality. MovingForward on line boundary moves caret to the right edge of line if the containing block is LTR, and it moves caret to the left edge of the line if the containing block is RTL.

I prefer not changing the logical behavior in the same patch. There is a bug filed against HOME/END issue: https://bugs.webkit.org/show_bug.cgi?id=49107.
If we are going to change that, I'd prefer in a different patch.

[Xiaomei Ji]

Created attachment 76149 [details]
patch w/ layout test

[Ryosuke Niwa]

Comment on attachment 76149 [details]
patch w/ layout test

Thanks for the clarification.  The patch makes a lot more sense now.

View in context: https://bugs.webkit.org/attachment.cgi?id=76149&action=review

> WebCore/editing/visible_units.cpp:1160
> +    // There is discussion on the logical start of line in the context of
> +    // bidirectional text.
> +    // For example, if the paragraph direction is RTL, given the following text
> +    // in logical order: "hello WORLD", its visual display will be:
> +    // "DLROW hello". Logical start of line should be a logical position before
> +    // the first logical character in a line. Should it map to the visual position
> +    // at the right of 'o' in "hello", or should it map to the visual position 
> +    // at the left of 'h' in "hello"?
> +    // For example, if the paragraph direction is LTR, given a text 
> +    // "HELLO world", it is visual display will be "OLLEH world".

Nit: "its" visual display

> WebCore/editing/visible_units.cpp:1167
> +    // There is discussion on the logical start of line in the context of
> +    // bidirectional text.
> +    // For example, if the paragraph direction is RTL, given the following text
> +    // in logical order: "hello WORLD", its visual display will be:
> +    // "DLROW hello". Logical start of line should be a logical position before
> +    // the first logical character in a line. Should it map to the visual position
> +    // at the right of 'o' in "hello", or should it map to the visual position 
> +    // at the left of 'h' in "hello"?
> +    // For example, if the paragraph direction is LTR, given a text 
> +    // "HELLO world", it is visual display will be "OLLEH world".
>  
>  
> +    // Should the logical start of line map to visual position at left of 
> +    // 'O' in "OLLLEH", or should it map to the visual position at right of
> +    // 'H' in "OLLEH"?
> +    // 
> +    // Current implementation takes the 1st alternative. The caret will be
> +    // displayed on the leftmost visual position for logical start of line in
> +    // LTR paragraph and on the right most visual position for the RTL paragraphs.

I'm not sure if we should include all of this in the code.  We should just post this on the bug 49107.  We can still add the first two lines and if someone wanted to know what's the discusion, he/she can open the bug and read.

> WebCore/editing/visible_units.cpp:1184
> +// For a LTR InlineBox, the logical end position for the line is the maximum caret offset of the last
> +// logical node in the line. 
> +// For example, if the last logical node in the line is a LTR node, the node and its position information is
> +// |......(n)a(n+1)b(n+2)c(n+3)|.
> +// If the last logical node in the line is a RTL node, the node and its position information is
> +// |......(n)C(n+2)B(n+1)A(n+3)|.
> +//
> +// Same for a RTL InlineBox.
> +// For example, if the last logical node in the line is a LTR node, the node and its position information is
> +// |(n+3)a(n+1)b(n+2)c(n).......|.
> +// If the last logical node in the line is a RTL node, the node and its position information is
> +// |(n+3)C(n+2)B(n+1)A(n).......|.

I'm not even sure if we should have this comment.  I suppose this is fairly complex function, but we don't normally add a long comment like this to explain a function.  You can add some comment but I think this one is too verbose.

> WebCore/editing/visible_units.cpp:1246
> +    if (direction == LTR)
> +        return logicalStartOfLine(c);
> +    return logicalEndOfLine(c);

Great. This code makes a lot more sense now!

You can do:
return direction == LTR ? logicalStartOfLine(c) : logicalEndOfLine(c);

In fact, you can make it an inline function since this is really short.  It might give us a little performance improvement since instantiating VisiblePosition is expensive.

> WebCore/editing/visible_units.cpp:1253
> +    if (direction == LTR)
> +        return logicalEndOfLine(c);
> +    return logicalStartOfLine(c);

Ditto.

> WebCore/editing/visible_units.h:64
> +VisiblePosition rightEdgeOfLine(const VisiblePosition &, TextDirection);
> +VisiblePosition leftEdgeOfLine(const VisiblePosition &, TextDirection);

I'm not sure if "Edge" makes sense.  "edge" seems to indicate the edge of the containing block, which isn't really the case here.
Maybe visualRightOfLine or rightBoundaryOfLine?

> LayoutTests/editing/selection/home-end.html:77
>                  var positionsMovingRight;
>                  var positionsMovingLeft;

Maybe you should rename these variables

[Xiaomei Ji]

Created attachment 76459 [details]
patch w/ layout test

update per rniwa's feedback except I did not make left/rightBoundarOfLine() inline to avoid introducing dependency on VisiblePosition.h.

[Xiaomei Ji]

Committed r75018: <http://trac.webkit.org/changeset/75018>