MacViews: Fix behavior of move and select commands when selection direction changes.

ui/gfx/text_constants.h

Index: ui/gfx/text_constants.h
diff --git a/ui/gfx/text_constants.h b/ui/gfx/text_constants.h
index b09081dd8a0b51fd955f319e665a2ced94634503..51e72652e4d190c40609e4bbfda9fcb610e9e1a4 100644
--- a/ui/gfx/text_constants.h
+++ b/ui/gfx/text_constants.h
@@ -15,6 +15,34 @@ enum BreakType {
   LINE_BREAK,          // Stop cursor movement on line ends as shown on screen.
 };
 
+// Specifies how a move and select command behaves if the selection direction
+// is reversed. For example consider the state "ab|cd|e", i.e. cd is selected.
+// Assume the selection direction is from left to right. If we move to the
+// beginning of the line while modifying the selection, the resultant state is:
+// "|ab|cde" for SELECTION_START_NEW, selection direction from right to left.
+// "|abcd|e" for SELECTION_EXTEND, selection direction from right to left.
+// "ab|cde" for SELECTION_CARET.
+enum SelectionReversedBehavior {

[tapted, 2016/08/11 03:44:07]

What about calling this just `SelectionBehavior` and removing the separate `bool
select` argument. bool arguments that need explainers in the function comments
are usually discouraged (and mixing bool arguments with default arguments can
lead to a lot of confusion since so many things convert to bool -- e.g. you'd
get no compile error passing SELECTION_START_NEW as the `bool select` argument,
but it will convert to `false` and do the wrong thing).

We'd just need a new enum for SELECTION_NONE (or CLEAR/CANCEL/COLLAPSE) - 

[karandeepb, 2016/08/16 10:24:52]

Nice suggestion. Done.

+  // Existing selection is deselected and a new selection is started in the
+  // opposite direction. Default. For example, this is the behavior of
+  // textfields on Mac for the command moveUpAndModifySelection (Shift + Up).
+  // Also this is the default behavior for text editing commands on Linux and
+  // Windows.
+  SELECTION_START_NEW,
+
+  // The existing selection is extended in the opposite direction. For example,
+  // this is the behavior for textfields on Mac for the command
+  // moveToLeftEndOfLineAndModifySelection (Command + Shift + Left).
+  SELECTION_EXTEND,
+
+  // The existing selection reduces to a caret. For example, this is the
+  // behavior for textfields on Mac for the command
+  // moveWordLeftAndModifySelection (Alt + Shift + Left).
+  SELECTION_CARET,

[msw, 2016/08/11 23:46:37]

Why do we need explicit behavior for this? The existing behavior should work as:
"a |bc d" -> ALT+SHIFT+RIGHT -> "a |bc| d" -> ALT+SHIFT+LEFT -> "a |bc d"
The selection should already collapse to a cursor when the range is empty.

Do you want the selection to collapse in this case? That seems very wrong:
"a |bc de| f" -> ALT+SHIFT+LEFT -> "a |bc de f"

If that's actually what you'd want, why not supply false for |modify_selection|?
Sorry if I'm not understanding this correctly!

[tapted, 2016/08/12 00:12:00]

Yeah this tripped me up too :). Karan can probably explain better, but I think
the problem is that SELECTION_CARET only comes into play when the selection gets
reversed.

e.g. 

one t|wo three| four  // selected left-to-right

Alt+Shift+Left
one t|wo |three four  // selected left-to-right - still selected so
|modify_selection| can't be false

Alt+Shift+Left
one t|wo three four  // would reverse: SELECTION_CARET

Alt+Shift+Left
one |t|wo three four  // selected right-to-left

[karandeepb, 2016/08/12 01:26:31]

No we don't. As Trent said, this only comes into play, when the selection
direction is reversed. In this case, the result currently is "a |bc |de f", so
the selection direction is the same and the enum value won't come into play.
It's ok. I should probably work on making the code clearer.
Here's the need for this. Consider-
abc|def

Alt+Shift+Right
abc|def|

Alt+Shift+Left
|abc|def  -> Current behavior
abc|def -> What we want on Mac.

Also see the example given by Trent.

[msw, 2016/08/12 21:58:30]

Ah, we need tests for this like TextfieldTest.ControlAndSelectTest.

[karandeepb, 2016/08/16 10:24:52]

Added tests. Was planning to add them after the initial review.

+
+  SELECTION_DEFAULT = SELECTION_START_NEW,
+};
+
 // Specifies the word wrapping behavior when a word would exceed the available
 // display width. All words that are too wide will be put on a new line, and
 // then: