Bug 24303 – Using keyboard select RTL text, Highlight goes to opposite direction from FF&IE

• Component: HTML Editing
• Status: RESOLVED FIXED

[Xiaomei Ji]

Test case is attached.

Currently, Chrome/Webkit text selection always follows logical order, no 
matter on bidi text or on pure RTL text, no matter in an RTL text box (for 
example, set dir="rtl") or in LTR text box.

By logical order, I mean shift-left-arrow moves cursor logically backward, 
while shift-right-arrow moves cursor logically forward.


But that is not the case for FF and IE.

For FF&IE, in LTR text box, text selection followings logical order, same 
as Chrome.


But in RTL text box, FF&IE follows anti-logical order.

Given the pure RTL text in the test file, text selection follows the same 
order as visual order.
Given the 1st bidi text example in the test file, text selection follows 
anti-logical order, which means shift-left-arrow moves cursor logically 
forward, while shift-right-arrow moves cursor logically backward.

[Xiaomei Ji]

Created attachment 28200 [details]
test case

[Xiaomei Ji]

Also in Chrome bug:
http://code.google.com/p/chromium/issues/detail?id=8141

I am not a native speaker, and I am not sure which way is better.
Would like to get a confirmation that this is designed by intention or this is a bug.

Following is a comment from a native speaker in Chrome's bugdb.

"all i know is that as a Hebrew native speaker (and i can speak for most Israelis as 
99% here use windows) having to click right to select left is very confusing
i am sure there is a reason for that in an LTR aligned box as it's that way in 
windows too, but in an RTL box we all got used to... meaning the direction we are 
clicking ;)"

[Xiaomei Ji]

screenshot from progame:

"i made a screenshot comparing the behaviour of Chrome/IE/FF while selecting text in 
an RTLed textbox

http://img24.imageshack.us/img24/7484/chromeselectrtl.jpg
"

[Xiaomei Ji]

The character and word text selection (using shift-arrow and ctrl-shift-arrow) difference of Webkit and FF/IE is:
although they all use logical order in text selection in LTR text box,
in RTL text box, Webkit still use logical order (shift+right arrow moves text forward, and shift+left arrow moves text backward), but FF/IE uses reversed logical order (shift+right arrow moves text backward, and shift+left arrow moves text forward).

If this is a bug in WebKit,
can we change SelectionController::modifyExtendingRightForward(),
in the cases of CharacterGranularity and WordGranularity, check whether
"pos.deepEquivalent().node()->renderer()->style()->direction() == WebCore::RTL",
extend selection to leftBackward if it is RTL, and extend selection to rightForward if it is LTR?

Or maybe introduce a new method to handle the forward/backward based on element style as above.

Same applies to SelectionController::modifyExtendingLeftBackward().

Please let me know whether this is a bug and whether the above works without complication, and I can start to work on the fix.

