[omnibox] Fix accessibility bugs for dedicated row and pedals

Fixed a11y for remove suggestion button and buttons in dedicated row
(set role to be list box option and set correct accessible name).
Fixed selection event not being triggered for dedicated row buttons
(GetSecondaryButton wasn't returning buttons from dedicated row).
Fixed value changed event triggering extra time when switching selection
from button to button.

Bug: 1104264
Change-Id: If5fc4692ac4e86f50e1a1d1b754feec795b76c10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2366758
Commit-Queue: Olesia Marukhno <olesiamarukhno@google.com>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800300}

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

@@ -45,6 +45,26 @@
 #include "base/win/atl.h"
 #endif
 
+namespace {
+
+class OmniboxRemoveSuggestionButton : public views::ImageButton {
+ public:
+  explicit OmniboxRemoveSuggestionButton(views::ButtonListener* listener)
+      : ImageButton(listener) {
+    views::ConfigureVectorImageButton(this);
+  }
+
+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
+    node_data->SetName(
+        l10n_util::GetStringUTF16(IDS_ACC_REMOVE_SUGGESTION_BUTTON));
+    // Although this appears visually as a button, expose as a list box option
+    // so that it matches the other options within its list box container.
+    node_data->role = ax::mojom::Role::kListBoxOption;
+  }
+};
+
+}  // namespace
+
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxResultView, public:
 
@@ -72,7 +92,7 @@ OmniboxResultView::OmniboxResultView(
   // TODO(tommycli): Make sure we announce the Shift+Delete capability in the
   // accessibility node data for removable suggestions.
   remove_suggestion_button_ =
-      AddChildView(views::CreateVectorImageButton(this));
+      AddChildView(std::make_unique<OmniboxRemoveSuggestionButton>(this));
   // The remove suggestion button may receive mouse enter/exit events with very
   // quick mouse movements. Monitor the button to update our state.
   update_on_mouse_enter_exit_.emplace(this, remove_suggestion_button_);
@@ -276,6 +296,10 @@ views::Button* OmniboxResultView::GetSecondaryButton() {
   if (remove_suggestion_button_->GetVisible())
     return remove_suggestion_button_;
 
+  if (OmniboxFieldTrial::IsSuggestionButtonRowEnabled()) {
+    return button_row_->GetActiveButton();
+  }
+
   return nullptr;
 }
 

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

@@ -17,9 +17,11 @@
 #include "components/strings/grit/components_strings.h"
 #include "components/vector_icons/vector_icons.h"
 #include "third_party/metrics_proto/omnibox_event.pb.h"
+#include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/animation/ink_drop_highlight.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/button/label_button_border.h"
@@ -33,8 +35,12 @@ class OmniboxSuggestionRowButton : public views::MdTextButton {
   OmniboxSuggestionRowButton(views::ButtonListener* listener,
                              const base::string16& text,
                              const gfx::VectorIcon& icon,
-                             const views::FocusRing::ViewPredicate& predicate)
-      : MdTextButton(listener, text, CONTEXT_OMNIBOX_PRIMARY), icon_(icon) {
+                             OmniboxPopupContentsView* popup_contents_view,
+                             OmniboxPopupModel::Selection selection)
+      : MdTextButton(listener, text, CONTEXT_OMNIBOX_PRIMARY),
+        icon_(icon),
+        popup_contents_view_(popup_contents_view),
+        selection_(selection) {
     views::InstallPillHighlightPathGenerator(this);
     SetImageLabelSpacing(ChromeLayoutProvider::Get()->GetDistanceMetric(
         DISTANCE_RELATED_LABEL_HORIZONTAL_LIST));
@@ -44,7 +50,10 @@ class OmniboxSuggestionRowButton : public views::MdTextButton {
                     GetLayoutConstant(LOCATION_BAR_ICON_SIZE));
 
     set_ink_drop_highlight_opacity(CalculateInkDropHighlightOpacity());
-    focus_ring()->SetHasFocusPredicate(predicate);
+    focus_ring()->SetHasFocusPredicate([=](View* view) {
+      return view->GetVisible() &&
+             popup_contents_view_->model()->selection() == selection_;
+    });
   }
 
   OmniboxSuggestionRowButton(const OmniboxSuggestionRowButton&) = delete;
@@ -59,6 +68,8 @@ class OmniboxSuggestionRowButton : public views::MdTextButton {
 
   void OnStyleRefresh() { focus_ring()->SchedulePaint(); }
 
+  OmniboxPopupModel::Selection selection() { return selection_; }
+
   std::unique_ptr<views::InkDropHighlight> CreateInkDropHighlight()
       const override {
     // MdTextButton uses custom colors when creating ink drop highlight.
@@ -77,8 +88,17 @@ class OmniboxSuggestionRowButton : public views::MdTextButton {
                  icon_, GetLayoutConstant(LOCATION_BAR_ICON_SIZE), color));
   }
 
+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
+    node_data->SetName(GetAccessibleName());
+    // Although this appears visually as a button, expose as a list box option
+    // so that it matches the other options within its list box container.
+    node_data->role = ax::mojom::Role::kListBoxOption;
+  }
+
  private:
   const gfx::VectorIcon& icon_;
+  OmniboxPopupContentsView* popup_contents_view_;
+  OmniboxPopupModel::Selection selection_;
 
   float CalculateInkDropHighlightOpacity() {
     // Ink drop highlight opacity is result of mixing a layer with hovered
@@ -109,30 +129,27 @@ OmniboxSuggestionButtonRowView::OmniboxSuggestionButtonRowView(
           gfx::Insets(0, ChromeLayoutProvider::Get()->GetDistanceMetric(
                              views::DISTANCE_RELATED_BUTTON_HORIZONTAL)));
 
-  const auto make_predicate = [=](auto state) {
-    return [=](View* view) {
-      return view->GetVisible() &&
-             model()->selection() ==
-                 OmniboxPopupModel::Selection(model_index_, state);
-    };
-  };
-
   // For all of these buttons, the visibility set from UpdateFromModel().
   // The Keyword and Pedal buttons also get their text from there, since the
   // text depends on the actual match. That shouldn't produce a flicker, because
   // it's called directly from OmniboxResultView::SetMatch(). If this flickers,
   // then so does everything else in the result view.
   keyword_button_ = AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
-      this, base::string16(), vector_icons::kSearchIcon,
-      make_predicate(OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD)));
+      this, base::string16(), vector_icons::kSearchIcon, popup_contents_view_,
+      OmniboxPopupModel::Selection(model_index_,
+                                   OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD)));
   tab_switch_button_ =
       AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
           this, l10n_util::GetStringUTF16(IDS_OMNIBOX_TAB_SUGGEST_HINT),
