Cursor jumps to end of omnibox when activating keyword mode via inserted space

chrome/browser/ui/omnibox/omnibox_edit_model.cc

Index: chrome/browser/ui/omnibox/omnibox_edit_model.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_edit_model.cc b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
index 8232aae8b8de4a26080de0f111343c22d7212a14..f811a5ed4fc557d2388aeccd0cd0174e2b140770 100644
--- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
+++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
@@ -883,9 +883,19 @@ void OmniboxEditModel::OnPopupDataChanged(
 
   bool call_controller_onchanged = true;
   inline_autocomplete_text_ = text;
-  if (view_->OnInlineAutocompleteTextMaybeChanged(
-      DisplayTextFromUserText(user_text_ + inline_autocomplete_text_),
-      DisplayTextFromUserText(user_text_).length()))
+
+  // Keyword search mode can be activated if the text before the inserted space
+  // matches a keyword. The position of the cursor should be retained after
+  // keyword mode is activated and it should be at the left of the 'search'
+  // string. The window text should be set after stripping the keyword string
+  // and the inserted space. And to set the caret at the left of the search
+  // string to maintain the old position, it needs to be set to 0.

[Peter Kasting, 2012/10/03 17:08:12]

Thanks for adding comments here.

I wonder if we could make them even clearer, something like:

  if (keyword_state_changed && KeywordIsSelected()) {
    // If we reach here, the user most likely entered keyword mode by inserting
    // a space between a keyword name and a search string (as pressing space or
    // tab after the keyword name alone would have been be handled in
    // MaybeAcceptKeywordBySpace() by calling AcceptKeyword(), which won't reach
    // here).  In this case, we don't want to call
    // OnInlineAutocompleteTextMaybeChanged() as normal, because that will
    // correctly change the text (to the search string alone) but move the caret
    // to the end of the string; instead we want the caret at the start of the
    // search string since that's where it was in the original input.  So we set
    // the text and caret position directly.
    //
    // It may also be possible to reach here if we're reverting from having
    // temporary text back to a default match that's a keyword search, but in
    // that case the RevertTemporaryText() call below will reset the caret or
    // selection correctly so the caret positioning we do here won't matter.
    ...

[Joe Thomas, 2012/10/03 18:04:59]

This looks a lot better than the comments that I added. Thank you so much.

+  if (keyword_state_changed && KeywordIsSelected())

[Peter Kasting, 2012/10/03 17:08:12]

Nit: {} needed on both arms, as one arm's body is >1 line

[Joe Thomas, 2012/10/03 18:04:59]

Done.

+    view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text_), 0,
+                                                            false, false);
+  else if (view_->OnInlineAutocompleteTextMaybeChanged(
+           DisplayTextFromUserText(user_text_ + inline_autocomplete_text_),
+           DisplayTextFromUserText(user_text_).length()))
     call_controller_onchanged = false;
 
   // If |has_temporary_text_| is true, then we previously had a manual selection