[omnibox] Enable tabbing to the Remove Suggestion button

This enables tabbing to the Remove Suggestion X button, and pressing
Enter or Space to trigger it.

This also shows the X for the selected row, in addition to any
mouse-hovered rows.

This also shows a blue focus ring for the X, when it gets the keyboard
focus. That may not be the final UX design.

Bug: 1205
Change-Id: I6e2e175841d293d81306cec86a11d60a77edfd6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1931581Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718882}

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

@@ -194,6 +194,10 @@ void OmniboxPopupContentsView::UnselectButton() {
   model_->SetSelectedLineState(OmniboxPopupModel::NORMAL);
 }
 
+OmniboxResultView* OmniboxPopupContentsView::result_view_at(size_t i) {
+  return static_cast<OmniboxResultView*>(children()[i]);
+}
+
 bool OmniboxPopupContentsView::InExplicitExperimentalKeywordMode() {
   return model_->edit_model()->InExplicitExperimentalKeywordMode();
 }
@@ -393,10 +397,6 @@ size_t OmniboxPopupContentsView::GetIndexForPoint(const gfx::Point& point) {
   return OmniboxPopupModel::kNoMatch;
 }
 
-OmniboxResultView* OmniboxPopupContentsView::result_view_at(size_t i) {
-  return static_cast<OmniboxResultView*>(children()[i]);
-}
-
 void OmniboxPopupContentsView::GetAccessibleNodeData(
     ui::AXNodeData* node_data) {
   node_data->role = ax::mojom::Role::kListBox;

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

@@ -60,6 +60,9 @@ class OmniboxPopupContentsView : public views::View, public OmniboxPopupView {
   // Called by the active result view to inform model (due to mouse event).
   void UnselectButton();
 
+  // Gets the OmniboxResultView for match |i|.
+  OmniboxResultView* result_view_at(size_t i);
+
   // Returns whether we're in experimental keyword mode and the input gives
   // sufficient confidence that the user wants keyword mode.
   bool InExplicitExperimentalKeywordMode();
@@ -97,8 +100,6 @@ class OmniboxPopupContentsView : public views::View, public OmniboxPopupView {
   // the specified point.
   size_t GetIndexForPoint(const gfx::Point& point);
 
-  OmniboxResultView* result_view_at(size_t i);
-
   LocationBarView* location_bar_view() { return location_bar_view_; }
 
   // views::View:

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

@@ -37,6 +37,7 @@
 #include "ui/events/event.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/controls/button/image_button_factory.h"
+#include "ui/views/controls/focus_ring.h"
 #include "ui/views/controls/highlight_path_generator.h"
 #if defined(OS_WIN)
 #include "base/win/atl.h"
@@ -77,6 +78,12 @@ OmniboxResultView::OmniboxResultView(
   views::SetImageFromVectorIcon(remove_suggestion_button_,
                                 vector_icons::kCloseRoundedIcon,
                                 GetColor(OmniboxPart::RESULTS_ICON));
+  remove_suggestion_focus_ring_ =
+      views::FocusRing::Install(remove_suggestion_button_);
+  remove_suggestion_focus_ring_->SetHasFocusPredicate([&](View* view) {
+    return view->GetVisible() && IsSelected() &&
+           popup_contents_view_->IsButtonSelected();
+  });
 
   keyword_view_ = AddChildView(std::make_unique<OmniboxMatchCellView>(this));
   keyword_view_->icon()->EnableCanvasFlippingForRTLUI(true);
@@ -129,8 +136,8 @@ void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
                                           keyword_match->description_class);
   }
 
-  InvalidateLayout();
   Invalidate();
+  InvalidateLayout();
 }
 
 void OmniboxResultView::ShowKeyword(bool show_keyword) {
@@ -156,6 +163,8 @@ void OmniboxResultView::Invalidate(bool force_reapply_styles) {
   keyword_view_->separator()->ApplyTextColor(OmniboxPart::RESULTS_TEXT_DIMMED);
   if (suggestion_tab_switch_button_->GetVisible())
     suggestion_tab_switch_button_->UpdateBackground();
+  if (remove_suggestion_button_->GetVisible())
+    remove_suggestion_focus_ring_->SchedulePaint();
 
   // Recreate the icons in case the color needs to change.
   // Note: if this is an extension icon or favicon then this can be done in
@@ -186,6 +195,22 @@ void OmniboxResultView::Invalidate(bool force_reapply_styles) {
     keyword_view_->description()->ApplyTextColor(
         OmniboxPart::RESULTS_TEXT_DIMMED);
   }
+
+  // Refresh the Remove button.
+  {
+    bool old_visibility = remove_suggestion_button_->GetVisible();
+    bool new_visibility = match_.SupportsDeletion() &&
+                          !match_.associated_keyword &&
+                          !match_.ShouldShowTabMatchButton() &&
+                          base::FeatureList::IsEnabled(
+                              omnibox::kOmniboxSuggestionTransparencyOptions) &&
+                          (IsSelected() || IsMouseHovered());
+
+    remove_suggestion_button_->SetVisible(new_visibility);
+
+    if (old_visibility != new_visibility)
+      InvalidateLayout();
+  }
 }
 
 void OmniboxResultView::OnSelected() {
@@ -209,6 +234,25 @@ bool OmniboxResultView::IsSelected() const {
   return popup_contents_view_->IsSelectedIndex(model_index_);
 }
 
+views::Button* OmniboxResultView::GetSecondaryButton() {
+  if (suggestion_tab_switch_button_->GetVisible())
+    return suggestion_tab_switch_button_;
+
+  if (remove_suggestion_button_->GetVisible())
+    return remove_suggestion_button_;
+
+  return nullptr;
+}
+
+bool OmniboxResultView::MaybeTriggerSecondaryButton(const ui::Event& event) {
+  views::Button* button = GetSecondaryButton();
+  if (!button)
+    return false;
+
+  ButtonPressed(button, event);
+  return true;
+}
+
 OmniboxPartState OmniboxResultView::GetThemeState() const {
   if (IsSelected())
     return OmniboxPartState::SELECTED;
@@ -280,11 +324,6 @@ void OmniboxResultView::Layout() {
   // Add buttons from right to left, shrinking the suggestion width as we go.
   // To avoid clutter, don't show either button for matches with keyword.
   // TODO(tommycli): We should probably use a layout manager here.
-  remove_suggestion_button_->SetVisible(
-      match_.SupportsDeletion() && !match_.associated_keyword &&
-      IsMouseHovered() && !match_.ShouldShowTabMatchButton() &&
-      base::FeatureList::IsEnabled(
-          omnibox::kOmniboxSuggestionTransparencyOptions));
   if (remove_suggestion_button_->GetVisible()) {
     const gfx::Size button_size = remove_suggestion_button_->GetPreferredSize();
     suggestion_width -=
@@ -460,7 +499,6 @@ void OmniboxResultView::SetHovered(bool hovered) {
   if (is_hovered_ != hovered) {
     is_hovered_ = hovered;
     Invalidate();
-    InvalidateLayout();
   }
 }
 

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

@@ -34,6 +34,11 @@ namespace gfx {
 class Image;
 }
 
+namespace views {
+class Button;
+class FocusRing;
+}  // namespace views
+
 namespace ui {
 class ThemeProvider;
 }
@@ -65,6 +70,13 @@ class OmniboxResultView : public views::View,
   // Whether |this| matches the model's selected index.
   bool IsSelected() const;
 
+  // Returns the visible (and keyboard-focusable) secondary button, or nullptr
+  // if none exists for this suggestion.
+  views::Button* GetSecondaryButton();
+
+  // If this view has a secondary button, triggers the action and returns true.
+  bool MaybeTriggerSecondaryButton(const ui::Event& event);
+
   OmniboxPartState GetThemeState() const;
 
   // Notification that the match icon has changed and schedules a repaint.
@@ -154,6 +166,7 @@ class OmniboxResultView : public views::View,
 
   // The "X" button at the end of the match cell, used to remove suggestions.
   views::ImageButton* remove_suggestion_button_;
+  std::unique_ptr<views::FocusRing> remove_suggestion_focus_ring_;
 
   base::WeakPtrFactory<OmniboxResultView> weak_factory_{this};
 

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

@@ -26,6 +26,7 @@
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h"
+#include "chrome/browser/ui/views/omnibox/omnibox_result_view.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/feature_engagement/buildflags.h"
 #include "components/omnibox/browser/autocomplete_input.h"
@@ -651,7 +652,7 @@ bool OmniboxViewViews::HandleEarlyTabActions(const ui::KeyEvent& event) {
   // If tabbing forwards (shift is not pressed) and suggestion button is not
   // selected, select it.
   if (!event.IsShiftDown()) {
-    if (MaybeFocusTabButton())
+    if (MaybeFocusSecondaryButton())
       return true;
   }
 
@@ -659,7 +660,7 @@ bool OmniboxViewViews::HandleEarlyTabActions(const ui::KeyEvent& event) {
   // the tab switch button.
   if (event.IsShiftDown()) {
     // If tab switch button is focused, unfocus it.
-    if (MaybeUnfocusTabButton())
+    if (MaybeUnfocusSecondaryButton())
       return true;
   }
 
@@ -667,8 +668,7 @@ bool OmniboxViewViews::HandleEarlyTabActions(const ui::KeyEvent& event) {
   model()->OnUpOrDownKeyPressed(event.IsShiftDown() ? -1 : 1);
   // If we shift-tabbed (and actually moved) to a suggestion with a tab
   // switch button, select it.
-  if (event.IsShiftDown() &&
-      model()->popup_model()->SelectedLineHasTabMatch()) {
+  if (event.IsShiftDown() && GetSecondaryButtonForSelectedLine()) {
     model()->popup_model()->SetSelectedLineState(
         OmniboxPopupModel::BUTTON_FOCUSED);
   }
@@ -696,9 +696,16 @@ bool OmniboxViewViews::TextAndUIDirectionMatch() const {
           base::i18n::RIGHT_TO_LEFT) == base::i18n::IsRTL();
 }
 
-bool OmniboxViewViews::SelectedSuggestionHasTabMatch() const {
-  return model()->popup_model() &&  // Can be null in tests.
-         model()->popup_model()->SelectedLineHasTabMatch();
+views::Button* OmniboxViewViews::GetSecondaryButtonForSelectedLine() const {
+  OmniboxPopupModel* popup_model = model()->popup_model();
+  if (!popup_model)
+    return nullptr;
+
+  size_t selected_line = popup_model->selected_line();
+  if (selected_line == OmniboxPopupModel::kNoMatch)
+    return nullptr;
+
+  return popup_view_->result_view_at(selected_line)->GetSecondaryButton();
 }
 
 bool OmniboxViewViews::DirectionAwareSelectionAtEnd() const {
@@ -707,8 +714,8 @@ bool OmniboxViewViews::DirectionAwareSelectionAtEnd() const {
   return TextAndUIDirectionMatch() ? SelectionAtEnd() : SelectionAtBeginning();
 }
 
-bool OmniboxViewViews::MaybeFocusTabButton() {
-  if (SelectedSuggestionHasTabMatch() &&
+bool OmniboxViewViews::MaybeFocusSecondaryButton() {
+  if (GetSecondaryButtonForSelectedLine() &&
       model()->popup_model()->selected_line_state() ==
           OmniboxPopupModel::NORMAL) {
     model()->popup_model()->SetSelectedLineState(
@@ -718,8 +725,8 @@ bool OmniboxViewViews::MaybeFocusTabButton() {
   return false;
 }
 
-bool OmniboxViewViews::MaybeUnfocusTabButton() {
-  if (SelectedSuggestionHasTabMatch() &&
+bool OmniboxViewViews::MaybeUnfocusSecondaryButton() {
+  if (GetSecondaryButtonForSelectedLine() &&
       model()->popup_model()->selected_line_state() ==
           OmniboxPopupModel::BUTTON_FOCUSED) {
     model()->popup_model()->SetSelectedLineState(OmniboxPopupModel::NORMAL);
@@ -728,13 +735,21 @@ bool OmniboxViewViews::MaybeUnfocusTabButton() {
   return false;
 }
 
-bool OmniboxViewViews::MaybeSwitchToTab(const ui::KeyEvent& event) {
+bool OmniboxViewViews::MaybeTriggerSecondaryButton(const ui::KeyEvent& event) {
   if (model()->popup_model()->selected_line_state() !=
       OmniboxPopupModel::BUTTON_FOCUSED)
     return false;
-  popup_view_->OpenMatch(WindowOpenDisposition::SWITCH_TO_TAB,
-                         event.time_stamp());
-  return true;
+
+  OmniboxPopupModel* popup_model = model()->popup_model();
+  if (!popup_model)
+    return false;
+
+  size_t selected_line = popup_model->selected_line();
+  if (selected_line == OmniboxPopupModel::kNoMatch)
+    return false;
+
+  return popup_view_->result_view_at(selected_line)
+      ->MaybeTriggerSecondaryButton(event);
 }
 
 void OmniboxViewViews::SetWindowTextAndCaretPos(const base::string16& text,
@@ -1559,24 +1574,24 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
   const bool command = event.IsCommandDown();
   switch (event.key_code()) {
     case ui::VKEY_RETURN:
-      if (!MaybeSwitchToTab(event)) {
-        if (alt || (shift && command)) {
-          model()->AcceptInput(WindowOpenDisposition::NEW_FOREGROUND_TAB,
-                               event.time_stamp());
-        } else if (command) {
-          model()->AcceptInput(WindowOpenDisposition::NEW_BACKGROUND_TAB,
-                               event.time_stamp());
-        } else if (shift) {
-          model()->AcceptInput(WindowOpenDisposition::NEW_WINDOW,
+      if (MaybeTriggerSecondaryButton(event)) {
+        return true;
+      } else if (alt || (shift && command)) {
+        model()->AcceptInput(WindowOpenDisposition::NEW_FOREGROUND_TAB,
+                             event.time_stamp());
+      } else if (command) {
+        model()->AcceptInput(WindowOpenDisposition::NEW_BACKGROUND_TAB,
+                             event.time_stamp());
+      } else if (shift) {
+        model()->AcceptInput(WindowOpenDisposition::NEW_WINDOW,
+                             event.time_stamp());
+      } else {
+        if (model()->popup_model()->SelectedLineIsTabSwitchSuggestion()) {
+          model()->AcceptInput(WindowOpenDisposition::SWITCH_TO_TAB,
                                event.time_stamp());
         } else {
-          if (model()->popup_model()->SelectedLineIsTabSwitchSuggestion()) {
-            model()->AcceptInput(WindowOpenDisposition::SWITCH_TO_TAB,
-                                 event.time_stamp());
-          } else {
-            model()->AcceptInput(WindowOpenDisposition::CURRENT_TAB,
-                                 event.time_stamp());
-          }
+          model()->AcceptInput(WindowOpenDisposition::CURRENT_TAB,
+                               event.time_stamp());
         }
       }
       return true;
@@ -1648,10 +1663,10 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
           model()->AcceptKeyword(OmniboxEventProto::SELECT_SUGGESTION);
           OnAfterPossibleChange(true);
           return true;
-        } else if (MaybeFocusTabButton()) {
+        } else if (MaybeFocusSecondaryButton()) {
           return true;
         }
-      } else if (MaybeUnfocusTabButton()) {
+      } else if (MaybeUnfocusSecondaryButton()) {
         return true;
       }
       break;
@@ -1704,7 +1719,7 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
 
     case ui::VKEY_SPACE:
       if (!control && !alt && !shift && SelectionAtEnd() &&
-          MaybeSwitchToTab(event))
+          MaybeTriggerSecondaryButton(event))
         return true;
       break;
 

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

@@ -40,12 +40,16 @@ class WebContents;
 
 namespace gfx {
 class RenderText;
-}
+}  // namespace gfx
 
 namespace ui {
 class OSExchangeData;
 }  // namespace ui
 
+namespace views {
+class Button;
+}  // namespace views
+
 // Views-implementation of OmniboxView.
 class OmniboxViewViews : public OmniboxView,
                          public views::Textfield,
@@ -201,21 +205,22 @@ class OmniboxViewViews : public OmniboxView,
   // flip.)
   bool TextAndUIDirectionMatch() const;
 
-  // Helper function for MaybeFocusTabButton() and MaybeUnfocusTabButton().
-  bool SelectedSuggestionHasTabMatch() const;
+  // Gets the secondary button (like the tab switch or remove suggestion)
+  // for the selected line. Returns nullptr if there is no secondary button.
+  views::Button* GetSecondaryButtonForSelectedLine() const;
 
   // Like SelectionAtEnd(), but accounts for RTL.
   bool DirectionAwareSelectionAtEnd() const;
 
-  // Attempts to either focus or unfocus the tab switch button (tests if all
+  // Attempts to either focus or unfocus the secondary button (tests if all
   // conditions are met and makes necessary subroutine call) and returns
   // whether it succeeded.
-  bool MaybeFocusTabButton();
-  bool MaybeUnfocusTabButton();
+  bool MaybeFocusSecondaryButton();
+  bool MaybeUnfocusSecondaryButton();
 
-  // If the tab switch button is focused, switches to the relevant tab.  Returns
-  // whether the switch was attempted.
-  bool MaybeSwitchToTab(const ui::KeyEvent& event);
+  // If the Secondary button for the current suggestion is focused, clicks it
+  // and returns true.
+  bool MaybeTriggerSecondaryButton(const ui::KeyEvent& event);
 
   // OmniboxView:
   void SetCaretPos(size_t caret_pos) override;

components/omnibox/browser/omnibox_popup_model.cc

@@ -185,12 +185,8 @@ void OmniboxPopupModel::SetSelectedLineState(LineState state) {
   GURL current_destination(match.destination_url);
   DCHECK((state != KEYWORD) || match.associated_keyword.get());
 
-  if (state == BUTTON_FOCUSED) {
-    // TODO(orinj): If in-suggestion Pedals are kept, refactor a bit
-    // so that button presence doesn't always assume tab switching use case.
-    DCHECK(match.has_tab_match || match.pedal);
+  if (state == BUTTON_FOCUSED)
     old_focused_url_ = current_destination;
-  }
 
   selected_line_state_ = state;
   view_->InvalidateLine(selected_line_);
@@ -323,11 +319,6 @@ gfx::Image OmniboxPopupModel::GetMatchIcon(const AutocompleteMatch& match,
 }
 #endif  // !defined(OS_ANDROID) && !defined(OS_IOS)
 
-bool OmniboxPopupModel::SelectedLineHasTabMatch() {
-  return selected_line_ != kNoMatch &&
-         result().match_at(selected_line_).ShouldShowTabMatchButton();
-}
-
 bool OmniboxPopupModel::SelectedLineIsTabSwitchSuggestion() {
   return selected_line_ != kNoMatch &&
          result().match_at(selected_line_).IsTabSwitchSuggestion();

components/omnibox/browser/omnibox_popup_model.h

@@ -142,10 +142,6 @@ class OmniboxPopupModel {
                           SkColor vector_icon_color);
 #endif
 
-  // Helper function to see if the current selection specifically has a
-  // tab switch button.
-  bool SelectedLineHasTabMatch();
-
   // Helper function to see if current selection is a tab switch suggestion
   // dedicated row.
   bool SelectedLineIsTabSwitchSuggestion();