Make Views Textfield key handling execute commands.

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 f3f58593762e27544c5edf5a8c10c871a0fb5ae6..d1a357da60e063d85a22e9dde6548db3ab12d447 100644
--- a/ui/views/controls/textfield/textfield.cc
+++ b/ui/views/controls/textfield/textfield.cc
@@ -424,124 +424,115 @@ bool Textfield::OnKeyPressed(const ui::KeyEvent& event) {
   if (handled)
     return true;
 
-  // TODO(oshima): Refactor and consolidate with ExecuteCommand.
   if (event.type() == ui::ET_KEY_PRESSED) {
     ui::KeyboardCode key_code = event.key_code();
     if (key_code == ui::VKEY_TAB || event.IsUnicodeKeyCode())
       return false;
 
-    gfx::RenderText* render_text = GetRenderText();
-    const bool editable = !read_only();
-    const bool readable = text_input_type_ != ui::TEXT_INPUT_TYPE_PASSWORD;
     const bool shift = event.IsShiftDown();
     const bool control = event.IsControlDown();
     const bool alt = event.IsAltDown() || event.IsAltGrDown();
-    bool text_changed = false;
-    bool cursor_changed = false;
 
+    // TODO(msw): Make a helper CommandForKeyEvent or similar. 
     OnBeforeUserAction();
     switch (key_code) {
       case ui::VKEY_Z:
-        if (control && !shift && !alt && editable)
-          cursor_changed = text_changed = model_->Undo();
-        else if (control && shift && !alt && editable)
-          cursor_changed = text_changed = model_->Redo();
+        if (control && !shift && !alt)
+          ExecuteCommand(IDS_APP_UNDO);
+        else if (control && shift && !alt)
+          ExecuteCommand(IDS_APP_REDO);
         break;
       case ui::VKEY_Y:
-        if (control && !alt && editable)
-          cursor_changed = text_changed = model_->Redo();
+        if (control && !alt)
+          ExecuteCommand(IDS_APP_REDO);
         break;
       case ui::VKEY_A:
-        if (control && !alt) {
-          model_->SelectAll(false);
-          UpdateSelectionClipboard();
-          cursor_changed = true;
-        }
+        if (control && !alt)
+          ExecuteCommand(IDS_APP_SELECT_ALL);
         break;
       case ui::VKEY_X:
-        if (control && !alt && editable && readable)
-          cursor_changed = text_changed = Cut();
+        if (control && !alt)
+          ExecuteCommand(IDS_APP_CUT);
         break;
       case ui::VKEY_C:
-        if (control && !alt && readable)
-          Copy();
+        if (control && !alt)
+          ExecuteCommand(IDS_APP_COPY);
         break;
       case ui::VKEY_V:
-        if (control && !alt && editable)
-          cursor_changed = text_changed = Paste();
+        if (control && !alt)
+          ExecuteCommand(IDS_APP_PASTE);
         break;
       case ui::VKEY_RIGHT:
-      case ui::VKEY_LEFT: {
-        // We should ignore the alt-left/right keys because alt key doesn't make
-        // any special effects for them and they can be shortcut keys such like
-        // forward/back of the browser history.
+        // Ignore alt+right, which may be a browser navigation shortcut.
         if (alt)
           break;
-        const gfx::Range selection_range = render_text->selection();
-        model_->MoveCursor(
-            control ? gfx::WORD_BREAK : gfx::CHARACTER_BREAK,
-            (key_code == ui::VKEY_RIGHT) ? gfx::CURSOR_RIGHT : gfx::CURSOR_LEFT,
-            shift);
-        UpdateSelectionClipboard();
-        cursor_changed = render_text->selection() != selection_range;
+        if (shift) {
+          ExecuteCommand(control ? IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION :
+                                   IDS_MOVE_RIGHT_AND_MODIFY_SELECTION);
+        } else {
+          ExecuteCommand(control ? IDS_MOVE_WORD_RIGHT : IDS_MOVE_RIGHT);
+        }
+        break;
+      case ui::VKEY_LEFT:
+        // Ignore alt+left, which may be a browser navigation shortcut.
+        if (alt)
+          break;
+        if (shift) {
+          ExecuteCommand(control ? IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION :
+                                   IDS_MOVE_LEFT_AND_MODIFY_SELECTION);
+        } else {
+          ExecuteCommand(control ? IDS_MOVE_WORD_LEFT : IDS_MOVE_LEFT);
+        }
         break;
-      }
-      case ui::VKEY_END:
       case ui::VKEY_HOME:
-        if ((key_code == ui::VKEY_HOME) ==
-            (render_text->GetTextDirection() == base::i18n::RIGHT_TO_LEFT))
-          model_->MoveCursor(gfx::LINE_BREAK, gfx::CURSOR_RIGHT, shift);
-        else
-          model_->MoveCursor(gfx::LINE_BREAK, gfx::CURSOR_LEFT, shift);
-        UpdateSelectionClipboard();
-        cursor_changed = true;
+        ExecuteCommand(shift ? IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION :
+                               IDS_MOVE_TO_BEGINNING_OF_LINE);
+        break;
+      case ui::VKEY_END:
+        ExecuteCommand(shift? IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION :
+                              IDS_MOVE_TO_END_OF_LINE);
         break;
       case ui::VKEY_BACK:
+        if (control && !model_->HasSelection()) {
+          if (shift) {
+#if defined(OS_LINUX)
+            // Only erase by line break on Linux and ChromeOS.
+            ExecuteCommand(IDS_DELETE_TO_BEGINNING_OF_LINE);
+#endif
+          } else {
+            ExecuteCommand(IDS_DELETE_WORD_BACKWARD);
+          }
+        } else {
+          ExecuteCommand(IDS_DELETE_BACKWARD);
+        }
+        break;
       case ui::VKEY_DELETE:
-        if (!editable)
-          break;
-        if (!model_->HasSelection()) {
-          gfx::VisualCursorDirection direction = (key_code == ui::VKEY_DELETE) ?
-              gfx::CURSOR_RIGHT : gfx::CURSOR_LEFT;
-          if (shift && control) {
-            // If shift and control are pressed, erase up to the next line break
-            // on Linux and ChromeOS. Otherwise, do nothing.
+        if (control && !model_->HasSelection()) {
+          if (shift) {
 #if defined(OS_LINUX)
-            model_->MoveCursor(gfx::LINE_BREAK, direction, true);
-#else
-            break;
+            // Only erase by line break on Linux and ChromeOS.
+            ExecuteCommand(IDS_DELETE_TO_END_OF_LINE);
 #endif
-          } else if (control) {
-            // If only control is pressed, then erase the previous/next word.
-            model_->MoveCursor(gfx::WORD_BREAK, direction, true);
+          } else {
+            ExecuteCommand(IDS_DELETE_WORD_FORWARD);
           }
+        } else if (shift && model_->HasSelection()) {
+          ExecuteCommand(IDS_APP_CUT);
+        } else {
+          ExecuteCommand(IDS_DELETE_FORWARD);
         }
-        if (key_code == ui::VKEY_BACK)
-          model_->Backspace();
-        else if (shift && model_->HasSelection() && readable)
-          Cut();
-        else
-          model_->Delete();
-
-        // Consume backspace and delete keys even if the edit did nothing. This
-        // prevents potential unintended side-effects of further event handling.
-        text_changed = true;
         break;
       case ui::VKEY_INSERT:
-        if (control && !shift && readable)
-          Copy();
-        else if (shift && !control && editable)
-          cursor_changed = text_changed = Paste();
+        if (control && !shift)
+          ExecuteCommand(IDS_APP_COPY);
+        else if (shift && !control)
+          ExecuteCommand(IDS_APP_PASTE);
         break;
       default:
-        break;
+        return false;
     }
 
-    // We must have input method in order to support text input.
-    DCHECK(GetInputMethod());
-    UpdateAfterChange(text_changed, cursor_changed);
-    OnAfterUserAction();
-    return (text_changed || cursor_changed);
+    return true;
   }
   return false;
 }
@@ -960,6 +951,8 @@ bool Textfield::IsCommandIdEnabled(int command_id) const {
   switch (command_id) {
     case IDS_APP_UNDO:
       return editable && model_->CanUndo();
+    case IDS_APP_REDO:
+      return editable && model_->CanRedo();
     case IDS_APP_CUT:
       return editable && readable && model_->HasSelection();
     case IDS_APP_COPY:
@@ -972,6 +965,26 @@ bool Textfield::IsCommandIdEnabled(int command_id) const {
       return editable && model_->HasSelection();
     case IDS_APP_SELECT_ALL:
       return !text().empty();
+    case IDS_DELETE_FORWARD:
+    case IDS_DELETE_BACKWARD:
+    case IDS_DELETE_TO_BEGINNING_OF_LINE:
+    case IDS_DELETE_TO_END_OF_LINE:
+    case IDS_DELETE_WORD_BACKWARD:
+    case IDS_DELETE_WORD_FORWARD:
+      return editable;
+    case IDS_MOVE_LEFT:
+    case IDS_MOVE_LEFT_AND_MODIFY_SELECTION:

[Elliot Glaysher, 2014/03/28 22:47:49]

So, if we're going to have versions for the events that are
_AND_MODIFY_SELECTION, I'll need to modify my approach to not just setting a
bool for the "modify selection" part.

(This list will also need unification with the uncomitted
ui/wm/public/text_edit_command_x11.cc.)

Also, at what level is it safe to use these IDS_* commands? Shouldn't these all
be IDC? (I thought IDS was for "S"tring while IDC was for "C"ommand.) Would I be
able to use the IDC commands up one level at ui/wm/?

[msw, 2014/03/28 22:55:29]

The separate _AND_MODIFY_SELECTION commands come from following the conventions
of third_party/WebKit/Source/core/editing/EditorCommand.cpp.
That consolidation can come later, views and web content textfield event/command
handling won't overlap at all, at least for now.
IDC_* is defined at the chrome level, whereas these are needed at the ui level.
Using IDS_* instead has been the convention for IDS_APP_CUT/COPY/PASTE, etc. and
seems appropriate as a medium-term solution; these same IDS_* should be
available to ui/wm/.

+    case IDS_MOVE_RIGHT:
+    case IDS_MOVE_RIGHT_AND_MODIFY_SELECTION:
+    case IDS_MOVE_WORD_LEFT:
+    case IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
+    case IDS_MOVE_WORD_RIGHT:
+    case IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
+    case IDS_MOVE_TO_BEGINNING_OF_LINE:
+    case IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
+    case IDS_MOVE_TO_END_OF_LINE:
+    case IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
+      return true;
     default:
       return false;
   }
@@ -988,31 +1001,102 @@ void Textfield::ExecuteCommand(int command_id, int event_flags) {
     return;
 
   bool text_changed = false;
+  bool cursor_changed = false;
+  bool rtl = GetTextDirection() == base::i18n::RIGHT_TO_LEFT;
+  gfx::VisualCursorDirection begin = rtl ? gfx::CURSOR_RIGHT : gfx::CURSOR_LEFT;
+  gfx::VisualCursorDirection end = rtl ? gfx::CURSOR_LEFT : gfx::CURSOR_RIGHT;
+  gfx::Range selection_range = GetSelectedRange();
+
   OnBeforeUserAction();
   switch (command_id) {
     case IDS_APP_UNDO:
-      text_changed = model_->Undo();
+      text_changed = cursor_changed = model_->Undo();
+      break;
+    case IDS_APP_REDO:
+      text_changed = cursor_changed = model_->Redo();
       break;
     case IDS_APP_CUT:
-      text_changed = Cut();
+      text_changed = cursor_changed = Cut();
       break;
     case IDS_APP_COPY:
       Copy();
       break;
     case IDS_APP_PASTE:
-      text_changed = Paste();
+      text_changed = cursor_changed = Paste();
       break;
     case IDS_APP_DELETE:
-      text_changed = model_->Delete();
+      text_changed = cursor_changed = model_->Delete();
       break;
     case IDS_APP_SELECT_ALL:
       SelectAll(false);
       break;
+    case IDS_DELETE_BACKWARD:
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case IDS_DELETE_FORWARD:
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case IDS_DELETE_TO_END_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, end, true);
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case IDS_DELETE_TO_BEGINNING_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case IDS_DELETE_WORD_BACKWARD:
+      model_->MoveCursor(gfx::WORD_BREAK, begin, true);
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case IDS_DELETE_WORD_FORWARD:
+      model_->MoveCursor(gfx::WORD_BREAK, end, true);
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case IDS_MOVE_LEFT:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, false);
+      break;
+    case IDS_MOVE_LEFT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, true);
+      break;
+    case IDS_MOVE_RIGHT:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, false);
+      break;
+    case IDS_MOVE_RIGHT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, true);
+      break;
+    case IDS_MOVE_WORD_LEFT:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, false);
+      break;
+    case IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, true);
+      break;
+    case IDS_MOVE_WORD_RIGHT:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, false);
+      break;
+    case IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, true);
+      break;
+    case IDS_MOVE_TO_BEGINNING_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, false);
+      break;
+    case IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
+      break;
+    case IDS_MOVE_TO_END_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, end, false);
+      break;
+    case IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, end, true);
+      break;
     default:
       NOTREACHED();
       break;
   }
-  UpdateAfterChange(text_changed, text_changed);
+
+  cursor_changed |= GetSelectedRange() != selection_range;
+  if (cursor_changed)
+    UpdateSelectionClipboard();
+  UpdateAfterChange(text_changed, cursor_changed);
   OnAfterUserAction();
 }