[webkit-reviews] review granted: [Bug 49511] RTL: Caret goes to the opposite direction when pressing an arrow key after selection is made : [Attachment 78065] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Levi Weintraub <leviw at google.com>'s
request for review:
Bug 49511: RTL: Caret goes to the opposite direction when pressing an arrow key
after selection is made
https://bugs.webkit.org/show_bug.cgi?id=49511

Attachment 78065: Patch
https://bugs.webkit.org/attachment.cgi?id=78065&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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".