-          omnibox::kSwitchIcon,
-          make_predicate(OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH)));
+          omnibox::kSwitchIcon, popup_contents_view_,
+          OmniboxPopupModel::Selection(
+              model_index_, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH)));
+  tab_switch_button_->SetAccessibleName(
+      l10n_util::GetStringUTF16(IDS_ACC_TAB_SWITCH_BUTTON));
   pedal_button_ = AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
-      this, base::string16(), omnibox::kProductIcon,
-      make_predicate(OmniboxPopupModel::FOCUSED_BUTTON_PEDAL)));
+      this, base::string16(), omnibox::kProductIcon, popup_contents_view_,
+      OmniboxPopupModel::Selection(model_index_,
+                                   OmniboxPopupModel::FOCUSED_BUTTON_PEDAL)));
 }
 
 OmniboxSuggestionButtonRowView::~OmniboxSuggestionButtonRowView() = default;
@@ -150,14 +167,17 @@ void OmniboxSuggestionButtonRowView::UpdateFromModel() {
     const auto names = SelectedKeywordView::GetKeywordLabelNames(
         keyword, edit_model->client()->GetTemplateURLService());
     keyword_button_->SetText(names.full_name);
+    keyword_button_->SetAccessibleName(
+        l10n_util::GetStringFUTF16(IDS_ACC_KEYWORD_BUTTON, names.short_name));
   }
 
   SetPillButtonVisibility(pedal_button_,
                           OmniboxPopupModel::FOCUSED_BUTTON_PEDAL);
   if (pedal_button_->GetVisible()) {
-    pedal_button_->SetText(match().pedal->GetLabelStrings().hint);
-    pedal_button_->SetTooltipText(
-        match().pedal->GetLabelStrings().suggestion_contents);
+    const auto pedal_strings = match().pedal->GetLabelStrings();
+    pedal_button_->SetText(pedal_strings.hint);
+    pedal_button_->SetTooltipText(pedal_strings.suggestion_contents);
+    pedal_button_->SetAccessibleName(pedal_strings.accessibility_hint);
   }
 
   SetPillButtonVisibility(tab_switch_button_,
@@ -210,6 +230,28 @@ void OmniboxSuggestionButtonRowView::ButtonPressed(views::Button* button,
   }
 }
 
+views::Button* OmniboxSuggestionButtonRowView::GetActiveButton() const {
+  std::vector<OmniboxSuggestionRowButton*> visible_buttons;
+  if (keyword_button_->GetVisible())
+    visible_buttons.push_back(keyword_button_);
+  if (tab_switch_button_->GetVisible())
+    visible_buttons.push_back(tab_switch_button_);
+  if (pedal_button_->GetVisible())
+    visible_buttons.push_back(pedal_button_);
+
+  if (visible_buttons.empty())
+    return nullptr;
+
+  // Find first visible button that matches model selection.
+  auto selected_button =
+      std::find_if(visible_buttons.begin(), visible_buttons.end(),
+                   [=](OmniboxSuggestionRowButton* button) {
+                     return model()->selection() == button->selection();
+                   });
+  return selected_button == visible_buttons.end() ? visible_buttons.front()
+                                                  : *selected_button;
+}
+
 const OmniboxPopupModel* OmniboxSuggestionButtonRowView::model() const {
   return popup_contents_view_->model();
 }

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

@@ -32,6 +32,8 @@ class OmniboxSuggestionButtonRowView : public views::View,
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
+  views::Button* GetActiveButton() const;
+
  private:
   // Get the popup model from the view.
   const OmniboxPopupModel* model() const;

components/omnibox/browser/omnibox_popup_model.cc

@@ -181,6 +181,7 @@ void OmniboxPopupModel::SetSelection(Selection new_selection,
                                     base::string16());
   } else if (old_selection.line != selection_.line ||
              (old_selection.IsButtonFocused() &&
+              !new_selection.IsButtonFocused() &&
               new_selection.state != KEYWORD_MODE)) {
     // Otherwise, only update the edit model for line number changes, or
     // when the old selection was a button and we're not entering keyword mode.