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

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/memory/ptr_util.h"
@@ skipping 751 matching lines @@
     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());
   ExecuteTextEditCommand(GetCommandForKeyEvent(event));
+  UpdateAfterChange(false, true);

[sadrul, 2017/03/08 02:02:52]

We shouldn't need this here? ExecuteTextEditCommand() should (and looks like
does) make the necessary call to UpdateAfterChange() when appropriate. Are there
cases when that does not happen?

[yiyix, 2017/03/08 03:32:41]

ExecuteTextEditCommand first calls OmniboxViewViews::ExecuteTextEditCommand,
command "move up", "move down" and "paste" is implemented in the
OmniboxViewViews.  OnAfterPossibleChange is called after paste. So the cursor's
visibility may change after calling paste and it is not updated in
UpdateAfterChange. To catch this particular path, i added this line.

[sadrul, 2017/03/08 13:45:36]

OK. So it looks like there *are* some cases where UpdateAfterChange() does not
get called. That's a bit unfortunate. For the call to UpdateAfterChange() here,
why do we use false for text-change and true for cursor-change?

I think we can make this code a little cleaner. We can probably split
ExecuteTextEditCommand() into two parts: non-virtual ExecuteTextEditCommand(),
which calls a virtual ExecuteTextEditCommandImpl() (or a better name).
OmniboxViewViews overrides the latter. For Textfield,
ExecuteTextEditCommandImpl() includes only the big switch-case block, and the
rest remains in ExecuteTextEditCommand(). That way, ExecuteTextEditCommand()
always calls UpdateAfterChange() when necessary, with the right parameters.
OmniboxViewViews::ExecuteTextEditCommand() can also be cleaned a little (e.g. we
could remove 829:836 in that file.)

   return true;
 }
 
 bool Textfield::CanHandleAccelerators() const {
   return GetRenderText()->focused() && View::CanHandleAccelerators();
 }
 
 void Textfield::AboutToRequestFocusFromTabTraversal(bool reverse) {
   SelectAll(false);
 }
@@ skipping 94 matching lines @@
     // Adjust the drop destination if it is on or after the current selection.
     size_t pos = drop_destination_model.caret_pos();
     pos -= render_text->selection().Intersect(gfx::Range(0, pos)).length();
     model_->DeleteSelectionAndInsertTextAt(new_text, pos);
   } else {
     model_->MoveCursorTo(drop_destination_model);
     // Drop always inserts text even if the textfield is not in insert mode.
     model_->InsertText(new_text);
   }
   skip_input_method_cancel_composition_ = false;
+  OnAfterUserAction();
   UpdateAfterChange(true, true);
-  OnAfterUserAction();
   return move ? ui::DragDropTypes::DRAG_MOVE : ui::DragDropTypes::DRAG_COPY;
 }
 
 void Textfield::OnDragDone() {
   initiating_drag_ = false;
   drop_cursor_visible_ = false;
 }
 
 void Textfield::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->role = ui::AX_ROLE_TEXT_FIELD;
@@ skipping 358 matching lines @@
 // Textfield, ui::TextInputClient overrides:
 
 void Textfield::SetCompositionText(const ui::CompositionText& composition) {
   if (GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE)
     return;
 
   OnBeforeUserAction();
   skip_input_method_cancel_composition_ = true;
   model_->SetCompositionText(composition);
   skip_input_method_cancel_composition_ = false;
+  OnAfterUserAction();
   UpdateAfterChange(true, true);
-  OnAfterUserAction();
 }
 
 void Textfield::ConfirmCompositionText() {
   if (!model_->HasCompositionText())
     return;
 
   OnBeforeUserAction();
   skip_input_method_cancel_composition_ = true;
   model_->ConfirmCompositionText();
   skip_input_method_cancel_composition_ = false;
+  OnAfterUserAction();
   UpdateAfterChange(true, true);
-  OnAfterUserAction();
 }
 
 void Textfield::ClearCompositionText() {
   if (!model_->HasCompositionText())
     return;
 
   OnBeforeUserAction();
   skip_input_method_cancel_composition_ = true;
   model_->CancelCompositionText();
   skip_input_method_cancel_composition_ = false;
+  OnAfterUserAction();
   UpdateAfterChange(true, true);
-  OnAfterUserAction();
 }
 
 void Textfield::InsertText(const base::string16& new_text) {
   // TODO(suzhe): Filter invalid characters.
   if (GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE || new_text.empty())
     return;
 
   OnBeforeUserAction();
   skip_input_method_cancel_composition_ = true;
   model_->InsertText(new_text);
   skip_input_method_cancel_composition_ = false;
+  OnAfterUserAction();
   UpdateAfterChange(true, true);
-  OnAfterUserAction();
 }
 
 void Textfield::InsertChar(const ui::KeyEvent& event) {
   if (read_only()) {
     OnEditFailed();
     return;
   }
 
   // Filter out all control characters, including tab and new line characters,
   // and all characters with Alt modifier (and Search on ChromeOS, Ctrl on
@@ skipping 261 matching lines @@
 
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, protected:
 
 void Textfield::DoInsertChar(base::char16 ch) {
   OnBeforeUserAction();
   skip_input_method_cancel_composition_ = true;
   model_->InsertChar(ch);
   skip_input_method_cancel_composition_ = false;
 
+  OnAfterUserAction();
   UpdateAfterChange(true, true);
-  OnAfterUserAction();
 }
 
 gfx::RenderText* Textfield::GetRenderText() const {
   return model_->render_text();
 }
 
 gfx::Point Textfield::GetLastClickLocation() const {
   return selection_controller_.last_click_location();
 }
 
@@ skipping 190 matching lines @@
     case ui::TextEditCommand::SET_MARK:
     case ui::TextEditCommand::UNSELECT:
     case ui::TextEditCommand::INVALID_COMMAND:
       NOTREACHED();
       break;
   }
 
   cursor_changed |= GetSelectionModel() != selection_model;
   if (cursor_changed && HasSelection())
     UpdateSelectionClipboard();
+  OnAfterUserAction();
   UpdateAfterChange(text_changed, cursor_changed);
-  OnAfterUserAction();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, private:
 
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, SelectionControllerDelegate overrides:
 
 gfx::RenderText* Textfield::GetRenderTextForSelectionController() {
   return GetRenderText();
@@ skipping 276 matching lines @@
   cursor_blink_timer_.Stop();
 }
 
 void Textfield::OnCursorBlinkTimerFired() {
   DCHECK(ShouldBlinkCursor());
   cursor_view_.SetVisible(!cursor_view_.visible());
   UpdateCursorView();
 }
 
 }  // namespace views