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

ui/views/controls/textfield/textfield.cc

Index: ui/views/controls/textfield/textfield.cc
diff --git a/ui/views/controls/textfield/textfield.cc b/ui/views/controls/textfield/textfield.cc
index be796b27549a59f49444ee6261134903192ac70b..be38f7e0d6669c3382e62159eeb7d6ff3fb30f4b 100644
--- a/ui/views/controls/textfield/textfield.cc
+++ b/ui/views/controls/textfield/textfield.cc
@@ -1467,6 +1467,8 @@ bool Textfield::IsTextEditCommandEnabled(ui::TextEditCommand command) const {
     case ui::TextEditCommand::MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_TO_END_OF_PARAGRAPH:
     case ui::TextEditCommand::MOVE_TO_END_OF_PARAGRAPH_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_WORD_BACKWARD:
     case ui::TextEditCommand::MOVE_WORD_BACKWARD_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_WORD_FORWARD:
@@ -1503,6 +1505,13 @@ bool Textfield::IsTextEditCommandEnabled(ui::TextEditCommand command) const {
     case ui::TextEditCommand::MOVE_PAGE_UP_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_UP:
     case ui::TextEditCommand::MOVE_UP_AND_MODIFY_SELECTION:
+// On Mac, the textfield should respond to Up/Down arrows keys and
+// PageUp/PageDown.
+#if defined(OS_MACOSX)
+      return true;
+#else
+      return false;
+#endif
     case ui::TextEditCommand::INSERT_TEXT:
     case ui::TextEditCommand::SET_MARK:
     case ui::TextEditCommand::UNSELECT:
@@ -1630,6 +1639,8 @@ void Textfield::ExecuteTextEditCommand(ui::TextEditCommand command) {
     case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_DOCUMENT:
     case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE:
     case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_PARAGRAPH:
+    case ui::TextEditCommand::MOVE_UP:
+    case ui::TextEditCommand::MOVE_PAGE_UP:
       model_->MoveCursor(gfx::LINE_BREAK, begin, false);
       break;
     case ui::TextEditCommand::
@@ -1637,41 +1648,65 @@ void Textfield::ExecuteTextEditCommand(ui::TextEditCommand command) {
     case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::
         MOVE_TO_BEGINNING_OF_PARAGRAPH_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true,
+                         PlatformStyle::kLineSelectionBehavior);

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

I'd like to avoid the behavior enum if possible. Could we encapsulate this
behavior within RenderText, using an OS_MAC preprocessor block (or some
ui/gfx-level platform-specific flags) to simply extend the selection for
|break_type == LINE_BREAK && select|? From my quick testing, it really seems
like Mac does the magic selection-extending for all move-by-line/para/page/doc
and select commands; it seems like all the (gfx::LINE_BREAK, ...,  true) calls
here should be supplying PlatformStyle::kLineSelectionBehavior.

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

For CHARACTER_BREAK, the enum is not really relevant. For WORD_BREAK, its
possible to derive the behavior based on the platform. But for LINE_BREAK, Mac
does exhibit all the 3 kinds of behavior-

Eg consider

-SELECTION_EXTEND
  one tw|o three 

  Command+Shift+Right
  one tw|o three|

  Command+Shift+Left
  |one two three|

-SELECTION_START_NEW
  one tw|o three

  Shift+Down
  one tw|o three|

  Shift+Up
  |one tw|o three

-SELECTION_CARET
  one tw|o three

  Alt+Shift+Up
  one tw|o three|

  Alt+Shift+Down
  one tw|o three

  Alt+Shift+Down
  |one tw|o three


Now for the LINE_BREAK case, you are correct that most of the Mac commands use
the SELECTION_EXTEND behavior. 

SELECTION_START_NEW behavior is used by moveUp/DownAndModifySelection and 
movePageUp/DownAndModifySelection.

SELECTION_CARET is used by moveParagraphForward/BackwardAndModifySelection.

A case can be made that these commands are not that common and thus not worth
the added complexity, in which case we can make the SELECTION_EXTEND behavior
the default on Mac for LINE_BREAK commands. Over to Trent, if we really need the
different behaviors on Mac. The commands affected would be-
moveUp/DownAndModifySelection
movePageUp/DownAndModifySelection
moveParagraphForward/BackwardAndModifySelection

and the behavior will be incorrect only for the case when the selection
direction is reversed.

+      break;
+    case ui::TextEditCommand::MOVE_PAGE_UP_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_UP_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true,
+                         gfx::SelectionReversedBehavior::SELECTION_START_NEW);

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

nit: `::SelectionReversedBehavior` not needed, more below

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

Shouldn't these also extend the selection on mac? (afaict from blink's behavior)

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

Nope, try yourself with Shift+Up/Down. And the Blink behavior is also correct.
Here is a page which lists most of the default system key bindings on Mac-
https://www.hcs.harvard.edu/~jrus/site/system-bindings.html

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

Done.

       break;
     case ui::TextEditCommand::MOVE_TO_END_OF_DOCUMENT:
     case ui::TextEditCommand::MOVE_TO_END_OF_LINE:
     case ui::TextEditCommand::MOVE_TO_END_OF_PARAGRAPH:
+    case ui::TextEditCommand::MOVE_DOWN:
+    case ui::TextEditCommand::MOVE_PAGE_DOWN:
       model_->MoveCursor(gfx::LINE_BREAK, end, false);
       break;
     case ui::TextEditCommand::MOVE_TO_END_OF_DOCUMENT_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_TO_END_OF_PARAGRAPH_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::LINE_BREAK, end, true);
