Accept keyword by pressing space.

chrome/browser/autocomplete/autocomplete_edit.cc

 // Copyright (c) 2011 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/autocomplete/autocomplete_edit.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/metrics/histogram.h"
@@ skipping 27 matching lines @@
 
 AutocompleteEditController::~AutocompleteEditController() {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // AutocompleteEditModel::State
 
 AutocompleteEditModel::State::State(bool user_input_in_progress,
                                     const std::wstring& user_text,
                                     const std::wstring& keyword,
-                                    bool is_keyword_hint,
-                                    KeywordUIState keyword_ui_state)
+                                    bool is_keyword_hint)
     : user_input_in_progress(user_input_in_progress),
       user_text(user_text),
       keyword(keyword),
-      is_keyword_hint(is_keyword_hint),
-      keyword_ui_state(keyword_ui_state) {
+      is_keyword_hint(is_keyword_hint) {
 }
 
 AutocompleteEditModel::State::~State() {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // AutocompleteEditModel
 
 AutocompleteEditModel::AutocompleteEditModel(
     AutocompleteEditView* view,
     AutocompleteEditController* controller,
     Profile* profile)
     : view_(view),
       popup_(NULL),
       controller_(controller),
       has_focus_(false),
       user_input_in_progress_(false),
       just_deleted_text_(false),
       has_temporary_text_(false),
-      original_keyword_ui_state_(NORMAL),
       paste_state_(NONE),
       control_key_state_(UP),
       is_keyword_hint_(false),
-      keyword_ui_state_(NORMAL),
       paste_and_go_transition_(PageTransition::TYPED),
       profile_(profile) {
 }
 
 AutocompleteEditModel::~AutocompleteEditModel() {
 }
 
 void AutocompleteEditModel::SetPopupModel(AutocompletePopupModel* popup_model) {
   popup_ = popup_model;
   registrar_.Add(this,
@@ skipping 18 matching lines @@
     // on switching back, typing will "just work").
     const std::wstring user_text(UserTextFromDisplayText(view_->GetText()));
     if (user_text.empty()) {
       view_->RevertAll();
       view_->SelectAll(true);
     } else {
       InternalSetUserText(user_text);
     }
   }
 
-  return State(user_input_in_progress_, user_text_, keyword_, is_keyword_hint_,
-               keyword_ui_state_);
+  return State(user_input_in_progress_, user_text_, keyword_, is_keyword_hint_);
 }
 
 void AutocompleteEditModel::RestoreState(const State& state) {
   // Restore any user editing.
   if (state.user_input_in_progress) {
     // NOTE: Be sure and set keyword-related state BEFORE invoking
     // DisplayTextFromUserText(), as its result depends upon this state.
     keyword_ = state.keyword;
     is_keyword_hint_ = state.is_keyword_hint;
-    keyword_ui_state_ = state.keyword_ui_state;
     view_->SetUserText(state.user_text,
         DisplayTextFromUserText(state.user_text), false);
   }
 }
 
 AutocompleteMatch AutocompleteEditModel::CurrentMatch() {
   AutocompleteMatch match;
   GetInfoForCurrentText(&match, NULL);
   return match;
 }
@@ skipping 150 matching lines @@
   user_input_in_progress_ = in_progress;
   controller_->OnInputInProgress(in_progress);
 }
 
 void AutocompleteEditModel::Revert() {
   SetInputInProgress(false);
   paste_state_ = NONE;
   InternalSetUserText(std::wstring());
   keyword_.clear();
   is_keyword_hint_ = false;
-  keyword_ui_state_ = NORMAL;
   has_temporary_text_ = false;
   view_->SetWindowTextAndCaretPos(permanent_text_,
                                   has_focus_ ? permanent_text_.length() : 0);
 }
 
 void AutocompleteEditModel::StartAutocomplete(
     bool has_selected_text,
     bool prevent_inline_autocomplete) const {
+  bool keyword_is_selected = KeywordIsSelected();
   popup_->StartAutocomplete(user_text_, GetDesiredTLD(),
       prevent_inline_autocomplete || just_deleted_text_ ||
       (has_selected_text && inline_autocomplete_text_.empty()) ||
-      (paste_state_ != NONE), keyword_ui_state_ == KEYWORD,
-      keyword_ui_state_ != NO_KEYWORD);
+      (paste_state_ == REPLACED_ALL), keyword_is_selected, keyword_is_selected);
 }
 
 bool AutocompleteEditModel::CanPasteAndGo(const std::wstring& text) const {
   if (!view_->GetCommandUpdater()->IsCommandEnabled(IDC_OPEN_CURRENT_URL))
     return false;
 
   AutocompleteMatch match;
   profile_->GetAutocompleteClassifier()->Classify(text, std::wstring(), false,
       &match, &paste_and_go_alternate_nav_url_);
   paste_and_go_url_ = match.destination_url;
@@ skipping 26 matching lines @@
     // When the user hit enter on the existing permanent URL, treat it like a
     // reload for scoring purposes.  We could detect this by just checking
     // user_input_in_progress_, but it seems better to treat "edits" that end
     // up leaving the URL unchanged (e.g. deleting the last character and then
     // retyping it) as reloads too.  We exclude non-TYPED transitions because if
     // the transition is GENERATED, the user input something that looked
     // different from the current URL, even if it wound up at the same place
     // (e.g. manually retyping the same search query), and it seems wrong to
     // treat this as a reload.
     match.transition = PageTransition::RELOAD;
-  } else if (for_drop || ((paste_state_ != NONE) &&
+  } else if (for_drop || ((paste_state_ == REPLACED_ALL) &&
                           match.is_history_what_you_typed_match)) {
     // When the user pasted in a URL and hit enter, score it like a link click
     // rather than a normal typed URL, so it doesn't get inline autocompleted
     // as aggressively later.
     match.transition = PageTransition::LINK;
   }
 
   if (match.type == AutocompleteMatch::SEARCH_WHAT_YOU_TYPED ||
       match.type == AutocompleteMatch::SEARCH_HISTORY ||
       match.type == AutocompleteMatch::SEARCH_SUGGEST) {
@@ skipping 62 matching lines @@
 
   if (disposition != NEW_BACKGROUND_TAB) {
     controller_->OnAutocompleteWillAccept();
     view_->RevertAll();  // Revert the box to its unedited state
   }
   controller_->OnAutocompleteAccept(url, disposition, transition,
                                     alternate_nav_url);
 }
 
 void AutocompleteEditModel::AcceptKeyword() {
+  DCHECK(is_keyword_hint_ && !keyword_.empty());
+
   view_->OnBeforePossibleChange();
   view_->SetWindowTextAndCaretPos(std::wstring(), 0);
   is_keyword_hint_ = false;
-  keyword_ui_state_ = KEYWORD;
   view_->OnAfterPossibleChange();
   just_deleted_text_ = false;  // OnAfterPossibleChange() erroneously sets this
                                // since the edit contents have disappeared.  It
                                // doesn't really matter, but we clear it to be
                                // consistent.
   UserMetrics::RecordAction(UserMetricsAction("AcceptedKeywordHint"), profile_);
 }
 
 void AutocompleteEditModel::ClearKeyword(const std::wstring& visible_text) {
   view_->OnBeforePossibleChange();
   const std::wstring window_text(keyword_ + visible_text);
   view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length());
   keyword_.clear();
-  keyword_ui_state_ = NORMAL;
+  is_keyword_hint_ = false;
   view_->OnAfterPossibleChange();
   just_deleted_text_ = true;  // OnAfterPossibleChange() fails to clear this
                               // since the edit contents have actually grown
                               // longer.
 }
 
 bool AutocompleteEditModel::query_in_progress() const {
   return !popup_->autocomplete_controller()->done();
 }
 
@@ skipping 25 matching lines @@
 bool AutocompleteEditModel::OnEscapeKeyPressed() {
   if (has_temporary_text_) {
     AutocompleteMatch match;
     popup_->InfoForCurrentSelection(&match, NULL);
     if (match.destination_url != original_url_) {
       // The user typed something, then selected a different item.  Restore the
       // text they typed and change back to the default item.
       // NOTE: This purposefully does not reset paste_state_.
       just_deleted_text_ = false;
       has_temporary_text_ = false;
-      keyword_ui_state_ = original_keyword_ui_state_;
       popup_->ResetToDefaultMatch();
       view_->OnRevertTemporaryText();
       return true;
     }
   }
 
   // If the user wasn't editing, but merely had focus in the edit, allow <esc>
   // to be processed as an accelerator, so it can still be used to stop a load.
   // When the permanent text isn't all selected we still fall through to the
   // SelectAll() call below so users can arrow around in the text and then hit
@@ skipping 21 matching lines @@
     }
     if ((old_state != DOWN_WITH_CHANGE) && popup_->IsOpen()) {
       // Autocomplete history provider results may change, so refresh the
       // popup.  This will force user_input_in_progress_ to true, but if the
       // popup is open, that should have already been the case.
       view_->UpdatePopup();
     }
   }
 }
 
