Bug 49511 – RTL: Caret goes to the opposite direction when pressing an arrow key after selection is made

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

[Yair Yogev]

Created attachment 73852 [details]
testcase

1. Open the attached testcase.
2. Select a word (using the mouse or the keyboard, it also doesn't matter if the selection starts from the beginning of the word or the end of it)
3. Press the Right(or Left) Arrow key - Caret will appear in the Left(or Right) part of the previous selection.

Same thing goes for multi lined selections.

How other browsers are doing?
IE, Opera: OK, after deselection, caret moves to the requested position.
Firefox: not relevant because its post-selection-arrow-key-press(-...) behavior is different - after the deselection, it moves the caret one char to the Left/Right from the End-Of-Selection position.

[Yair Yogev]

tracked as http://crbug.com/46518 in the chromium bug tracker

[Levi Weintraub]

Created attachment 74704 [details]
Patch

[Levi Weintraub]

Created attachment 75041 [details]
Patch

[Levi Weintraub]

Now with a better test case that covers all the cases.

[Ryosuke Niwa]

Comment on attachment 75041 [details]
Patch

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

> WebCore/editing/SelectionController.cpp:285
> -            m_selection.setBase(start);
> -            m_selection.setExtent(end);
> +            if (directionOfEnclosingBlock() == LTR) {
> +                m_selection.setBase(start);
> +                m_selection.setExtent(end);
> +            } else {
> +                m_selection.setBase(end);
> +                m_selection.setExtent(start);
> +            }

I don't think we should be doing this for DirectionRight.  New code seems to make sense only for DirectionForward.

Dan & Xiaomei, could you comment on this?

> WebCore/editing/SelectionController.cpp:295
> -            m_selection.setBase(end);
> -            m_selection.setExtent(start);
> +            if (directionOfEnclosingBlock() == LTR) {
> +                m_selection.setBase(end);
> +                m_selection.setExtent(start);
> +            } else {
> +                m_selection.setBase(start);
> +                m_selection.setExtent(end);
> +            }

Ditto.

> WebCore/editing/SelectionController.cpp:425
> -            pos = VisiblePosition(m_selection.end(), m_selection.affinity());
> +            if (directionOfEnclosingBlock() == LTR)
> +                pos = VisiblePosition(m_selection.end(), m_selection.affinity());
> +            else
> +                pos = VisiblePosition(m_selection.start(), m_selection.affinity());

You could write this as:
pos = VisiblePosition(directionOfEnclosingBlock() == LTR ? m_selection.end() : m_selection.start(), m_selection.affinity());
But maybe this is less readable.

> LayoutTests/editing/selection/rtl-move-selection-right-left.html:71
> +    testSelectionChange(setSelection, "move", "right", 0, undefined);
> +    testSelectionChange(setSelection, "move", "left", undefined, 0);

Why don't we test end & start offsets for these two tests?  I think we should.

[Xiaomei Ji]

Comment on attachment 75041 [details]
Patch

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

>> WebCore/editing/SelectionController.cpp:285
>> +            }
> 
> I don't think we should be doing this for DirectionRight.  New code seems to make sense only for DirectionForward.
> 
> Dan & Xiaomei, could you comment on this?

Is it the other way around? the new change should only apply for DirectionRight/Left.

> WebCore/editing/SelectionController.cpp:426
>          else

you probably forgot to enclose them into "{......}"

> WebCore/editing/SelectionController.cpp:579
> +                pos = VisiblePosition(m_selection.end(), m_selection.affinity());

enclose in { }.

[Levi Weintraub]

Created attachment 78065 [details]
Patch

[Levi Weintraub]

