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

chrome/browser/ui/omnibox/omnibox_edit_model.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 "chrome/browser/ui/omnibox/omnibox_edit_model.h"
 
 #include <string>
 
 #include "base/format_macros.h"
 #include "base/metrics/histogram.h"
@@ skipping 865 matching lines @@
       // pressed, even though maybe it isn't any more.  There is no obvious
       // right answer here :(
     }
     view_->OnTemporaryTextMaybeChanged(DisplayTextFromUserText(text),
                                        save_original_selection);
     return;
   }
 
   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
   // but now don't (or |destination_for_temporary_text_change| would have been
   // non-NULL). This can happen when deleting the selected item in the popup.
   // In this case, we've already reverted the popup to the default match, so we
   // need to revert ourselves as well.
   if (has_temporary_text_) {
     RevertTemporaryText(false);
     call_controller_onchanged = false;
@@ skipping 366 matching lines @@
 }
 
 void OmniboxEditModel::ClassifyStringForPasteAndGo(
     const string16& text,
     AutocompleteMatch* match,
     GURL* alternate_nav_url) const {
   DCHECK(match);
   AutocompleteClassifierFactory::GetForProfile(profile_)->Classify(text,
       string16(), false, false, match, alternate_nav_url);
 }