[omnibox] Fixed chrome crash when stepping backwards onto keyword button

Previously, stepping backwards into keyword mode was not supported. This
meant that we were always on the "correct" suggestion before trying to
enter keyword mode. This crash happens when we try to enter keyword mode
before updating the selected line, so there would be no keyword
associated with the selected line, causing a segfault or a check to fail.

Changing the accept/clear/setselection order fixed the crash, but
revealed some problems with the KEYWORD_MODE/FOCUSED_BUTTON_KEYWORD
focus distinctions.  These were combined into a singular KEYWORD_MODE
state.

Bug: 1122158
Change-Id: Ib346087e2ebe333c352de6c901ba8662597c849f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2389160Reviewed-by: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Angela Yoeurng <yoangela@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804511}

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

@@ -75,7 +75,7 @@ OmniboxResultView::OmniboxResultView(
     : AnimationDelegateViews(this),
       popup_contents_view_(popup_contents_view),
       model_index_(model_index),
-      animation_(new gfx::SlideAnimation(this)),
+      keyword_slide_animation_(new gfx::SlideAnimation(this)),
       // Using base::Unretained is correct here. 'this' outlives the callback.
       mouse_enter_exit_handler_(
           base::BindRepeating(&OmniboxResultView::UpdateHoverState,
@@ -150,7 +150,7 @@ SkColor OmniboxResultView::GetColor(OmniboxPart part) const {
 
 void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
   match_ = match.GetMatchWithContentsAndDescriptionPossiblySwapped();
-  animation_->Reset();
+  keyword_slide_animation_->Reset();
 
   suggestion_view_->OnMatchUpdate(this, match_);
   keyword_view_->OnMatchUpdate(this, match_);
@@ -190,11 +190,11 @@ void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
   InvalidateLayout();
 }
 
-void OmniboxResultView::ShowKeyword(bool show_keyword) {
+void OmniboxResultView::ShowKeywordSlideAnimation(bool show_keyword) {
   if (show_keyword)
-    animation_->Show();
+    keyword_slide_animation_->Show();
   else
-    animation_->Hide();
+    keyword_slide_animation_->Hide();
 }
 
 void OmniboxResultView::ApplyThemeAndRefreshIcons(bool force_reapply_styles) {
@@ -280,11 +280,12 @@ void OmniboxResultView::OnSelectionStateChanged() {
       popup_contents_view_->FireAXEventsForNewActiveDescendant(this);
     }
 
-    // TODO(orinj): Eventually the deep digging in this class should get
-    //  replaced with a single local point of access to all selection state.
-    ShowKeyword(selection_state == OmniboxPopupModel::KEYWORD_MODE);
+    // The slide animation is not used in the new suggestion button row UI.
+    ShowKeywordSlideAnimation(
+        !OmniboxFieldTrial::IsKeywordSearchButtonEnabled() &&
+        selection_state == OmniboxPopupModel::KEYWORD_MODE);
   } else {
-    ShowKeyword(false);
+    ShowKeywordSlideAnimation(false);
   }
   ApplyThemeAndRefreshIcons();
 }
@@ -360,7 +361,8 @@ void OmniboxResultView::Layout() {
   if (keyword_view_->GetVisible()) {
     const int max_kw_x =
         suggestion_width - OmniboxMatchCellView::GetTextIndent();
-    suggestion_width = animation_->CurrentValueBetween(max_kw_x, 0);
+    suggestion_width =
+        keyword_slide_animation_->CurrentValueBetween(max_kw_x, 0);
     keyword_view_->SetBounds(suggestion_width, 0, width() - suggestion_width,
                              height());
   }
@@ -592,7 +594,8 @@ const char* OmniboxResultView::GetClassName() const {
 }
 
 void OmniboxResultView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
-  animation_->SetSlideDuration(base::TimeDelta::FromMilliseconds(width() / 4));
+  keyword_slide_animation_->SetSlideDuration(
+      base::TimeDelta::FromMilliseconds(width() / 4));
   InvalidateLayout();
 }
 

chrome/browser/ui/views/omnibox/omnibox_result_view.h

@@ -63,7 +63,8 @@ class OmniboxResultView : public views::View,
   // model has changed.
   void SetMatch(const AutocompleteMatch& match);
 
-  void ShowKeyword(bool show_keyword);
+  // Sets the visibility of the keyword mode slide animation.
+  void ShowKeywordSlideAnimation(bool show_keyword);
 
   // Applies the current theme to the current text and widget colors.
   // Also refreshes the icons which may need to be re-colored as well.
@@ -146,7 +147,7 @@ class OmniboxResultView : public views::View,
   base::string16 accessible_name_;
 
   // For sliding in the keyword search.
-  std::unique_ptr<gfx::SlideAnimation> animation_;
+  std::unique_ptr<gfx::SlideAnimation> keyword_slide_animation_;
 
   // Weak pointers for easy reference.
   OmniboxMatchCellView* suggestion_view_;  // The leading (or left) view.

chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.cc

@@ -137,7 +137,7 @@ OmniboxSuggestionButtonRowView::OmniboxSuggestionButtonRowView(
   keyword_button_ = AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
       this, base::string16(), vector_icons::kSearchIcon, popup_contents_view_,
       OmniboxPopupModel::Selection(model_index_,
-                                   OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD)));
+                                   OmniboxPopupModel::KEYWORD_MODE)));
   tab_switch_button_ =
       AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
           this, l10n_util::GetStringUTF16(IDS_OMNIBOX_TAB_SUGGEST_HINT),
