Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

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 65 matching lines @@
   switch (ui::ScopedAnimationDurationScaleMode::duration_scale_mode()) {
       case ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION: return 100;
       case ui::ScopedAnimationDurationScaleMode::FAST_DURATION:   return 25;
       case ui::ScopedAnimationDurationScaleMode::SLOW_DURATION:   return 400;
       case ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION:   return 1;
       case ui::ScopedAnimationDurationScaleMode::ZERO_DURATION:   return 0;
     }
   return 100;
 }
 
-// Get the default command for a given key |event| and selection state.
-int GetCommandForKeyEvent(const ui::KeyEvent& event, bool has_selection) {
+// Get the default command for a given key |event|.
+int GetCommandForKeyEvent(const ui::KeyEvent& event) {
   if (event.type() != ui::ET_KEY_PRESSED || event.IsUnicodeKeyCode())
     return kNoCommand;
 
   const bool shift = event.IsShiftDown();
   const bool control = event.IsControlDown();
   const bool alt = event.IsAltDown() || event.IsAltGrDown();
   switch (event.key_code()) {
     case ui::VKEY_Z:
       if (control && !shift && !alt)
         return IDS_APP_UNDO;
@@ skipping 24 matching lines @@
         return control ? IDS_MOVE_WORD_LEFT : IDS_MOVE_LEFT;
       return control ? IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION :
                         IDS_MOVE_LEFT_AND_MODIFY_SELECTION;
     case ui::VKEY_HOME:
       return shift ? IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION :
                       IDS_MOVE_TO_BEGINNING_OF_LINE;
     case ui::VKEY_END:
       return shift ? IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION :
                       IDS_MOVE_TO_END_OF_LINE;
     case ui::VKEY_BACK:
-      if (!control || has_selection)
+      if (!control)
         return IDS_DELETE_BACKWARD;
 #if defined(OS_LINUX)
       // Only erase by line break on Linux and ChromeOS.
       if (shift)
         return IDS_DELETE_TO_BEGINNING_OF_LINE;
 #endif
       return IDS_DELETE_WORD_BACKWARD;
     case ui::VKEY_DELETE:

[msw, 2016/04/28 18:22:02]

Consider rewriting this case a little bit for simplicity:
#if defined(OS_LINUX)
      // Only erase by line break on Linux and ChromeOS.
      if (shift && control)
        return IDS_DELETE_TO_END_OF_LINE;
#endif
      if (control)
        return IDS_DELETE_WORD_FORWARD;
      return shift ? IDS_APP_CUT : IDS_DELETE_FORWARD;	

[karandeepb, 2016/04/29 01:40:59]

Done.

-      if (!control || has_selection)
-        return (shift && has_selection) ? IDS_APP_CUT : IDS_DELETE_FORWARD;
+      if (shift) {
 #if defined(OS_LINUX)
-      // Only erase by line break on Linux and ChromeOS.
-      if (shift)
-        return IDS_DELETE_TO_END_OF_LINE;
+        // Only erase by line break on Linux and ChromeOS.
+        if (control)
+          return IDS_DELETE_TO_END_OF_LINE;
 #endif
-      return IDS_DELETE_WORD_FORWARD;
+        return control ? IDS_DELETE_WORD_FORWARD : IDS_APP_CUT;

[msw, 2016/04/28 18:22:02]

It's weird to return IDS_APP_CUT without knowing if there's a selection, but I
guess we paste without knowing if the clipboard has text content... Would it be
preferable for IDS_APP_CUT to fall back to IDS_DELETE_FORWARD when there is no
selection, instead of a no-op? In abstract, that may not seem right, but it's
reasonable if you consider "we used to just delete-forward on SHIFT+DELETE with
no selection, now it does nothing." I'm not sure it's a big deal... On Windows,
Notepad does a backspace on SHIFT+DELETE, and Edge does nothing. +CC Peter in
case he has an opinion here.

[Peter Kasting, 2016/04/28 19:09:18]

Shift-delete with no selection should no-op.  This matches Wordpad and Edge, and
when Wordpad and Notepad disagree I normally go with Wordpad.

[msw, 2016/04/28 19:17:43]

Okay, but note that Shift-Backspace acts as simply Backspace in Chrome, Wordpad,
Notepad, and Edge; it might be a little odd that Shift would make Delete no-op,
but do nothing to modify Backspace. I'm not too strongly opinionated here, but
I'd like it to be sensible/consistent.

[Peter Kasting, 2016/04/28 19:21:48]

Shift-delete is normally "cut" whereas shift-backspace isn't normally anything. 
Having shift-delete no-op with no selection is consistent with it meaning "cut",
which is to me a more important consistency than being consistent with the way
that shift does not modify backspace.

[msw, 2016/04/28 19:23:41]

Okay, sounds good to me!

+      }
+      return control ? IDS_DELETE_WORD_FORWARD : IDS_DELETE_FORWARD;
     case ui::VKEY_INSERT:
       if (control && !shift)
         return IDS_APP_COPY;
       return (shift && !control) ? IDS_APP_PASTE : kNoCommand;
     default:
       return kNoCommand;
   }
 }
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
@@ skipping 564 matching lines @@
       if (IsCommandIdEnabled(command)) {
         ExecuteCommand(command);
         handled = true;
       }
     }
     return handled;
   }
 #endif
 
   if (edit_command == kNoCommand)
