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 865 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 107 matching lines @@
 }
 
 bool Textfield::DeleteRange(const gfx::Range& range) {
   if (!ImeEditingAllowed() || range.is_empty())
     return false;
 
   OnBeforeUserAction();
   model_->SelectRange(range);
   if (model_->HasSelection()) {
     model_->DeleteSelection();
     UpdateAfterChange(true, true);

[msw, 2017/03/09 17:07:45]

I really don't understand this change well enough... Why doesn't this CL flip
all calls to UpdateAfterChange and OnAfterUserAction? I'm not sure I understand
why we need to flip calls in the first place. Since this CL is meant to address
a beta blocker, should we be looking to revert prior changes instead?

[sadrul, 2017/03/09 18:55:05]

I missed this. Thanks for catching this.

I think we should be consistent, and make sure the calls are flipped in all
cases. So this needs to  be flipped too.

   }
   OnAfterUserAction();
   return true;
 }
 
 bool Textfield::GetTextFromRange(const gfx::Range& range,
                                  base::string16* range_text) const {
   if (!ImeEditingAllowed() || !range.IsValid())
     return false;
 
@@ skipping 133 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();
 }
 
 base::string16 Textfield::GetSelectionClipboardText() const {
   base::string16 selection_clipboard_text;
   ui::Clipboard::GetForCurrentThread()->ReadText(
       ui::CLIPBOARD_TYPE_SELECTION, &selection_clipboard_text);
   return selection_clipboard_text;
 }
 
 void Textfield::ExecuteTextEditCommand(ui::TextEditCommand command) {
   DestroyTouchSelection();
 
-  bool add_to_kill_buffer = false;
+  // We only execute the commands enabled in Textfield::IsTextEditCommandEnabled
+  // below. Hence don't do a virtual IsTextEditCommandEnabled call.
+  if (!Textfield::IsTextEditCommandEnabled(command))
+    return;
+
+  gfx::SelectionModel selection_model = GetSelectionModel();
+  OnBeforeUserAction();
 
   // Some codepaths may bypass GetCommandForKeyEvent, so any selection-dependent
   // modifications of the command should happen here.
   switch (command) {
     case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_LINE:
     case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_PARAGRAPH:
     case ui::TextEditCommand::DELETE_TO_END_OF_LINE:
     case ui::TextEditCommand::DELETE_TO_END_OF_PARAGRAPH:
-      add_to_kill_buffer = text_input_type_ != ui::TEXT_INPUT_TYPE_PASSWORD;
-    // Fall through.
     case ui::TextEditCommand::DELETE_WORD_BACKWARD:
     case ui::TextEditCommand::DELETE_WORD_FORWARD:
       if (HasSelection())
         command = ui::TextEditCommand::DELETE_FORWARD;
       break;
     default:
       break;
   }
+  bool text_changed = ExecuteTextEditCommandImpl(command);
+  bool cursor_changed = text_changed;
+
+  cursor_changed |= GetSelectionModel() != selection_model;
+  if (cursor_changed && HasSelection())
+    UpdateSelectionClipboard();
+  OnAfterUserAction();
+  UpdateAfterChange(text_changed, cursor_changed);
+}
+
+bool Textfield::ExecuteTextEditCommandImpl(ui::TextEditCommand command) {
+  bool add_to_kill_buffer = false;
+
+  // Some codepaths may bypass GetCommandForKeyEvent, so any selection-dependent
+  // modifications of the command should happen here.
+  switch (command) {
+    case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_LINE:
+    case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_PARAGRAPH:
+    case ui::TextEditCommand::DELETE_TO_END_OF_LINE:
+    case ui::TextEditCommand::DELETE_TO_END_OF_PARAGRAPH:
+      add_to_kill_buffer = text_input_type_ != ui::TEXT_INPUT_TYPE_PASSWORD;
+      break;
+    default:
+      break;
+  }
 
-  // We only execute the commands enabled in Textfield::IsTextEditCommandEnabled
-  // below. Hence don't do a virtual IsTextEditCommandEnabled call.
-  if (!Textfield::IsTextEditCommandEnabled(command))
-    return;
-
-  bool text_changed = false;
-  bool cursor_changed = false;
   bool rtl = GetTextDirection() == base::i18n::RIGHT_TO_LEFT;
   gfx::VisualCursorDirection begin = rtl ? gfx::CURSOR_RIGHT : gfx::CURSOR_LEFT;
   gfx::VisualCursorDirection end = rtl ? gfx::CURSOR_LEFT : gfx::CURSOR_RIGHT;
-  gfx::SelectionModel selection_model = GetSelectionModel();
+  bool is_text_changed = false;
 
-  OnBeforeUserAction();
   switch (command) {
     case ui::TextEditCommand::DELETE_BACKWARD:
-      text_changed = cursor_changed = model_->Backspace(add_to_kill_buffer);
+      is_text_changed = model_->Backspace(add_to_kill_buffer);
       break;
     case ui::TextEditCommand::DELETE_FORWARD:
-      text_changed = cursor_changed = model_->Delete(add_to_kill_buffer);
+      is_text_changed = model_->Delete(add_to_kill_buffer);
       break;
     case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_LINE:
     case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_PARAGRAPH:
       model_->MoveCursor(gfx::LINE_BREAK, begin, gfx::SELECTION_RETAIN);
-      text_changed = cursor_changed = model_->Backspace(add_to_kill_buffer);
+      is_text_changed = model_->Backspace(add_to_kill_buffer);
       break;
     case ui::TextEditCommand::DELETE_TO_END_OF_LINE:
     case ui::TextEditCommand::DELETE_TO_END_OF_PARAGRAPH:
       model_->MoveCursor(gfx::LINE_BREAK, end, gfx::SELECTION_RETAIN);
-      text_changed = cursor_changed = model_->Delete(add_to_kill_buffer);
+      is_text_changed = model_->Delete(add_to_kill_buffer);
       break;
     case ui::TextEditCommand::DELETE_WORD_BACKWARD:
       model_->MoveCursor(gfx::WORD_BREAK, begin, gfx::SELECTION_RETAIN);
-      text_changed = cursor_changed = model_->Backspace(add_to_kill_buffer);
+      is_text_changed = model_->Backspace(add_to_kill_buffer);
       break;
     case ui::TextEditCommand::DELETE_WORD_FORWARD:
       model_->MoveCursor(gfx::WORD_BREAK, end, gfx::SELECTION_RETAIN);
-      text_changed = cursor_changed = model_->Delete(add_to_kill_buffer);
+      is_text_changed = model_->Delete(add_to_kill_buffer);
       break;
     case ui::TextEditCommand::MOVE_BACKWARD:
       model_->MoveCursor(gfx::CHARACTER_BREAK, begin, gfx::SELECTION_NONE);
       break;
     case ui::TextEditCommand::MOVE_BACKWARD_AND_MODIFY_SELECTION:
       model_->MoveCursor(gfx::CHARACTER_BREAK, begin, gfx::SELECTION_RETAIN);
       break;
     case ui::TextEditCommand::MOVE_FORWARD:
       model_->MoveCursor(gfx::CHARACTER_BREAK, end, gfx::SELECTION_NONE);
       break;
@@ skipping 79 matching lines @@
       break;
     case ui::TextEditCommand::MOVE_WORD_RIGHT:
       model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT,
                          gfx::SELECTION_NONE);
       break;
     case ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
       model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT,
                          kWordSelectionBehavior);
       break;
     case ui::TextEditCommand::UNDO:
-      text_changed = cursor_changed = model_->Undo();
+      is_text_changed = model_->Undo();
       break;
     case ui::TextEditCommand::REDO:
-      text_changed = cursor_changed = model_->Redo();
+      is_text_changed = model_->Redo();
       break;
     case ui::TextEditCommand::CUT:
-      text_changed = cursor_changed = Cut();
+      is_text_changed = Cut();
       break;
     case ui::TextEditCommand::COPY:
       Copy();
       break;
     case ui::TextEditCommand::PASTE:
-      text_changed = cursor_changed = Paste();
+      is_text_changed = Paste();
       break;
     case ui::TextEditCommand::SELECT_ALL:
       SelectAll(false);
       break;
     case ui::TextEditCommand::TRANSPOSE:
-      text_changed = cursor_changed = model_->Transpose();
+      is_text_changed = model_->Transpose();
       break;
     case ui::TextEditCommand::YANK:
-      text_changed = cursor_changed = model_->Yank();
+      is_text_changed = model_->Yank();
       break;
     case ui::TextEditCommand::INSERT_TEXT:
     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();
-  UpdateAfterChange(text_changed, cursor_changed);
-  OnAfterUserAction();
+  return is_text_changed;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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