Bug 49511

Summary: RTL: Caret goes to the opposite direction when pressing an arrow key after selection is made
Product: WebKit Reporter: Yair Yogev <progame+wk>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, eric, jshin, kenneth, leviw, mitz, playmobil, rniwa, tonikitoo, webkit.review.bot, xji
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 52883    
Bug Blocks:    
Attachments:
Description Flags
testcase
none
Patch
none
Patch
none
Patch rniwa: review+, rniwa: commit-queue-

Description Yair Yogev 2010-11-14 10:44:08 PST
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.
Comment 1 Yair Yogev 2010-11-14 10:47:43 PST
tracked as http://crbug.com/46518 in the chromium bug tracker
Comment 2 Levi Weintraub 2010-11-23 15:42:43 PST
Created attachment 74704 [details]
Patch
Comment 3 Levi Weintraub 2010-11-29 11:00:48 PST
Created attachment 75041 [details]
Patch
Comment 4 Levi Weintraub 2010-11-29 11:01:40 PST
Now with a better test case that covers all the cases.
Comment 5 Ryosuke Niwa 2010-12-08 16:28:48 PST
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.
Comment 6 Xiaomei Ji 2010-12-09 11:40:32 PST
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 { }.
Comment 7 Levi Weintraub 2011-01-05 16:57:34 PST
Created attachment 78065 [details]
Patch
Comment 8 Levi Weintraub 2011-01-05 16:58:20 PST
(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 :)
Comment 9 Ryosuke Niwa 2011-01-10 14:18:49 PST
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".
Comment 10 Yair Yogev 2011-01-10 16:56:10 PST
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?
Comment 11 Levi Weintraub 2011-01-20 12:03:23 PST
(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
Comment 12 Levi Weintraub 2011-01-20 17:21:27 PST
Committed r76312: <http://trac.webkit.org/changeset/76312>
Comment 13 WebKit Review Bot 2011-01-20 17:25:34 PST
http://trac.webkit.org/changeset/76312 might have broken Qt Linux Release minimal