+      model_->MoveCursor(gfx::LINE_BREAK, end, true,
+                         PlatformStyle::kLineSelectionBehavior);

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

Can you define the platform-specific constants here? This extra layer of
indirection (with a platform-specific value) makes the code even more difficult
to understand, especially when it's defined elsewhere.

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

Will do!

+      break;
+    case ui::TextEditCommand::MOVE_PAGE_DOWN_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_DOWN_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, end, true,
+                         gfx::SelectionReversedBehavior::SELECTION_START_NEW);

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

Shouldn't these also extend the selection on mac? (afaict from blink's behavior)

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

Nope, try yourself with Shift+Up/Down.

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

Wow, Mac's behavior is so odd; we definitely need tests.

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

Done.

+      break;
+    case ui::TextEditCommand::MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true, gfx::SELECTION_CARET);

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

ditto: Why don't these (para fwd/back) extend the selection on mac? Why would
they collapse the selection into a cursor instead of extending or simply
modifying the existing selection? If you want a cursor instead, why not just
pass false for |select| here on Mac?

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

This is how Cocoa does it. Try with Alt+Shift+Up/Down.
This is only relevant if the selection direction is reversed. 

+      break;
+    case ui::TextEditCommand::MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, end, true, gfx::SELECTION_CARET);
       break;
     case ui::TextEditCommand::MOVE_WORD_BACKWARD:
       model_->MoveCursor(gfx::WORD_BREAK, begin, false);
       break;
     case ui::TextEditCommand::MOVE_WORD_BACKWARD_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::WORD_BREAK, begin, true);
+      model_->MoveCursor(gfx::WORD_BREAK, begin, true,
+                         PlatformStyle::kWordSelectionBehavior);
       break;
     case ui::TextEditCommand::MOVE_WORD_FORWARD:
       model_->MoveCursor(gfx::WORD_BREAK, end, false);
       break;
     case ui::TextEditCommand::MOVE_WORD_FORWARD_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::WORD_BREAK, end, true);
+      model_->MoveCursor(gfx::WORD_BREAK, end, true,
+                         PlatformStyle::kWordSelectionBehavior);
       break;
     case ui::TextEditCommand::MOVE_WORD_LEFT:
       model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, false);
       break;
     case ui::TextEditCommand::MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, true);
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, true,
+                         PlatformStyle::kWordSelectionBehavior);
       break;
     case ui::TextEditCommand::MOVE_WORD_RIGHT:
       model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, false);
       break;
     case ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, true);
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, true,
+                         PlatformStyle::kWordSelectionBehavior);
       break;
     case ui::TextEditCommand::UNDO:
       text_changed = cursor_changed = model_->Undo();
@@ -1697,14 +1732,6 @@ void Textfield::ExecuteTextEditCommand(ui::TextEditCommand command) {
     case ui::TextEditCommand::YANK:
       text_changed = cursor_changed = model_->Yank();
       break;
-    case ui::TextEditCommand::MOVE_DOWN:
-    case ui::TextEditCommand::MOVE_DOWN_AND_MODIFY_SELECTION:
-    case ui::TextEditCommand::MOVE_PAGE_DOWN:
-    case ui::TextEditCommand::MOVE_PAGE_DOWN_AND_MODIFY_SELECTION:
-    case ui::TextEditCommand::MOVE_PAGE_UP:
-    case ui::TextEditCommand::MOVE_PAGE_UP_AND_MODIFY_SELECTION:
-    case ui::TextEditCommand::MOVE_UP:
-    case ui::TextEditCommand::MOVE_UP_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::INSERT_TEXT:
     case ui::TextEditCommand::SET_MARK:
     case ui::TextEditCommand::UNSELECT: