[Webkit-unassigned] [Bug 33435] selection.modify("move", "left", "lineboundary") in RTL text moves to the right lineboundary

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33435





--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org>  2010-12-10 09:40:23 PST ---
(From update of attachment 76149)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.