Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

ui/views/controls/textfield/textfield_unittest.cc

 // Copyright 2014 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 <stddef.h>
 #include <stdint.h>
 
 #include <set>
@@ skipping 849 matching lines @@
   SendWordEvent(ui::VKEY_RIGHT, shift);
   shift = true;
   SendWordEvent(ui::VKEY_DELETE, shift);
 #if defined(OS_LINUX)
   EXPECT_STR_EQ(" two", textfield_->text());
 #else
   EXPECT_STR_EQ(" two four", textfield_->text());
 #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);
   EXPECT_EQ(ui::TEXT_INPUT_TYPE_PASSWORD, textfield_->GetTextInputType());
   EXPECT_TRUE(textfield_->enabled());
   EXPECT_TRUE(textfield_->IsFocusable());
 
   last_contents_.clear();
   textfield_->SetText(ASCIIToUTF16("password"));
   // Ensure text() and the callback returns the actual text instead of "*".
@@ skipping 919 matching lines @@
   EXPECT_EQ(ui::CLIPBOARD_TYPE_COPY_PASTE, GetAndResetCopiedToClipboard());
 
   // Ensure [Shift]+[Delete] cuts.
   textfield_->SetText(ASCIIToUTF16("123"));
   textfield_->SelectAll(false);
   SendAlternateCut();
   EXPECT_STR_EQ("123", GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE));
   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);
   EXPECT_TRUE(textfield_->IsCommandIdEnabled(IDS_APP_COPY));
   textfield_->ExecuteCommand(IDS_APP_COPY, 0);
   EXPECT_STR_EQ("789", GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE));
   EXPECT_EQ(ui::CLIPBOARD_TYPE_COPY_PASTE, GetAndResetCopiedToClipboard());
 
   // Ensure [Ctrl]+[c] copies and [Ctrl]+[Alt][c] does nothing.
   textfield_->SetText(ASCIIToUTF16("012"));
@@ skipping 785 matching lines @@
 
   textfield_->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD);
   ui::AXViewState state_protected;
   textfield_->GetAccessibleState(&state_protected);
   EXPECT_EQ(ui::AX_ROLE_TEXT_FIELD, state_protected.role);
   EXPECT_EQ(ASCIIToUTF16("********"), state_protected.value);
   EXPECT_TRUE(state_protected.HasStateFlag(ui::AX_STATE_PROTECTED));
 }
 
 }  // namespace views