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

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 234 matching lines @@
 
 bool MockInputMethod::HasComposition() {
   return composition_.text.length() || result_text_.length();
 }
 
 void MockInputMethod::ClearComposition() {
   composition_.Clear();
   result_text_.clear();
 }
 
 // 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()
      : Textfield(),
        key_handled_(false),
        key_received_(false),
        weak_ptr_factory_(this) {}
 
   // ui::TextInputClient overrides:
   void InsertChar(const ui::KeyEvent& e) override {
@@ skipping 123 matching lines @@
     // paste action did not change the content. So |new_contents| may match
     // |last_contents_|. For more info, see http://crbug.com/79002
     last_contents_ = new_contents;
   }
 
   void OnBeforeUserAction(Textfield* sender) override {
     ++on_before_user_action_;
   }
 
   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_;
   }
 
   void OnAfterCutOrCopy(ui::ClipboardType clipboard_type) override {
     copied_to_clipboard_ = clipboard_type;
   }
 
   void InitTextfield() {
     InitTextfields(1);
   }
@@ skipping 2696 matching lines @@
   textfield_->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD);
   ui::AXNodeData node_data_protected;
   node_data_protected.state = 0;
   textfield_->GetAccessibleNodeData(&node_data_protected);
   EXPECT_EQ(ui::AX_ROLE_TEXT_FIELD, node_data_protected.role);
   EXPECT_EQ(ASCIIToUTF16("********"),
             node_data_protected.GetString16Attribute(ui::AX_ATTR_VALUE));
   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();
   gfx::RenderText* render_text = test_api_->GetRenderText();
 
   render_text->SetCursorEnabled(false);

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

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

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

Done.

   textfield_->OnBlur();
   textfield_->OnFocus();
   EXPECT_FALSE(test_api_->IsCursorVisible());
 
   render_text->SetCursorEnabled(true);

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

Similarly, should this call textfield_->SetCursorEnabled(true) and then be able
to check EXPECT_TRUE(test_api_->IsCursorVisible()) before calling
OnBlur/OnFocus?

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

In the approach I had before, the the cursor visibility is changed after
OnBlur/OnFocus. In this new approach, the cursor visibility is updated in
setcursorEnabled. I will update the test case to match the current approach. 

   textfield_->OnBlur();
   textfield_->OnFocus();
   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