[omnibox] Make Enter/Space trigger Suggestion Button Row buttons

There was no case to handle KeyEvents for the Button Row buttons
previously. This was added so Enter and Space will trigger the button's
actions.

This CL moves some of the business logic to PopupModel, merging the
handling of Mouse/ButtonPressed events and Key events

Keyword entry method was also changed to Tab rather than Keyboard shortcut.

Bug: 1091351
Change-Id: I2818e47860d8993bbb99836c40d60197b8ef7e6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2233780
Commit-Queue: Angela Yoeurng <yoangela@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777957}

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

@@ -90,19 +90,24 @@ void OmniboxSuggestionButtonRowView::OnStyleRefresh() {
 
 void OmniboxSuggestionButtonRowView::ButtonPressed(views::Button* button,
                                                    const ui::Event& event) {
+  OmniboxPopupModel* popup_model = popup_contents_view_->model();
+  if (!popup_model)
+    return;
+
   if (button == tab_switch_button_) {
-    popup_contents_view_->OpenMatch(
-        model_index_, WindowOpenDisposition::SWITCH_TO_TAB, event.time_stamp());
+    popup_model->TriggerSelectionAction(
+        OmniboxPopupModel::Selection(
+            model_index_, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
+        event.time_stamp());
   } else if (button == keyword_button_) {
+    // TODO(yoangela): Port to PopupModel and merge with keyEvent
     // TODO(orinj): Clear out existing suggestions, particularly this one, as
     // once we AcceptKeyword, we are really in a new scope state and holding
     // onto old suggestions is confusing and error prone. Without this check,
     // a second click of the button violates assumptions in |AcceptKeyword|.
     if (model()->edit_model()->is_keyword_hint()) {
       auto method = metrics::OmniboxEventProto::INVALID;
-      if (event.IsKeyEvent()) {
-        method = metrics::OmniboxEventProto::KEYBOARD_SHORTCUT;
-      } else if (event.IsMouseEvent()) {
+      if (event.IsMouseEvent()) {
         method = metrics::OmniboxEventProto::CLICK_HINT_VIEW;
       } else if (event.IsGestureEvent()) {
         method = metrics::OmniboxEventProto::TAP_HINT_VIEW;
@@ -111,9 +116,10 @@ void OmniboxSuggestionButtonRowView::ButtonPressed(views::Button* button,
       model()->edit_model()->AcceptKeyword(method);
     }
   } else if (button == pedal_button_) {
-    DCHECK(match().pedal);
-    // Pedal action intent means we execute the match instead of opening it.
-    model()->edit_model()->ExecutePedal(match(), event.time_stamp());
+    popup_model->TriggerSelectionAction(
+        OmniboxPopupModel::Selection(model_index_,
+                                     OmniboxPopupModel::FOCUSED_BUTTON_PEDAL),
+        event.time_stamp());
   }
 }
 

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

@@ -1725,8 +1725,8 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
   switch (event.key_code()) {
     case ui::VKEY_RETURN: {
       OmniboxPopupModel* popup_model = model()->popup_model();
-      if (popup_model &&
-          popup_model->TriggerSelectionAction(popup_model->selection())) {
+      if (popup_model && popup_model->TriggerSelectionAction(
+                             popup_model->selection(), event.time_stamp())) {
         return true;
       } else if (MaybeTriggerSecondaryButton(event)) {
         return true;
@@ -1881,8 +1881,8 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
     case ui::VKEY_SPACE: {
       if (!control && !alt && !shift && SelectionAtEnd()) {
         OmniboxPopupModel* popup_model = model()->popup_model();
-        if (popup_model &&
-            popup_model->TriggerSelectionAction(popup_model->selection())) {
+        if (popup_model && popup_model->TriggerSelectionAction(
+                               popup_model->selection(), event.time_stamp())) {
           return true;
         }
 

components/omnibox/browser/omnibox_popup_model.cc

@@ -554,21 +554,54 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const {
   return false;
 }
 
-bool OmniboxPopupModel::TriggerSelectionAction(Selection selection) {
+bool OmniboxPopupModel::TriggerSelectionAction(Selection selection,
+                                               base::TimeTicks timestamp) {
   // Early exit for the kNoMatch case. Also exits if the calling UI passes in
   // an invalid |selection|.
   if (selection.line >= result().size())
     return false;
 
   auto& match = result().match_at(selection.line);
-  if (selection.state == HEADER_BUTTON_FOCUSED &&
-      match.suggestion_group_id.has_value()) {
-    omnibox::ToggleSuggestionGroupIdVisibility(
-        pref_service_, match.suggestion_group_id.value());
-    return true;
+  switch (selection.state) {
+    case HEADER_BUTTON_FOCUSED:
+      DCHECK(match.suggestion_group_id.has_value());
+      omnibox::ToggleSuggestionGroupIdVisibility(
+          pref_service_, match.suggestion_group_id.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()->AcceptInput(WindowOpenDisposition::SWITCH_TO_TAB,
+                                timestamp);
+      break;
+
+    case FOCUSED_BUTTON_PEDAL:
+      DCHECK(timestamp != base::TimeTicks());
+      DCHECK(match.pedal);
+      edit_model()->ExecutePedal(match, timestamp);
+      break;
+
+    default:
+      // Behavior is not yet supported, return false.
+      return false;
   }
 
-  return false;
+  return true;
 }
 
 base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection(

components/omnibox/browser/omnibox_popup_model.h

@@ -242,7 +242,10 @@ class OmniboxPopupModel {
   // Triggers the action on |selection| (usually an auxiliary button).
   // If the popup model supports the action and performs it, this returns true.
   // This can't handle all actions currently, and returns false in those cases.
-  bool TriggerSelectionAction(Selection selection);
+  // The timestamp parameter is currently only used by FOCUSED_BUTTON_TAB_SWITCH
+  // and FOCUSED_BUTTON_PEDAL, so is set by default for other use cases.
+  bool TriggerSelectionAction(Selection selection,
+                              base::TimeTicks timestamp = base::TimeTicks());
 
   // This returns the accessibility label for current selection. This is an
   // extended version of AutocompleteMatchType::ToAccessibilityLabel() which