[Webkit-unassigned] [Bug 49511] RTL: Caret goes to the opposite direction when pressing an arrow key after selection is made

[bugzilla-daemon at webkit.org]

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


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78065|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org>  2011-01-10 14:18:49 PST ---
(From update of attachment 78065)
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".

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