(In reply to comment #4)
> If this is a bug in WebKit,
> can we change SelectionController::modifyExtendingRightForward(),
> in the cases of CharacterGranularity and WordGranularity, check whether
> "pos.deepEquivalent().node()->renderer()->style()->direction() ==
> WebCore::RTL",
> extend selection to leftBackward if it is RTL, and extend selection to
> rightForward if it is LTR?
> 
> Or maybe introduce a new method to handle the forward/backward based on element
> style as above.
> 
> Same applies to SelectionController::modifyExtendingLeftBackward().

I think the methods should be split into modifyExtendingForward(), modifyExtendingBackward(), which should retain the existing behavior, and modifyExtendingRight() and modifiyExtendingLeft(), which should behave "visually", essentially mimicking what was done for modifyMoving* in <http://trac.webkit.org/changeset/32605>.

I do not think that examining the direction property of the renderer would lead to the best results. I am unfamiliar with the Windows and Firefox behavior, but in the Cocoa text system on Mac OS X, the actual direction of the text runs (i.e. the resolved bidi level) is ultimately used to determine the position to move the selection's extent to; this is very similar to how moving the selection works in WebKit following r32605. So I think a better approximation of the desired behavior would be to use VisualPosition::right() and VisualPosition::left() to determine where to move the extent to, and refine as necessary and as possible when the extent is at a boundary between levels, although as long as Selection is limited to a single range, there is only so much that can be done to maintain the "visual" behavior in the bidirectional cases.

[Xiaomei Ji]

Maybe I did not understand what you mean by "visually" completely.

I attached the Firefox selection process.

I think Firefox does not use "visual" selection in RTL text box, it is still logical order, but it is a mirrored (or reversed) logical order. 

Using the test case that illustrates FF behavior, if we are using "visual order" (VisualPosition::left()), when the cursor is between 'x' and 'y', when we press shift+left_arrow, 
should the highlight highlights character 'x'? Or by "refine as
necessary and as possible when the extent is at a boundary between levels", it is able to highlight only character 'y'?
(the positions around "xyz" are: '(9) (8)x(6)y(7)z(5) (4)C(3)B(2)A(1)dummy_RTL')

Even worse, when the cursor is between 'y' and 'z', using VisualPosition::left(), shift-left_arrow will select character 'y' (it is within the same run, should not be any complication), but FF selects character 'z'.

[Xiaomei Ji]

Created attachment 28321 [details]
FF text selection by pressing shift-left_arrow

(In reply to comment #7)
> Created an attachment (id=28321) [review]
> FF text selection by pressing shift-left_arrow

I see. That is definitely not the way the Cocoa text system on Mac OS X works, so a setting may be required to satisfy different platforms' conventions.

(In reply to comment #8)
> I see. That is definitely not the way the Cocoa text system on Mac OS X works,
> so a setting may be required to satisfy different platforms' conventions.

I should have qualified that: that is not the way Mac OS X Leopard (10.5) works, but I was told that it matches Tiger's (10.4) behavior.

I think WebKit's current behavior is the worst possible. A patch that implemented the Firefox/Tiger behavior would be an improvement and would be useful for the Mac port, at least for the Tiger build; a patch that implemented "visual" selection would be better for current Mac OS X versions, and could also be used on Tiger.

[Xiaomei Ji]

Hi Mitz,

Thanks for your comments!

I am not familiar with Mac OS, I just tried a little bit on TextEdit in Mac OS 10.5 and 10.4. I am a bit confused on the behavior. 

Looks like the TextEdit in Mac OS 10.4 matches that of current WebKit (different from FF when the whole text paragraph is in *RTL* level). 

In TextEdit in Mac OS 10.5, if the cursor is between 'x' and 'y' in my example, press shift+left_arrow highlights *both* 'y' and 'z', press again un-highlights both 'y' and 'z' ..... (I am not familiar with Mac, but the highlights on the boundary looks weird to me).  If the cursor is between 'x' and 'y', and press shift+right_arrow, it selects 'y', press again, it un-selects 'y' but selects 'x'.....

But I am not sure whether I am testing on RTL environment (I just set the alignment to be right aligned, and seems the text is in right-to-left direction as well). 

If my observation is wrong and if it is not too much trouble, could you please use a similar HTML document to show the selection of the bidi text in both RTL and LTR text box in the Cocoa text system on Mac OS X 10.5 and 10.4? Appreciate your help.

Thanks,
Xiaomei

P.S. I am trying to figure this out because I think your preferred patch is:
1. having a setting to distinguish platform
2. implement the same behavior as FF in windows
3. implement the same behavior as Cocoa text system in Mac OS 10.5 in Mac.

[Xiaomei Ji]

Created attachment 28747 [details]
patch w/ Layout test

The expected behavior in Mac is unclear to me.
So, I am fixing this in windows only.
It matches the Firefox and Internet Explorer behavior in Windows.

Comment on attachment 28747 [details]
patch w/ Layout test


> [phone]  Xiaomei Ji  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Tests: editing/selection/extend-selection-bidi-start-with-english-in-rtl.html
> +               editing/selection/extend-selection-bidi-start-with-hebrew-in-rtl.html
> +               editing/selection/extend-selection-english-in-rtl.html
> +               editing/selection/extend-selection-hebrew-in-rtl.html
> +
> +        * editing/SelectionController.cpp:
> +        (WebCore::SelectionController::modifyExtendingRightForward):
> +        (WebCore::SelectionController::modifyExtendingForward):
> +        (WebCore::SelectionController::modifyExtendingLeftBackward):
> +        (WebCore::SelectionController::modifyExtendingBackward):
> +        * editing/SelectionController.h:
> +

The change log entry needs to include the bug's title and a link to it. It should also list the changes to each function.

>  VisiblePosition SelectionController::modifyExtendingRightForward(TextGranularity granularity)
>  {
>      VisiblePosition pos(m_sel.extent(), m_sel.affinity());
> +
> +#if PLATFORM(WIN_OS)
> +    WebCore::Node *n = pos.deepEquivalent().node();
> +    if (n) {
> +        WebCore::RenderObject* renderer = n->renderer();
> +        if (renderer) {
> +            if (renderer->style()->direction() == WebCore::RTL) {
> +                return modifyExtendingBackward(granularity, pos);
> +            }
> +        }  
> +    }
> +#endif

modifyExtendingRightForward() is called from both the RIGHT and FORWARD cases of modify(), but this new behavior is the "extend right" behavior, so it is wrong to do it in the FORWARD case. Like I said before, I think you need to define both modifyExtendingRight() and modifyExtendinForward(), implementing the new behavior in the former (and leaving existing behavior in the latter), and then change modify() to call the appropriate function in every case.

I don't think these changes should be limited to WIN_OS or to any specific platform. For now, this is supposed to be an improvement over the existing behavior for all platforms.

You are checking the direction of the node's renderer, but is this what Firefox does? I was under the impression that it uses the direction of the enclosing block. You can see if your patch behaves like Firefox in these cases:
<div>Lorem <span style="direction: rtl">ipsum dolor sit</span> amet</div>
<div>Lorem <span dir="rtl">ipsum dolor sit</span> amet</div>

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 41823)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,32 @@
> [phone]  Xiaomei Ji  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +

This should also reference the bug.

> +        * editing/selection/extend-selection-bidi-start-with-english-in-rtl.html: Added.
> +        * editing/selection/extend-selection-bidi-start-with-hebrew-in-rtl.html: Added.
> +        * editing/selection/extend-selection-english-in-rtl.html: Added.
> +        * editing/selection/extend-selection-hebrew-in-rtl.html: Added.

Can these be combined into a single test?

> +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.checksum: Added.
> +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.png: Added.
> +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.txt: Added.

These should be text-only tests (using layoutTestController.dumpAsText). And since the change should not be platform-specific, there shouldn't be platform-specific results.

> +        extendSelectionForwardByCharacterCommand();

As mentioned above, the behavior of "forward" should not change. You should be testing the behavior of extending "right" and "left". You are not testing word, sentence, line and paragraph granularities. If you change them (like this patch does) then you should test all of them. But are you sure you want to change the behavior for all granularities?

[Xiaomei Ji]

Hi Mitz,

Thanks for your quick review!
Please see my comments inlined.


(In reply to comment #12)
> (From update of attachment 28747 [details] [review])
> 
> > [phone]  Xiaomei Ji  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Tests: editing/selection/extend-selection-bidi-start-with-english-in-rtl.html
> > +               editing/selection/extend-selection-bidi-start-with-hebrew-in-rtl.html
> > +               editing/selection/extend-selection-english-in-rtl.html
> > +               editing/selection/extend-selection-hebrew-in-rtl.html
> > +
> > +        * editing/SelectionController.cpp:
> > +        (WebCore::SelectionController::modifyExtendingRightForward):
> > +        (WebCore::SelectionController::modifyExtendingForward):
> > +        (WebCore::SelectionController::modifyExtendingLeftBackward):
> > +        (WebCore::SelectionController::modifyExtendingBackward):
> > +        * editing/SelectionController.h:
> > +
> 
> The change log entry needs to include the bug's title and a link to it. It
> should also list the changes to each function.

Will do.

> 
> >  VisiblePosition SelectionController::modifyExtendingRightForward(TextGranularity granularity)
> >  {
> >      VisiblePosition pos(m_sel.extent(), m_sel.affinity());
> > +
> > +#if PLATFORM(WIN_OS)
> > +    WebCore::Node *n = pos.deepEquivalent().node();
> > +    if (n) {
> > +        WebCore::RenderObject* renderer = n->renderer();
> > +        if (renderer) {
> > +            if (renderer->style()->direction() == WebCore::RTL) {
> > +                return modifyExtendingBackward(granularity, pos);
> > +            }
> > +        }  
> > +    }
> > +#endif
> 
> modifyExtendingRightForward() is called from both the RIGHT and FORWARD cases
> of modify(), but this new behavior is the "extend right" behavior, so it is
> wrong to do it in the FORWARD case. Like I said before, I think you need to
> define both modifyExtendingRight() and modifyExtendinForward(), implementing
> the new behavior in the former (and leaving existing behavior in the latter),
> and then change modify() to call the appropriate function in every case.

I did not split into modifyExtendingRight() and modifyExtendingForward() because I thought "Right" implies visual.
I will do as what you suggested.

> 
> I don't think these changes should be limited to WIN_OS or to any specific
> platform. For now, this is supposed to be an improvement over the existing
> behavior for all platforms.

Will do that.


> 
> You are checking the direction of the node's renderer, but is this what Firefox
> does? I was under the impression that it uses the direction of the enclosing
> block. You can see if your patch behaves like Firefox in these cases:
> <div>Lorem <span style="direction: rtl">ipsum dolor sit</span> amet</div>
> <div>Lorem <span dir="rtl">ipsum dolor sit</span> amet</div>

You are right.
I will make the change.

> 
> > Index: LayoutTests/ChangeLog
> > ===================================================================
> > --- LayoutTests/ChangeLog	(revision 41823)
> > +++ LayoutTests/ChangeLog	(working copy)
> > @@ -1,3 +1,32 @@
> > [phone]  Xiaomei Ji  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> This should also reference the bug.

Will do.

> 
> > +        * editing/selection/extend-selection-bidi-start-with-english-in-rtl.html: Added.
> > +        * editing/selection/extend-selection-bidi-start-with-hebrew-in-rtl.html: Added.
> > +        * editing/selection/extend-selection-english-in-rtl.html: Added.
> > +        * editing/selection/extend-selection-hebrew-in-rtl.html: Added.
> 
> Can these be combined into a single test?

I will try.

> 
> > +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.checksum: Added.
> > +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.png: Added.
> > +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.txt: Added.
> 
> These should be text-only tests (using layoutTestController.dumpAsText). And
> since the change should not be platform-specific, there shouldn't be
> platform-specific results.

I was following the extend-selection-bidi.html test.
Will try to use layoutTestController.dumpAsText.
Do you mean after which, the png and checksum files are not necessary (if the text files show difference)?

> 
> > +        extendSelectionForwardByCharacterCommand();
> 
> As mentioned above, the behavior of "forward" should not change. You should be
> testing the behavior of extending "right" and "left". You are not testing word,
> sentence, line and paragraph granularities. If you change them (like this patch
> does) then you should test all of them. But are you sure you want to change the
> behavior for all granularities?
> 

You are right.
I think it would be better to restrict such change to only character and word  for now. And I will introduce the 2 new functionalities and test them out on 2 granularities.

Thanks,
Xiaomei

(In reply to comment #13)

> Will try to use layoutTestController.dumpAsText.
> Do you mean after which, the png and checksum files are not necessary (if the
> text files show difference)?

Yes, if you use dumpAsText, PNG and checksum files will not be created and should not be checked in.

[Xiaomei Ji]

Created attachment 28815 [details]
patch w/ Layout test (version 2)

There is one part I am not sure.

Looks like FF does not across block boundary when extending selection.
But currently, Webkit across block boundary while extending selection if the blocks are parent-child. It can extend selection inward or outward.
For example: 
<div> whatever <div dir="rtl"> just a test</div> end</div>
selection can be extended from the block "just a test" to "whatever ... end", and vice versa.

In the current fix, when checking the directionality of enclosing block 
(SelectionController::directionOfEnclosingBlock()), I used the starting point of the selection (m_sel.base().node()), not the ending point (m_sel.extend().node()).

So, for the above, if the selection start at the outer block, the direction is always treated as LTR even when it reaches the inner block. If the selection starts at the inner block, the direction is always treated as RTL even when it reaches the outer block.

Otherwise, the selection will be on and off by one character when it reaches the direction change boundary. Or maybe we can check whether the current extent reaches the enclosing block boundary and stops extending if it does.

Comment on attachment 28815 [details]
patch w/ Layout test (version 2)

> Looks like FF does not across block boundary when extending selection.
> But currently, Webkit across block boundary while extending selection if the
> blocks are parent-child. It can extend selection inward or outward.
> For example: 
> <div> whatever <div dir="rtl"> just a test</div> end</div>
> selection can be extended from the block "just a test" to "whatever ... end",
> and vice versa.
>
> In the current fix, when checking the directionality of enclosing block 
> (SelectionController::directionOfEnclosingBlock()), I used the starting point
> of the selection (m_sel.base().node()), not the ending point
> (m_sel.extend().node()).

What does Firefox do when the selection already crosses a block boundary and the extent is in a block that has different directionality from the base? I think using the directionality of the block enclosing the extent makes more sense, because the extent is what is being modified, although I realize that it may be weird when crossing into a different direction, like you said.

> +TextDirection SelectionController::directionOfEnclosingBlock() {
> +    Node *n = m_sel.base().node();

The * should go next to "Node".

> +    Node* enclosingBlockNode = enclosingBlock(n);
> +    if (enclosingBlockNode) {

You can write this as
       if (Node* enclosingBlockNode = enclosingBlock(n))...

or better yet, you can save some indentation by reversing the condition and turning this into an early return:
       if (!enclosingBlockNode)
           return LTR;

> +        WebCore::RenderObject* renderer = enclosingBlockNode->renderer();

Since this function is in the WebCore namespace, you shouldn't add the "WebCore::" prefix.

> +        if (renderer) {
> +            if (renderer->style()->direction() == WebCore::RTL) {
> +                return RTL;
> +            }

The WebKit coding style is to omit braces around single-line blocks like the above. But the inner if statement is unnecessary. You can simply write
           if (renderer)
               return renderer->style()->direction();

> +        case CharacterGranularity:
> +            if (directionOfEnclosingBlock() == RTL) {
> +                pos = pos.previous(true);
> +            } else {
> +                pos = pos.next(true);
> +            }

Please remove the braces around those single-line blocks.

In existing WebCore code, sometimes the code for the RTL case comes first and sometimes it comes second. In new code, I would rather make the LTR case come first.

> +        case WordGranularity:
> +            if (directionOfEnclosingBlock() == RTL) {
> +                pos = previousWordPosition(pos);
> +            } else {
> +                pos = nextWordPosition(pos);
> +            }

Ditto re: braces and order.

[Xiaomei Ji]

Created attachment 29016 [details]
webkit extend selection when pressing shift+right-arrow twice

[Xiaomei Ji]

Created attachment 29018 [details]
webkit extend selection when press shift+right-arrow twice, then shift+left-arrow

[Xiaomei Ji]

Created attachment 29019 [details]
FireFox extend selection snapshot (comparing with Webkit)

[Xiaomei Ji]

Created attachment 29020 [details]
Webkit extend selection snapshot (comparing with FF)

[Xiaomei Ji]

Hi Mitz,

Thanks for the review!
Please see my comments inlined.


(In reply to comment #16)
> (From update of attachment 28815 [details] [review])
> > Looks like FF does not across block boundary when extending selection.
> > But currently, Webkit across block boundary while extending selection if the
> > blocks are parent-child. It can extend selection inward or outward.
> > For example: 
> > <div> whatever <div dir="rtl"> just a test</div> end</div>
> > selection can be extended from the block "just a test" to "whatever ... end",
> > and vice versa.
> >
> > In the current fix, when checking the directionality of enclosing block 
> > (SelectionController::directionOfEnclosingBlock()), I used the starting point
> > of the selection (m_sel.base().node()), not the ending point
> > (m_sel.extend().node()).
> 
> What does Firefox do when the selection already crosses a block boundary and
> the extent is in a block that has different directionality from the base? I
> think using the directionality of the block enclosing the extent makes more
> sense, because the extent is what is being modified, although I realize that it
> may be weird when crossing into a different direction, like you said.
> 

I've changed the code to use the directionality of the block enclosing the extent.

I finally figured out how FF does selection across block boundary.
I thought it does not. But actually it does, and it does in a way that an implicit/control character is inserted between block boundary. 

I have attached 2 snapshots showing you the difference of FF selection and Webkit selection (after using the direction of the enclosing block contains the extent, not the base in the fix).

For example:
<div contenteditable >Lorem <span  style="direction: rtl">ipsum dolor<div > what ever you want </div> sit</span> amet</div>,

Although not showing visually in selection, but from the cursor movement and selection range, an implicit/control character is inserted between 'r' in "dolor' and 'w' in 'what".
So, if cursor is between 'o' and 'r' in 'dolor', press shift-right-arrow highlights 'r', press it again does not show anything visually, but it actually selects the implicit character. 

At this state, if press shift-left-arrow, it will select 'w'. 
But if press shift-right-arrow, it un-selects the implicit character (which not showing visually), if press shift-left-arrow then, it un-selects 'r'.

So, if the cursor is between 'o' and 'r', press shift-right-arrow once select 'r', press shift-right-arrow in odd number of times, then, press shift-left-arrow will select 'w'. But if press shift-right-arrow in even number of times,
 then, press shift-left-arrow will un-select 'r', because press shift-right-arrow in even number of times actually selects and un-selects the implicit/control character, although the selection does not show any visual effect.

Webkit behaves similar with the only difference that it highlights/un-highlights the implicit character selection.


For FF to selection from "what ever you want" to "sit amet" when the highlight already covers 't' in 'want', you need to press shift-left-arrow twice, then, shift-right-arrow twice to select to 's' in 'sit. I do not now why.

In WebKit, you just need to press shift-left-arrow once to select the implicit character, then, shift-right-arrow once to select 's' in 'sit'.


For another example, 
<div contenteditable >Lorem <div dir="rtl"> what ever you want </div> amet</div>

For FF to continue selection from "Lorem" to 'w', it needs to press shift-right arrow twice, then shift-left-arrow once.
While Webkit needs to press shift-right arrow once, then shift-left-arrow once.

I tried in Mac, seems FF in Mac behaves the same as in Windows.

By comparing Webkit with FF, I do not think WebKit is worse.
The only weird thing in Webkit is the select/un-select of implicit character when continuing press shift-right-arrow after selecting "Loren ipsum dolor",
while FF's selection stays the same.

But in a 2nd thought, Webkit visually shows what happen. It might be even better.

And if I copy what selected from FF or Webkit(with the selection of implicit character) and paste them, the text pasted does not show differences.
 



> > +TextDirection SelectionController::directionOfEnclosingBlock() {
> > +    Node *n = m_sel.base().node();
> 
> The * should go next to "Node".

Done.


> 
> > +    Node* enclosingBlockNode = enclosingBlock(n);
> > +    if (enclosingBlockNode) {
> 
> You can write this as
>        if (Node* enclosingBlockNode = enclosingBlock(n))...
> 
> or better yet, you can save some indentation by reversing the condition and
> turning this into an early return:
>        if (!enclosingBlockNode)
>            return LTR;

Used the 2nd suggestion.


> 
> > +        WebCore::RenderObject* renderer = enclosingBlockNode->renderer();
> 
> Since this function is in the WebCore namespace, you shouldn't add the
> "WebCore::" prefix.

Removed.


> 
> > +        if (renderer) {
> > +            if (renderer->style()->direction() == WebCore::RTL) {
> > +                return RTL;
> > +            }
> 
> The WebKit coding style is to omit braces around single-line blocks like the
> above. But the inner if statement is unnecessary. You can simply write
>            if (renderer)
>                return renderer->style()->direction();

Done.


> 
> > +        case CharacterGranularity:
> > +            if (directionOfEnclosingBlock() == RTL) {
> > +                pos = pos.previous(true);
> > +            } else {
> > +                pos = pos.next(true);
> > +            }
> 
> Please remove the braces around those single-line blocks.

Done.


> 
> In existing WebCore code, sometimes the code for the RTL case comes first and
> sometimes it comes second. In new code, I would rather make the LTR case come
> first.

Done.


> 
> > +        case WordGranularity:
> > +            if (directionOfEnclosingBlock() == RTL) {
> > +                pos = previousWordPosition(pos);
> > +            } else {
> > +                pos = nextWordPosition(pos);
> > +            }
> 
> Ditto re: braces and order.
> 

Done.

Will attach the patch for review soon.

Thanks,
Xiaomei

[Xiaomei Ji]

Created attachment 29031 [details]
patch w/ Layout test (version 3)

Comment on attachment 29031 [details]
patch w/ Layout test (version 3)

Great! r=me

Please don't make include this change when landing:

> Property changes on: LayoutTests/editing/selection/extend-selection.html
> ___________________________________________________________________
> Name: svn:executable
>    + *
>

[Xiaomei Ji]

Hi Mitz,

Thanks for your quick review!
And thanks very much for all your help on my questions, emails, comments, feedbacks, code reviews, etc.

Xiaomei


(In reply to comment #23)
> (From update of attachment 29031 [details] [review])
> Great! r=me
> 
> Please don't make include this change when landing:
> 
> > Property changes on: LayoutTests/editing/selection/extend-selection.html
> > ___________________________________________________________________
> > Name: svn:executable
> >    + *
> > 
> 

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42054