-    edit_command = GetCommandForKeyEvent(event, HasSelection());
+    edit_command = GetCommandForKeyEvent(event);
 
   if (!handled && IsCommandIdEnabled(edit_command)) {
     ExecuteCommand(edit_command);
     handled = true;
   }
   return handled;
 }
 
 void Textfield::OnGestureEvent(ui::GestureEvent* event) {
   switch (event->type()) {
@@ skipping 83 matching lines @@
       break;
     default:
       return;
   }
 }
 
 // This function is called by BrowserView to execute clipboard commands.
 bool Textfield::AcceleratorPressed(const ui::Accelerator& accelerator) {
   ui::KeyEvent event(accelerator.type(), accelerator.key_code(),
                      accelerator.modifiers());
-  ExecuteCommand(GetCommandForKeyEvent(event, HasSelection()));
+  ExecuteCommand(GetCommandForKeyEvent(event));
   return true;
 }
 
 bool Textfield::CanHandleAccelerators() const {
   return GetRenderText()->focused() && View::CanHandleAccelerators();
 }
 
 void Textfield::AboutToRequestFocusFromTabTraversal(bool reverse) {
   SelectAll(false);
 }
@@ skipping 448 matching lines @@
       *accelerator = ui::Accelerator(ui::VKEY_A, ui::EF_CONTROL_DOWN);
       return true;
 
     default:
       return false;
   }
 }
 
 void Textfield::ExecuteCommand(int command_id, int event_flags) {
   DestroyTouchSelection();
+
+  if (HasSelection()) {

[msw, 2016/04/28 18:22:02]

nit: add an explanatory comment that Linux and Mac (or maybe 'some codepaths')
may bypass GetCommandForKeyEvent, so any selection-dependent modifications of
the command should happen here.

[karandeepb, 2016/04/29 01:40:59]

Done.

+    switch (command_id) {
+      case IDS_DELETE_WORD_BACKWARD:
+      case IDS_DELETE_TO_BEGINNING_OF_LINE:
+        command_id = IDS_DELETE_BACKWARD;

[msw, 2016/04/28 18:22:02]

It seems a little odd that this function would supplant a given command. I
wonder if the 'correct' fix is to actually support knowledge of the selection
state in the places where Mac and Linux determine the commands to send (ie.
Gtk2UI::MatchEvent/Gtk2KeyBindingsHandler::BuildGdkEventKeyFromXEvent, but I'm
not sure where this is on Mac...). That said, changing the linux path looks far
less straightforward, so this is probably a reasonable fix.

[karandeepb, 2016/04/29 01:40:59]

Yeah this should be easy to do on Mac at least. But I like this fix more.
Similar to how an OS translates keystrokes into commands, GetCommandForKeyEvent
should only be responsible for translating a given KeyEvent to the appropriate
command w/o considering whether there is an active selection.

So in case of [Ctrl]+[Delete] with an active selection, the actual command we
get should still be IDS_DELETE_WORD_FORWARD. It just so happens, that on an
active selection, we want IDS_DELETE_WORD_FORWARD to behave like
IDS_DELETE_FORWARD.

+        break;
+      case IDS_DELETE_WORD_FORWARD:
+      case IDS_DELETE_TO_END_OF_LINE:
+        command_id = IDS_DELETE_FORWARD;
+        break;
+    }
+  }
+
   if (!IsCommandIdEnabled(command_id))
     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::SelectionModel selection_model = GetSelectionModel();
 
@@ skipping 597 matching lines @@
     RequestFocus();
   model_->MoveCursorTo(mouse);
   if (!selection_clipboard_text.empty()) {
     model_->InsertText(selection_clipboard_text);
     UpdateAfterChange(true, true);
   }
   OnAfterUserAction();
 }
 
 }  // namespace views