@@ -155,8 +155,7 @@ OmniboxSuggestionButtonRowView::OmniboxSuggestionButtonRowView(
 OmniboxSuggestionButtonRowView::~OmniboxSuggestionButtonRowView() = default;
 
 void OmniboxSuggestionButtonRowView::UpdateFromModel() {
-  SetPillButtonVisibility(keyword_button_,
-                          OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD);
+  SetPillButtonVisibility(keyword_button_, OmniboxPopupModel::KEYWORD_MODE);
   if (keyword_button_->GetVisible()) {
     const OmniboxEditModel* edit_model = model()->edit_model();
     base::string16 keyword;
@@ -215,7 +214,7 @@ void OmniboxSuggestionButtonRowView::ButtonPressed(views::Button* button,
     // Note: Since keyword mode logic depends on state of the edit model, the
     // selection must first be set to prepare for keyword mode before accepting.
     popup_model->SetSelection(OmniboxPopupModel::Selection(
-        model_index_, OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD));
+        model_index_, OmniboxPopupModel::KEYWORD_MODE));
     if (model()->edit_model()->is_keyword_hint()) {
       auto method = metrics::OmniboxEventProto::INVALID;
       if (event.IsMouseEvent()) {
@@ -267,6 +266,13 @@ const AutocompleteMatch& OmniboxSuggestionButtonRowView::match() const {
 void OmniboxSuggestionButtonRowView::SetPillButtonVisibility(
     OmniboxSuggestionRowButton* button,
     OmniboxPopupModel::LineState state) {
-  button->SetVisible(model()->IsControlPresentOnMatch(
-      OmniboxPopupModel::Selection(model_index_, state)));
+  // If the keyword button flag is not enabled, the classic keyword UI is
+  // used instead, so do not show the keyword button
+  if (button == keyword_button_ &&
+      !OmniboxFieldTrial::IsKeywordSearchButtonEnabled()) {
+    button->SetVisible(false);
+  } else {
+    button->SetVisible(model()->IsControlPresentOnMatch(
+        OmniboxPopupModel::Selection(model_index_, state)));
+  }
 }

components/omnibox/browser/omnibox_edit_model.cc

@@ -954,10 +954,11 @@ bool OmniboxEditModel::AcceptKeyword(
     original_user_text_with_keyword_ = user_text_;
   user_text_ = MaybeStripKeyword(user_text_);
 
-  if (PopupIsOpen())
+  if (PopupIsOpen()) {
     popup_model()->SetSelectedLineState(OmniboxPopupModel::KEYWORD_MODE);
-  else
+  } else {
     StartAutocomplete(false, true);
+  }
 
   // When entering keyword mode via tab, the new text to show is whatever the
   // newly-selected match in the dropdown is.  When entering via space, however,

components/omnibox/browser/omnibox_popup_model.cc

@@ -48,8 +48,7 @@ bool OmniboxPopupModel::Selection::operator<(const Selection& b) const {
 }
 
 bool OmniboxPopupModel::Selection::IsChangeToKeyword(Selection from) const {
-  return (state == KEYWORD_MODE || state == FOCUSED_BUTTON_KEYWORD) &&
-         !(from.state == KEYWORD_MODE || from.state == FOCUSED_BUTTON_KEYWORD);
+  return state == KEYWORD_MODE && from.state != KEYWORD_MODE;
 }
 
 bool OmniboxPopupModel::Selection::IsButtonFocused() const {
@@ -350,18 +349,13 @@ OmniboxPopupModel::GetAllAvailableSelectionsSorted(Direction direction,
 
     all_states.push_back(NORMAL);
 
-    if (OmniboxFieldTrial::IsKeywordSearchButtonEnabled()) {
-      // The keyword button experiment makes things simple. We no longer access
-      // keyword mode via pressing tab in this case.
-      all_states.push_back(FOCUSED_BUTTON_KEYWORD);
-    } else {
-      // Keyword mode is only accessible by Tabbing forward.
-      if (direction == kForward) {
-        if (step == kStateOrLine) {
-          all_states.push_back(KEYWORD_MODE);
-        }
-      }
+    // Keyword mode is accessible if the keyword search button is enabled. If
+    // not, then keyword mode is only accessible by tabbing forward.
+    if (OmniboxFieldTrial::IsKeywordSearchButtonEnabled() ||
+        (direction == kForward && step == kStateOrLine)) {
+      all_states.push_back(KEYWORD_MODE);
     }
+
     all_states.push_back(FOCUSED_BUTTON_TAB_SWITCH);
     if (OmniboxFieldTrial::IsSuggestionButtonRowEnabled())
       all_states.push_back(FOCUSED_BUTTON_PEDAL);
@@ -458,16 +452,19 @@ OmniboxPopupModel::Selection OmniboxPopupModel::StepSelection(
     Direction direction,
     Step step) {
   // This block steps the popup model, with special consideration
-  // for existing keyword logic in the edit model, where AcceptKeyword and
-  // ClearKeyword must be called before changing the selected line.
+  // for existing keyword logic in the edit model, where ClearKeyword must be
+  // called before changing the selected line.
+  // AcceptKeyword should be called after changing the selected line so we don't
+  // accept keyword on the wrong suggestion when stepping backwards.
   const auto old_selection = selection();
   const auto new_selection = GetNextSelection(direction, step);
-  if (new_selection.IsChangeToKeyword(old_selection)) {
-    edit_model()->AcceptKeyword(metrics::OmniboxEventProto::TAB);
-  } else if (old_selection.IsChangeToKeyword(new_selection)) {
+  if (old_selection.IsChangeToKeyword(new_selection)) {
     edit_model()->ClearKeyword();
   }
   SetSelection(new_selection);
+  if (new_selection.IsChangeToKeyword(old_selection)) {
+    edit_model()->AcceptKeyword(metrics::OmniboxEventProto::TAB);
+  }
   return selection_;
 }
 
@@ -500,11 +497,7 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const {
     case NORMAL:
       return true;
     case KEYWORD_MODE:
-      return !OmniboxFieldTrial::IsKeywordSearchButtonEnabled() &&
-             match.associated_keyword != nullptr;
-    case FOCUSED_BUTTON_KEYWORD:
-      return OmniboxFieldTrial::IsKeywordSearchButtonEnabled() &&
-             match.associated_keyword != nullptr;
+      return match.associated_keyword != nullptr;
     case FOCUSED_BUTTON_TAB_SWITCH:
       // Buttons are suppressed for matches with an associated keyword, unless
       // dedicated button row is enabled.
@@ -553,21 +546,6 @@ bool OmniboxPopupModel::TriggerSelectionAction(Selection selection,
           pref_service_, match.suggestion_group_id.value(), new_value);
       break;
     }
-    case FOCUSED_BUTTON_KEYWORD:
-      // TODO(yoangela): Merge logic with mouse/gesture events in
-      // OmniboxSuggestionButtonRowView::ButtonPressed - This case currently
-      // is only reached by the call in OmniboxViewViews::HandleKeyEvent.
-      if (edit_model()->is_keyword_hint()) {
-        // TODO(yoangela): Rename once tab to keyword search is deprecated
-        // Accept/ClearKeyword() has special conditions to handle searches
-        // initiated by pressing Tab. Since tab+enter on this button behaves
-        // more similar to a Tab than a Keyboard shortcut, it's easier
-        // for now to treat it as a Tab entry method, otherwise the
-        // autocomplete results will reset, leaving us in an unknown state.
-        edit_model()->AcceptKeyword(metrics::OmniboxEventProto::TAB);
-      }
-      break;
-
     case FOCUSED_BUTTON_TAB_SWITCH:
       DCHECK(timestamp != base::TimeTicks());
       edit_model()->OpenMatch(match, WindowOpenDisposition::SWITCH_TO_TAB,
@@ -621,7 +599,7 @@ base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection(
         additional_message_id = IDS_ACC_TAB_SWITCH_SUFFIX;
         available_actions_count++;
       }
-      if (IsControlPresentOnMatch(Selection(line, FOCUSED_BUTTON_KEYWORD))) {
+      if (IsControlPresentOnMatch(Selection(line, KEYWORD_MODE))) {
         additional_message_id = IDS_ACC_KEYWORD_SUFFIX;
         available_actions_count++;
       }
@@ -630,7 +608,7 @@ base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection(
             match.pedal->GetLabelStrings().id_accessibility_suffix;
         available_actions_count++;
       }
-      DCHECK_EQ(LINE_STATE_MAX_VALUE, 7);
+      DCHECK_EQ(LINE_STATE_MAX_VALUE, 6);
       if (available_actions_count > 1)
         additional_message_id = IDS_ACC_MULTIPLE_ACTIONS_SUFFIX;
 
@@ -639,11 +617,11 @@ base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection(
       break;
     }
     case KEYWORD_MODE:
+      if (OmniboxFieldTrial::IsKeywordSearchButtonEnabled()) {
+        additional_message_id = IDS_ACC_KEYWORD_BUTTON;
+      }
       // TODO(tommycli): Investigate whether the accessibility messaging for
-      // Keyword mode belongs here.
-      break;
-    case FOCUSED_BUTTON_KEYWORD:
-      additional_message_id = IDS_ACC_KEYWORD_BUTTON;
+      // (non-button row) Keyword mode belongs here.
       break;
     case FOCUSED_BUTTON_TAB_SWITCH:
       additional_message_id = IDS_ACC_TAB_SWITCH_BUTTON_FOCUSED_PREFIX;

components/omnibox/browser/omnibox_popup_model.h

@@ -61,26 +61,22 @@ class OmniboxPopupModel {
     // NORMAL means the row is focused, and Enter key navigates to the match.
     NORMAL = 1,
 
-    // FOCUSED_BUTTON_KEYWORD is used when the keyword button is in focus, not
-    // actually in Keyword Mode. This is currently only used if deciated button
-    // row is enabled
-    FOCUSED_BUTTON_KEYWORD = 2,
-
-    // KEYWORD_MODE state means actually in keyword mode, as distinct from the
-    // FOCUSED_BUTTON_KEYWORD state, which is only for button focus.
-    KEYWORD_MODE = 3,
+    // KEYWORD_MODE state is used when in Keyword mode.  If the keyword search
+    // button is enabled, keyword mode is entered when the keyword button is
+    // focused.
+    KEYWORD_MODE = 2,
 
     // FOCUSED_BUTTON_TAB_SWITCH state means the Switch Tab button is focused.
     // Pressing enter will switch to the tab match.
-    FOCUSED_BUTTON_TAB_SWITCH = 4,
+    FOCUSED_BUTTON_TAB_SWITCH = 3,
 
     // FOCUSED_BUTTON_PEDAL state means a Pedal button is in focus. This is
     // currently only used when dedicated button row and pedals are enabled.
-    FOCUSED_BUTTON_PEDAL = 5,
+    FOCUSED_BUTTON_PEDAL = 4,
 
     // FOCUSED_BUTTON_REMOVE_SUGGESTION state means the Remove Suggestion (X)
     // button is focused. Pressing enter will attempt to remove this suggestion.
-    FOCUSED_BUTTON_REMOVE_SUGGESTION = 6,
+    FOCUSED_BUTTON_REMOVE_SUGGESTION = 5,
 
     // Whenever new line state is added, accessibility label for current
     // selection should be revisited

components/omnibox/browser/omnibox_popup_model_unittest.cc

@@ -483,9 +483,10 @@ TEST_F(OmniboxPopupModelSuggestionButtonRowTest,
   }
   // Make match index 1 deletable to verify we can step to that.
   matches[1].deletable = true;
+  // Make match index 2 only have an associated keyword to verify we can step
+  // backwards into keyword search mode.
   matches[2].associated_keyword =
       std::make_unique<AutocompleteMatch>(matches.back());
-  matches[2].has_tab_match = true;
   // Make match index 3 have an associated keyword, tab match, and deletable to
   // verify keyword mode doesn't override tab match and remove suggestion
   // buttons (as it does with button row disabled)
@@ -523,10 +524,9 @@ TEST_F(OmniboxPopupModelSuggestionButtonRowTest,
            Selection(1, OmniboxPopupModel::NORMAL),
            Selection(1, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION),
            Selection(2, OmniboxPopupModel::NORMAL),
-           Selection(2, OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD),
-           Selection(2, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
+           Selection(2, OmniboxPopupModel::KEYWORD_MODE),
            Selection(3, OmniboxPopupModel::NORMAL),
-           Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD),
+           Selection(3, OmniboxPopupModel::KEYWORD_MODE),
            Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
            Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION),
            Selection(4, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
@@ -545,10 +545,9 @@ TEST_F(OmniboxPopupModelSuggestionButtonRowTest,
            Selection(4, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
            Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION),
            Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
-           Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD),
+           Selection(3, OmniboxPopupModel::KEYWORD_MODE),
            Selection(3, OmniboxPopupModel::NORMAL),
-           Selection(2, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
-           Selection(2, OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD),
+           Selection(2, OmniboxPopupModel::KEYWORD_MODE),
            Selection(2, OmniboxPopupModel::NORMAL),
            Selection(1, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION),
            Selection(1, OmniboxPopupModel::NORMAL),