(In reply to comment #7)
> Created an attachment (id=78065) [details]
> Patch

> > LayoutTests/editing/selection/rtl-move-selection-right-left.html:71
> > +    testSelectionChange(setSelection, "move", "right", 0, undefined);
> > +    testSelectionChange(setSelection, "move", "left", undefined, 0);
> Why don't we test end & start offsets for these two tests?  I think we should.

You're right. Fixed.

> (From update of attachment 75041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75041&action=review
> 
> >> WebCore/editing/SelectionController.cpp:285
> >> +            }
> > 
> > I don't think we should be doing this for DirectionRight.  New code seems to make sense only for DirectionForward.
> > 
> > Dan & Xiaomei, could you comment on this?
> 
> Is it the other way around? the new change should only apply for DirectionRight/Left.

Xiaomei is right, it's really just for Right/Left. I've fixed willBeModified to only consider LTR for Left/Right.

> > WebCore/editing/SelectionController.cpp:426
> >          else
> 
> you probably forgot to enclose them into "{......}"
> 
> > WebCore/editing/SelectionController.cpp:579
> > +                pos = VisiblePosition(m_selection.end(), m_selection.affinity());
> 
> enclose in { }.

Looks like a bug in WebKit Style as well! Fixed :)

[Ryosuke Niwa]

Comment on attachment 78065 [details]
Patch

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

r=me but cq- because I requested minor fixes.

> WebCore/editing/SelectionController.cpp:350
>          switch (direction) {
>          case DirectionRight:
> +            if (directionOfEnclosingBlock() == LTR) {
> +                m_selection.setBase(start);
> +                m_selection.setExtent(end);
> +            } else {
> +                m_selection.setBase(end);
> +                m_selection.setExtent(start);
> +            }
> +            break;
>          case DirectionForward:
>              m_selection.setBase(start);
>              m_selection.setExtent(end);
>              break;
> +            if (directionOfEnclosingBlock() == LTR) {
> +                m_selection.setBase(end);
> +                m_selection.setExtent(start);
> +            } else {
> +                m_selection.setBase(start);
> +                m_selection.setExtent(end);
> +            }
> +            break;

I'm not particularly happy about these duplicates calls to setBase & setExtent.  Can we define a boolean variable like baseIsFirst and make this switch statement update it?  e.g. move setBase/setExtent out of switch and put that into a separate if-else clauses.

If you do that, then you should probably be modifying the code for the case where m_isDirectional is true as well.

> LayoutTests/editing/selection/rtl-move-selection-right-left.html:28
> +    eventSender.mouseMoveTo(testContainer.offsetLeft + 2, y);

I'm worried that +2 will have an undesired effects on some platforms and the test fails.  You may want to consider adding some padding to this testContainer and drag from a padding on the left to a padding on the right.

> LayoutTests/editing/selection/rtl-move-selection-right-left.html:75
> +        window.layoutTestController.dumpAsText();
> +        testSelectionChange(dragSelection, "extend", "right", -1, 0);
> +        testSelectionChange(dragSelection, "extend", "left", 1, 0);

As we talked in person, you probably want to comment on what's the expected behavior here.  Something in the line of "When text is selection by mouse, there's no directionality and extending to the right should move the left end to the right and extending to the left should move the right end to the left on RTL text when the entire text is selected".

[Yair Yogev]

i may be understanding things wrong since i don't know the code very well... but it sound like you are also changing the behavior if extending the selection and not just the case of deselecting by pressing left/right?

if that is so i am a bit worried since extending RTL selections already works as is should AFAIK (at least as expected under windows, xji fixed it)

umm... am i wrong?

[Levi Weintraub]

(In reply to comment #10)
> i may be understanding things wrong since i don't know the code very well... but it sound like you are also changing the behavior if extending the selection and not just the case of deselecting by pressing left/right?
> 
> if that is so i am a bit worried since extending RTL selections already works as is should AFAIK (at least as expected under windows, xji fixed it)
> 
> umm... am i wrong?

This won't affect Windows at all. This change for extending selections only affects non-directional selections, which is to match NSTextView's behavior on Macs. Check out the comment here: http://trac.webkit.org/browser/trunk/Source/WebCore/editing/SelectionController.cpp#L743

[Levi Weintraub]

Committed r76312: <http://trac.webkit.org/changeset/76312>

[WebKit Review Bot]

http://trac.webkit.org/changeset/76312 might have broken Qt Linux Release minimal