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

ui/views/controls/textfield/textfield.cc

 // 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.
 
 #include "ui/views/controls/textfield/textfield.h"
 
 #include <string>
 #include <utility>
 
 #include "base/trace_event/trace_event.h"
@@ skipping 1449 matching lines @@
     case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_PARAGRAPH:
     case ui::TextEditCommand::
         MOVE_TO_BEGINNING_OF_PARAGRAPH_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_TO_END_OF_DOCUMENT:
     case ui::TextEditCommand::MOVE_TO_END_OF_DOCUMENT_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_TO_END_OF_LINE:
     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:
     case ui::TextEditCommand::MOVE_WORD_FORWARD_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_WORD_LEFT:
     case ui::TextEditCommand::MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
     case ui::TextEditCommand::MOVE_WORD_RIGHT:
     case ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
       return true;
     case ui::TextEditCommand::UNDO:
@@ skipping 16 matching lines @@
     case ui::TextEditCommand::YANK:
       return editable;
     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:
+// 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:
     case ui::TextEditCommand::INVALID_COMMAND:
       return false;
   }
   NOTREACHED();
   return false;
 }
 
@@ skipping 107 matching lines @@
       break;
     case ui::TextEditCommand::MOVE_RIGHT:
       model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, false);
       break;
     case ui::TextEditCommand::MOVE_RIGHT_AND_MODIFY_SELECTION:
       model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, true);
       break;
     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::
         MOVE_TO_BEGINNING_OF_DOCUMENT_AND_MODIFY_SELECTION:
     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();
       break;
     case ui::TextEditCommand::REDO:
       text_changed = cursor_changed = model_->Redo();
       break;
     case ui::TextEditCommand::CUT:
       text_changed = cursor_changed = Cut();
       break;
     case ui::TextEditCommand::COPY:
       Copy();
       break;
     case ui::TextEditCommand::PASTE:
       text_changed = cursor_changed = Paste();
       break;
     case ui::TextEditCommand::SELECT_ALL:
       SelectAll(false);
       break;
     case ui::TextEditCommand::TRANSPOSE:
       text_changed = cursor_changed = model_->Transpose();
       break;
     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:
     case ui::TextEditCommand::INVALID_COMMAND:
       NOTREACHED();
       break;
   }
 
   cursor_changed |= GetSelectionModel() != selection_model;
   if (cursor_changed)
@@ skipping 258 matching lines @@
     RequestFocus();
   model_->MoveCursorTo(mouse);
   if (!selection_clipboard_text.empty()) {
     model_->InsertText(selection_clipboard_text);
     UpdateAfterChange(true, true);
   }
   OnAfterUserAction();
 }
 
 }  // namespace views