+void AutocompleteEditModel::OnPaste(bool replacing_all) {
+  paste_state_ = replacing_all ? REPLACING_ALL : PASTING;
+}
+
 void AutocompleteEditModel::OnUpOrDownKeyPressed(int count) {
   // NOTE: This purposefully don't trigger any code that resets paste_state_.
 
   if (!popup_->IsOpen()) {
     if (!query_in_progress()) {
       // The popup is neither open nor working on a query already.  So, start an
       // autocomplete query for the current text.  This also sets
       // user_input_in_progress_ to true, which we want: if the user has started
       // to interact with the popup, changing the permanent_text_ shouldn't
       // change the displayed text.
@@ skipping 12 matching lines @@
     // normally.
     popup_->Move(count);
   }
 }
 
 void AutocompleteEditModel::OnPopupDataChanged(
     const std::wstring& text,
     GURL* destination_for_temporary_text_change,
     const std::wstring& keyword,
     bool is_keyword_hint) {
-  KeywordUIState old_keyword_ui_state = keyword_ui_state_;
-
   // Update keyword/hint-related local state.
   bool keyword_state_changed = (keyword_ != keyword) ||
       ((is_keyword_hint_ != is_keyword_hint) && !keyword.empty());
   if (keyword_state_changed) {
     keyword_ = keyword;
     is_keyword_hint_ = is_keyword_hint;
-  }
 
-  // Update |keyword_ui_state_| only when necessary. It may be changed even if
-  // |keyword_state_changed| is false.
-  if (keyword_ui_state_ != NO_KEYWORD) {
-    keyword_ui_state_ = (is_keyword_hint_ || keyword_.empty()) ?
-        NORMAL : KEYWORD;
+    // |is_keyword_hint_| should always be false if |keyword_| is empty.
+    DCHECK(!keyword_.empty() || !is_keyword_hint_);
   }
 
   // Handle changes to temporary text.
   if (destination_for_temporary_text_change != NULL) {
     const bool save_original_selection = !has_temporary_text_;
     if (save_original_selection) {
       // Save the original selection and URL so it can be reverted later.
       has_temporary_text_ = true;
       original_url_ = *destination_for_temporary_text_change;
-      original_keyword_ui_state_ = old_keyword_ui_state;
       inline_autocomplete_text_.clear();
     }
     if (control_key_state_ == DOWN_WITHOUT_CHANGE) {
       // Arrowing around the popup cancels control-enter.
       control_key_state_ = DOWN_WITH_CHANGE;
       // Now things are a bit screwy: the desired_tld has changed, but if we
       // update the popup, the new order of entries won't match the old, so the
       // user's selection gets screwy; and if we don't update the popup, and the
       // user reverts, then the selected item will be as if control is still
       // pressed, even though maybe it isn't any more.  There is no obvious
@@ skipping 24 matching lines @@
     const std::wstring& new_text,
     bool selection_differs,
     bool text_differs,
     bool just_deleted_text,
     bool allow_keyword_ui_change) {
   // Update the paste state as appropriate: if we're just finishing a paste
   // that replaced all the text, preserve that information; otherwise, if we've
   // made some other edit, clear paste tracking.
   if (paste_state_ == REPLACING_ALL)
     paste_state_ = REPLACED_ALL;
+  else if (paste_state_ == PASTING)
+    paste_state_ = PASTED;
   else if (text_differs)
     paste_state_ = NONE;
 
   // Modifying the selection counts as accepting the autocompleted text.
   const bool user_text_changed =
       text_differs || (selection_differs && !inline_autocomplete_text_.empty());
 
   // If something has changed while the control key is down, prevent
   // "ctrl-enter" until the control key is released.  When we do this, we need
   // to update the popup if it's open, since the desired_tld will have changed.
   if ((text_differs || selection_differs) &&
       (control_key_state_ == DOWN_WITHOUT_CHANGE)) {
     control_key_state_ = DOWN_WITH_CHANGE;
     if (!text_differs && !popup_->IsOpen())
       return false;  // Don't open the popup for no reason.
   } else if (!user_text_changed) {
     return false;
   }
 
-  const bool had_keyword = KeywordIsSelected();
-
   // If the user text has not changed, we do not want to change the model's
   // state associated with the text.  Otherwise, we can get surprising behavior
   // where the autocompleted text unexpectedly reappears, e.g. crbug.com/55983
   if (user_text_changed) {
-    InternalSetUserText(UserTextFromDisplayText(new_text));
+    const std::wstring new_user_text = UserTextFromDisplayText(new_text);
+
+    // Try to accept the current keyword if the user only typed a space at the
+    // end of content. Model's state and popup will be updated when the keyword
+    // is accepted. So we just need to return false here.
+    if (allow_keyword_ui_change && !selection_differs &&
+        MaybeAcceptKeywordBySpace(new_user_text))

[Peter Kasting, 2011/01/14 00:43:31]

I think that rather than trying to detect this case after a change, you should
just detect it when the space key is pressed, similar to how we detect
up/down/tab/control/etc. when they're pressed and take action immediately.

I think this may allow you to get rid of the concept of PASTING/PASTED entirely.

[James Su, 2011/01/14 01:03:54]

The difference between Tab and Space is that Tab doesn't insert anything into
the buffer but space does. And things become very complicated when involving an
IME. For example an IME can change the behavior of space to anything else, it
may insert an ideographic space or even nothing. Or if there is a candidate
list, space may be used for choosing the first candidate.
And detecting space key event requires to change all AutocompleteEditView
implementations, which would also much more complicated than adding
PASTING/PASTED.

[Peter Kasting, 2011/01/14 01:20:01]

This is part of why I suggested changing the underlying representation to be a
(keyword, input string) pair.  But even without making that change it's easy
enough to modify the |user_text_|.  In other words, when space triggers keyword
mode, we should never send it to the edit at all, just modify our local
variables appropriately and rerun the query, which will completely change the
contents of the edit.
We don't want space to toggle into keyword mode any time IME composition is
active.  Doesn't that handle all the cases?
I wasn't trying to optimize the total size of this patch.  I was merely
commenting that once you make this change, PASTING/PASTED become unnecessary.

[James Su, 2011/01/14 02:02:46]

The problem is, the IME will handle the space key first, then it may insert
arbitrary content into the edit box, and even if the IME inserts anything into
the edit box, it may still return an unhandled space key to us.
An IME can handle key events without composition text. Actually most popular
Chinese IMEs on Windows don't use composition text at all.
I'm just worry about the complexity regarding to IME support and platform
difference. Three platforms handle keyboard and IME event in totally different
ways, it's very hard to get things right on all three platforms.

[Peter Kasting, 2011/01/14 02:30:36]

It seems like I don't understand how IMEs work well enough.  How is it that we
can get away with all the other keychecks we do when receiving WM_KEYDOWN?  I am
surprised that we'd have no opportunity to intercept a keydown event before the
IME processes it.  There must be some way.
That's true, but we're already in that situation.

There are practical problems with trying to detect space being pressed
after-the-fact, but I'm more concerned about the conceptual issue.  Conceptually
we really want to treat the space keypress like an accelerator, not as a
character, like we do when we try and detect that the edit has gained a space
character and react to it.

[James Su, 2011/01/14 03:26:22]

There is no way on Windows to intercept a key event before the IME. The
situation is even worse on Linux, that the most popular input method intercepts
all events before the whole application, so when the input method is in action,
the application can not receive any events.
The situation is much better for keys that don't generate any content (like Tab,
arrow keys, etc.). Because we can simply do nothing if the input method consumes
them.
We can treat keys like Tab as accelerators, because they don't generate content.
But we can not do it for Space, because it generates content, so it IS a
character.

[Peter Kasting, 2011/01/14 17:31:33]

I've been reading a few MSDN docs and I don't think this is correct.

First, it sounds as if when the IME processes VK_SPACE and does something
special with it, the WM_KEYDOWN and WM_KEYUP messages that are passed on to us
are changed to say VK_PROCESSKEY instead of VK_SPACE.  That sounds as if just
checking VK_SPACE will do what we want since it won't occur when the IME handles
it specially.  It would be nice to test this.

Furthermore, it's possible to retrieve the original keycode in a VK_PROCESSKEY
case with ImmGetVirtualKey(), and it's also possible to set our own input
context so we get first crack at all IME messages before we hand them to the
"real" IME.  I'm not sure either of those is terribly useful, though.

[James Su, 2011/01/14 18:04:12]

Yes exactly. But if the user uses an IME and enables "full width" mode, then the
IME will handle the space key and insert an ideographic space instead. An IME
may even handle the space key then insert a normal half width space character. I
do think we need to accept keyword in such cases. But the IME may not always
generate an whitespace character when handling the space key, so we still need
to check the content generated by the space key to see whether it's a whitespace
character or not.

So again, the key point here is: the space key IS a character, which may
generate arbitrary content when using different IME and/or keyboard layout. We
cannot simply ignore the content.
As I said above, no matter what we do, we always need to check the content
generated by a space key event. And please take Linux and Mac into account as
well, which have totally different mechanism for handling keyboard events.

[Peter Kasting, 2011/01/19 19:24:42]

It seems like acting directly on VK_SPACE might be correct, but insufficient to
catch everything we care about.  Is that accurate?

If so, then what about checking WM_CHAR or WM_IME_CHAR for a space character? 
Maybe checking WM_IME_CHAR catches the other cases?

What about cases where the user doesn't press space (and space only) directly
but a space is inserted?  Maybe it's one of those Chinese IMEs that inserts
whitespace during composition or maybe we had a composition string and then
deleted most of it or I don't know.  Do we need a two-phase check for this where
we only act on a space character when we also just saw a WM_KEYDOWN or
VK_PROCESSKEY and ImmGetVirtualKey() returned VK_SPACE?
I want to understand my options on Windows and decide what the best course is
there before moving on to worry about Mac and Linux.

[James Su, 2011/01/19 19:57:15]

One possible case is that an IME may use a different key to insert whitespace
character, and I think we still need to trigger keyword mode in this case.
An IME may also use WM_IME_COMPOSITION to insert the whitespace character. I'm
actually not sure about WM_IME_CHAR.
I'm actually don't want to limit it to VK_SPACE. As I said above, an IME can use
any key to insert whitespace character, and we should definitely treat it as
normal space key and trigger keyword mode.
And besides the key event issue, for some IMEs, especially Korean and
Traditional Chinese, when the user press the space key, the IME may confirm the
current composition text and then insert a whitespace character (either half or
full width) immediately. Some IMEs may combine the whitespace character with the
current composition text and confirm them together. We need to trigger keyword
mode for such cases as well, otherwise those users may never trigger a keyword
in their native language by space.

[Peter Kasting, 2011/01/19 20:07:05]

We should not trigger keyword mode for this case.  The whole point is to treat
space as an accelerator.  If I wanted keyword mode to trigger on some other
method of inserting whitespace here, I'd want it to trigger when pasting in a
string with a space too.
Supposedly we get WM_IME_CHAR any time the IME has synthesized a character from
one or more original keypresses.
We should definitely _not_ do that.
This is why I'm trying to get us to a solution that involves looking at the
original keypress, not at what the IME inserts.

+      return false;
+
+    InternalSetUserText(new_user_text);
     has_temporary_text_ = false;
 
     // Track when the user has deleted text so we won't allow inline
     // autocomplete.
     just_deleted_text_ = just_deleted_text;
   }
 
-  // Disable the fancy keyword UI if the user didn't already have a visible
-  // keyword and the view doesn't want us to change the keyword UI state.
-  // This prevents us from showing the fancy UI and interrupting the user's
-  // editing if, for example, the user happens to have a keyword for 'a',
-  // types 'ab' then puts a space between the 'a' and the 'b'.
-  // If |keyword_ui_state_| is set to NORMAL here, then it will be updated
-  // to a proper value in OnPopupDataChanged() method according to the new
-  // match result.
-  if (!had_keyword)
-    keyword_ui_state_ = allow_keyword_ui_change ? NORMAL : NO_KEYWORD;
-
   view_->UpdatePopup();
   return true;
 }
 
 void AutocompleteEditModel::PopupBoundsChangedTo(const gfx::Rect& bounds) {
   controller_->OnPopupBoundsChanged(bounds);
 }
 
 void AutocompleteEditModel::ResultsUpdated() {
   UpdateSuggestedSearchText();
@@ skipping 53 matching lines @@
   OnPopupDataChanged(inline_autocomplete_text, NULL, keyword, is_keyword_hint);
 }
 
 void AutocompleteEditModel::InternalSetUserText(const std::wstring& text) {
   user_text_ = text;
   just_deleted_text_ = false;
   inline_autocomplete_text_.clear();
 }
 
 bool AutocompleteEditModel::KeywordIsSelected() const {
-  return keyword_ui_state_ == KEYWORD;
+  return !is_keyword_hint_ && !keyword_.empty();
 }
 
 std::wstring AutocompleteEditModel::DisplayTextFromUserText(
     const std::wstring& text) const {
   return KeywordIsSelected() ?
       KeywordProvider::SplitReplacementStringFromInput(text, false) : text;
 }
 
 std::wstring AutocompleteEditModel::UserTextFromDisplayText(
     const std::wstring& text) const {
@@ skipping 51 matching lines @@
             i->inline_autocomplete_offset != std::wstring::npos) {
           suggested_text = WideToUTF16(i->fill_into_edit.substr(
                                            i->inline_autocomplete_offset));
           break;
         }
       }
     }
   }
   controller_->OnSetSuggestedSearchText(suggested_text);
 }
+
+bool AutocompleteEditModel::MaybeAcceptKeywordBySpace(
+    const std::wstring& new_user_text) {
+  if (paste_state_ != NONE || !is_keyword_hint_ || keyword_.empty() ||
+      !controller_->IsKeywordHintVisible())
+    return false;
+
+  const size_t old_length = user_text_.length();
+  const size_t new_length = new_user_text.length();
+  if (old_length && old_length + 1 == new_length &&
+      user_text_ == new_user_text.substr(0, old_length) &&
+      IsWhitespace(new_user_text[old_length]) &&
+      !IsWhitespace(user_text_[old_length - 1])) {
+    AcceptKeyword();
+    return true;
+  }
+  return false;
+}