Commit 5beff680 authored by manuk's avatar manuk Committed by Commit Bot

[omnibox]: Show original text when reverting with a default keyword match.

Previously, reverting the omnibox's selected match (e.g. by pressing escape)
when the default match is in keyword mode resulted in incorrect behavior
regarding the user text, autocomplete text, and text styling:

(1) The user text has its first word removed for each time keyword mode was
entered. If the results popup included a navigation result, pressing
up/down to select the navigation result leaves keyword mode. Pressing
up/down to select a search result or pressing escape to revert the omnibox text
reenters keyword mode. Each time keyword mode is entered, the user text drops
the first word. E.g. if the 2nd result is a navigation result, and the third
result is a search result, pressing <down><down><up><escape> would enter keyword
mode twice, and drop the first two words of the user text.

(2) The user text would be styled as a URL (grey font color) if the result
selected before reverting was a URL result.

(3) The autocomplete text is not restored.

This CL addresses (1) and (3). (2) is partially addressed as well;
for cases where the default match has autocomplete text, restoring it correctly
also restores the user text styling. The fix is 2 part:

(1) Before, the first word of the user text was stripped upon entering keyword
mode because we assumed the first word must be the keyword word. However, this
assumption is only true when accepting a keyword (e.g. pressing <space> or
<tab>), and not when entering keyword mode by reverting the omnibox's selected
match (e.g. pressing <escape>). This CL moves the stripping call from
OmniboxEditModel::OnPopupDataChanged, which is called in both cases, to
OmniboxEditModel::OnCurrentMatchChanged, which is only called in the first case.

(2) Before, the cursor was reset to the beginning of the user text when entering
keyword mode while the default match was either already selected or becoming
selected. The first occurs when (a) inserting a space between a keyword name and
a search string, while the latter occurs when (b) reverting the omnibox's
selected match. This was desired behavior in case (a), but undesired in case
(b). We incorrectly assumed it was ok to do this in case (b) because we would be
reverting the text anyways. However, the revert happens prior to this, resulting
in incorrect autocomplete selection. This CL conditions the cursor reset on not
having autocomplete text.

Bug: 702867
Change-Id: Iac0bdae7e17c86b3e0feb3075dd916d85e5d7e39
Reviewed-on: https://chromium-review.googlesource.com/c/1458697
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: default avatarTommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630105}
parent 8b43cc42
......@@ -1186,8 +1186,6 @@ void OmniboxEditModel::OnPopupDataChanged(
keyword_ = keyword;
is_keyword_hint_ = is_keyword_hint;
if (!keyword_was_selected && is_keyword_selected()) {
// We just entered keyword mode, so remove the keyword from the input.
user_text_ = MaybeStripKeyword(user_text_);
// Since we entered keyword mode, record the reason. Note that we
// don't do this simply because the keyword changes, since the user
// never left keyword mode.
......@@ -1233,7 +1231,8 @@ void OmniboxEditModel::OnPopupDataChanged(
const base::string16& user_text =
user_input_in_progress_ ? user_text_ : view_->GetText();
if (keyword_state_changed && is_keyword_selected()) {
if (keyword_state_changed && is_keyword_selected() &&
inline_autocomplete_text_.empty()) {
// 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
......@@ -1255,16 +1254,6 @@ void OmniboxEditModel::OnPopupDataChanged(
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;
}
// We need to invoke OnChanged in case the destination url changed (as could
// happen when control is toggled).
if (call_controller_onchanged)
......@@ -1401,6 +1390,15 @@ void OmniboxEditModel::OnCurrentMatchChanged() {
match.GetKeywordUIState(service, &keyword, &is_keyword_hint);
if (popup_model())
popup_model()->OnResultChanged();
if (!is_keyword_selected() && !is_keyword_hint && !keyword.empty()) {
// We just entered keyword mode, so remove the keyword from the input.
// We don't call MaybeStripKeyword, as we haven't yet updated our internal
// state (keyword_ and is_keyword_hint_), and MaybeStripKeyword checks this.
user_text_ =
KeywordProvider::SplitReplacementStringFromInput(user_text_, false);
}
// OnPopupDataChanged() resets OmniboxController's |current_match_| early
// on. Therefore, copy match.inline_autocompletion to a temp to preserve
// its value across the entire call.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment