Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

ui/views/controls/textfield/textfield_unittest.cc

Index: ui/views/controls/textfield/textfield_unittest.cc
diff --git a/ui/views/controls/textfield/textfield_unittest.cc b/ui/views/controls/textfield/textfield_unittest.cc
index 8e14fa064c08840e994791923b0ba10c72409426..8a5591afc304b717c543e522099527432dcc1837 100644
--- a/ui/views/controls/textfield/textfield_unittest.cc
+++ b/ui/views/controls/textfield/textfield_unittest.cc
@@ -867,6 +867,57 @@ TEST_F(TextfieldTest, InsertionDeletionTest) {
 #endif
 }
 
+// Test that deletion operations behave correctly in case there is an active
+// selection.
+TEST_F(TextfieldTest, DeletionWithSelection) {

[karandeepb, 2016/04/27 05:27:56]

Ideally, this should fail on master for Linux. However, for tests on Linux, we
don't use a TextEditKeyBindingsDelegateAuraLinux, hence GetCommandForKeyEvent is
not bypassed in tests.

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

Since all cases have the same behavior, you could rewrite this test more
succinctly, perhaps something like:

  struct {
    ui::KeyboardCode key;
    bool shift;
  } cases[] = {
    { ui::VKEY_BACK, false }, { ui::VKEY_DELETE, false },
    { ui::VKEY_BACK, true }, { ui::VKEY_DELETE, true },
  };

  InitTextfield();
  // [Delete] and [Backspace] should delete a selection, regardless of [Shift].
  for (size_t i = 0; i < arraysize(cases); ++i) {
    SCOPED_TRACE(base::StringPrintf("Testing cases[%" PRIuS "]", i));
    textfield_->SetText(ASCIIToUTF16("one two three"));
    textfield_->SelectRange(gfx::Range(2, 6));
    SendWordEvent(cases[i].key, cases[i].shift);
    EXPECT_STR_EQ("ono three", textfield_->text());
    EXPECT_EQ(gfx::Range(2), textfield_->GetSelectedRange());
  }

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

Add a comment to that effect at a condition that should fail in the test.

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

Done.

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

I wasn't probably clear enough. I meant that it should have failed on the
current master. With the fix, it won't fail, even if we use a
TextEditKeyBindingsDelegateAuraLinux. Hence don't think a comment is needed.

+  InitTextfield();
+  textfield_->SetText(ASCIIToUTF16("one two three"));
+
+  // Make selection as - on|e tw|o three
+  textfield_->SelectRange(gfx::Range(2, 6));
+  // Send keyboard event for delete previous word.
+  bool shift = false;
+  SendWordEvent(ui::VKEY_BACK, shift);
+  // Verify state is on|o three.
+  EXPECT_STR_EQ("ono three", textfield_->text());
+  EXPECT_EQ(gfx::Range(2), textfield_->GetSelectedRange());
+
+  // Make selection as on|o th|ree.
+  textfield_->SelectRange(gfx::Range(2, 6));
+  // Delete to a line break on Linux and ChromeOS, to a word break on Windows

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

nit: Shouldn't this comment say "[Shift]+[Backspace] should only delete the
active selection, when one exists." or similar?

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

Yeah, it should. Though it's actually Ctrl + Shift + Backspace. 

+  // and Mac. However since there is an active selection, only the selection
+  // should be deleted.
+  shift = true;
+  SendWordEvent(ui::VKEY_BACK, shift);
+
+  // Verify state is on|ree.
+  EXPECT_STR_EQ("onree", textfield_->text());
+  EXPECT_EQ(gfx::Range(2), textfield_->GetSelectedRange());
+
+  textfield_->SetText(ASCIIToUTF16("one two three"));
+
+  // Make selection as - on|e tw|o three
+  textfield_->SelectRange(gfx::Range(2, 6));
+  // Send keyboard event for delete next word.
+  shift = false;
+  SendWordEvent(ui::VKEY_DELETE, shift);
+  // Verify state is on|o three.
+  EXPECT_STR_EQ("ono three", textfield_->text());
+  EXPECT_EQ(gfx::Range(2), textfield_->GetSelectedRange());
+
+  // Make selection as on|o th|ree.
+  textfield_->SelectRange(gfx::Range(2, 6));
+  // Delete to a line break on Linux and ChromeOS, to a word break on Windows

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

ditto nit: Shouldn't this comment say "[Shift]+[Delete] should only delete the
active selection, when one exists." or similar?

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

Done.

+  // and Mac. However since there is an active selection, only the selection
+  // should be deleted.
+  shift = true;
+  SendWordEvent(ui::VKEY_DELETE, shift);
+
+  // Verify state is on|ree.
+  EXPECT_STR_EQ("onree", textfield_->text());
+  EXPECT_EQ(gfx::Range(2), textfield_->GetSelectedRange());
+}
+
 TEST_F(TextfieldTest, PasswordTest) {
   InitTextfield();
   textfield_->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD);
@@ -1806,6 +1857,17 @@ TEST_F(TextfieldTest, CutCopyPaste) {
   EXPECT_STR_EQ("", textfield_->text());
   EXPECT_EQ(ui::CLIPBOARD_TYPE_COPY_PASTE, GetAndResetCopiedToClipboard());
 
+  // Reset clipboard text.
+  SetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE, "");
+
+  // Ensure [Shift]+[Delete] is a no-op in case there is no selection.
+  textfield_->SetText(ASCIIToUTF16("123"));
+  textfield_->SelectRange(gfx::Range(0));
+  SendAlternateCut();
+  EXPECT_STR_EQ("", GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE));
+  EXPECT_STR_EQ("123", textfield_->text());
+  EXPECT_EQ(ui::CLIPBOARD_TYPE_LAST, GetAndResetCopiedToClipboard());
+
   // Ensure IDS_APP_COPY copies.
   textfield_->SetText(ASCIIToUTF16("789"));
   textfield_->SelectAll(false);