Fix: Cursor missing in omnibox after entering a alphabet in NTP 'Search box'

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 45abeecdbe3bf527902236758f8841cefb57b56f..cd7a2e5de5da752da3f87a3ea3f46fa879375b7b 100644
--- a/ui/views/controls/textfield/textfield_unittest.cc
+++ b/ui/views/controls/textfield/textfield_unittest.cc
@@ -253,6 +253,7 @@ void MockInputMethod::ClearComposition() {
 }
 
 // A Textfield wrapper to intercept OnKey[Pressed|Released]() ressults.

[msw, 2017/03/09 21:05:45]

optional nit: please fix the spelling of 'results' if you don't mind.

[yiyix, 2017/03/10 20:19:29]

Sorry, I missed this.

+// After user event, the textfield sets cursor_enabled in RenderText to true.

[msw, 2017/03/09 21:05:46]

Move this comment to OnAfterUserAction and explain why this is done, rather than
reiterating what the code does; use vertical bars, like |cursor_enabled_|, when
referencing a variable identifier.

[yiyix, 2017/03/10 20:19:29]

As explained below, I can remove this comment now.

 class TestTextfield : public views::Textfield {
  public:
   TestTextfield()
@@ -396,6 +397,7 @@ class TextfieldTest : public ViewsTestBase, public TextfieldController {
   }
 
   void OnAfterUserAction(Textfield* sender) override {
+    textfield_->SetCursorEnabled(true);

[msw, 2017/03/09 21:05:46]

What is the motivation for this change? I'm not sure why we want this.

[yiyix, 2017/03/10 20:19:29]

I don't need this line now. I added this line to test the order between
OnafterUserAction and other commends.

     ++on_after_user_action_;
   }
 
@@ -3112,7 +3114,7 @@ TEST_F(TextfieldTest, AccessiblePasswordTest) {
   EXPECT_TRUE(node_data_protected.HasStateFlag(ui::AX_STATE_PROTECTED));
 }
 
-// Test if the cursor visibility is controlled by |cursor_enabled_| in
+// Verifies if the cursor visibility is controlled by |cursor_enabled_| in
 // RenderText.
 TEST_F(TextfieldTest, CursorVisibility) {
   InitTextfield();
@@ -3129,4 +3131,32 @@ TEST_F(TextfieldTest, CursorVisibility) {
   EXPECT_TRUE(test_api_->IsCursorVisible());
 }
 
+// Verifies if the cursor visibility is changed to true from false after insert

[msw, 2017/03/09 21:05:46]

nit: 'inserting'

[yiyix, 2017/03/10 20:19:29]

Done.

+// charactor in textfield.

[msw, 2017/03/09 21:05:46]

nit: 'a character in the textfield'.

[yiyix, 2017/03/10 20:19:29]

Done.

+TEST_F(TextfieldTest, CursorVisibilityChangeAfterInserting) {
+  InitTextfield();
+
+  test_api_->GetRenderText()->SetCursorEnabled(false);

[msw, 2017/03/09 21:05:46]

Ditto: Should this call textfield_->SetCursorEnabled(false) and then be able to
check EXPECT_FALSE(test_api_->IsCursorVisible()) before calling OnBlur/OnFocus?

+  textfield_->OnBlur();
+  textfield_->OnFocus();
+  EXPECT_FALSE(test_api_->IsCursorVisible());
+  SendKeyEvent('a');

[msw, 2017/03/09 21:05:46]

Why should inserting a character make the cursor visible if it was explicitly
set to be not visible? I'm guessing this happens because of the new
|textfield_->SetCursorEnabled(true)| call in OnAfterUserAction above, which is
added to match the omnibox's quirky behavior. Can we instead just test that
OmniboxViewViews does the right thing, instead of tweaking a test impl class to
mimic its behavior?

[yiyix, 2017/03/10 20:19:29]

I will remove this test case and add some new test case in omnibox_view_views.

+  EXPECT_TRUE(test_api_->IsCursorVisible());
+}
+
+// Verifies if the cursor visibility is changed to true from false after paste

[msw, 2017/03/09 21:05:46]

nit: 'pasting'

[yiyix, 2017/03/10 20:19:30]

Done.

+// some text to the textfield.
+TEST_F(TextfieldTest, CursorVisibilityChangeAfterPaste) {
+  InitTextfield();
+
+  test_api_->GetRenderText()->SetCursorEnabled(false);

[msw, 2017/03/09 21:05:46]

ditto

+  textfield_->OnBlur();
+  textfield_->OnFocus();
+  SetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE, "abc");
+  textfield_->SetText(base::string16());

[msw, 2017/03/09 21:05:45]

Is this needed?

+  SendKeyEvent(ui::VKEY_V, false, true);

[msw, 2017/03/09 21:05:46]

Ditto, can we test the omnibox instead?

[yiyix, 2017/03/10 20:19:30]

I added a new test case in omnibox. 

+  EXPECT_STR_EQ("abc", textfield_->text());
+  EXPECT_TRUE(test_api_->IsCursorVisible());
+}
+
 }  // namespace views