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

ui/gfx/text_constants.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef UI_GFX_TEXT_CONSTANTS_H_
 #define UI_GFX_TEXT_CONSTANTS_H_
 
 namespace gfx {
 
 // TODO(msw): Distinguish between logical character stops and glyph stops?
 // TODO(msw): Merge with base::i18n::BreakIterator::BreakType.
 enum BreakType {
   CHARACTER_BREAK = 0, // Stop cursor movement on neighboring characters.
   WORD_BREAK,          // Stop cursor movement on nearest word boundaries.
   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:
 enum WordWrapBehavior {
   IGNORE_LONG_WORDS,   // Overflowing word text is left on that line.
   TRUNCATE_LONG_WORDS, // Overflowing word text is truncated.
   ELIDE_LONG_WORDS,    // Overflowing word text is elided at the ellipsis.
   WRAP_LONG_WORDS,     // Overflowing word text is wrapped over multiple lines.
 };
 
@@ skipping 49 matching lines @@
   ELIDE_HEAD,   // Add an ellipsis at the start of the string.
   ELIDE_MIDDLE, // Add an ellipsis in the middle of the string.
   ELIDE_TAIL,   // Add an ellipsis at the end of the string.
   ELIDE_EMAIL,  // Add ellipses to username and domain substrings.
   FADE_TAIL,    // Fade the string's end opposite of its horizontal alignment.
 };
 
 }  // namespace gfx
 
 #endif  // UI_GFX_TEXT_CONSTANTS_H_