[omnibox] Add tests for dedicated row and pedals a11y

Added tests for keyword button, pedal button, remove suggestion button,
multiple buttons for one suggestion (tab switch and keyword button).
Cleaned up tests.

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: I0c7a7a9cee0622cce2a24f6ea58c73a56ac996bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2352826
Commit-Queue: Olesia Marukhno <olesiamarukhno@google.com>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799723}

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));
   views::InstallCircleHighlightPathGenerator(remove_suggestion_button_);
   remove_suggestion_button_->SetTooltipText(
       l10n_util::GetStringUTF16(IDS_OMNIBOX_REMOVE_SUGGESTION));
@@ -273,6 +293,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/autocomplete_controller.h

@@ -177,10 +177,6 @@ class AutocompleteController : public AutocompleteProviderListener,
   FRIEND_TEST_ALL_PREFIXES(AutocompleteProviderTest,
                            RedundantKeywordsIgnoredInResult);
   FRIEND_TEST_ALL_PREFIXES(AutocompleteProviderTest, UpdateAssistedQueryStats);
-  FRIEND_TEST_ALL_PREFIXES(OmniboxPopupContentsViewTest,
-                           EmitAccessibilityEvents);
-  FRIEND_TEST_ALL_PREFIXES(OmniboxPopupContentsViewTest,
-                           EmitAccessibilityEventsOnButtonFocusHint);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewTest, DoesNotUpdateAutocompleteOnBlur);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsTest, CloseOmniboxPopupOnTextDrag);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsTest, FriendlyAccessibleLabel);
@@ -205,6 +201,7 @@ class AutocompleteController : public AutocompleteProviderListener,
                            PopupStepSelectionWithButtonRowAndKeywordButton);
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupContentsViewTest,
                            EmitSelectedChildrenChangedAccessibilityEvent);
+  friend class OmniboxPopupContentsViewTest;
 
   // Updates |result_| to reflect the current provider state and fires
   // notifications.  If |regenerate_result| then we clear the result

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.

testing/buildbot/filters/lacros.browser_tests.filter

@@ -121,8 +121,10 @@
 -NtpExtensionBubbleViewBrowserTest.InvokeUi_ntp_override
 -NtpExtensionBubbleViewBrowserTest.TestControlledNewTabPageMessageBubble
 -NtpExtensionBubbleViewBrowserTest.TestControlledNewTabPageMessageBubbleLearnMore
--OmniboxPopupContentsViewTest.ClickOmnibox
--OmniboxPopupContentsViewTest.PopupMatchesLocationBarBackground
+-All/OmniboxPopupContentsViewTest.ClickOmnibox/0
+-All/OmniboxPopupContentsViewTest.ClickOmnibox/1
+-All/OmniboxPopupContentsViewTest.PopupMatchesLocationBarBackground/0
+-All/OmniboxPopupContentsViewTest.PopupMatchesLocationBarBackground/1
 -OutOfProcessPPAPITest.FlashClipboard
 -OutOfProcessPPAPITest.NetAddress
 -OutOfProcessPPAPITest.Printing

testing/buildbot/filters/ozone-linux.wayland_browser_tests.filter

@@ -13,8 +13,10 @@
 -ExtensionDialogTest.TextInputViaKeyEvent
 -ExternalProtocolDialogBrowserTest.TestFocus
 -FolderUploadConfirmationViewTest.InitiallyFocusesCancel
--OmniboxPopupContentsViewTest.ClickOmnibox
--OmniboxPopupContentsViewTest.PopupMatchesLocationBarBackground
+-All/OmniboxPopupContentsViewTest.ClickOmnibox/0
+-All/OmniboxPopupContentsViewTest.ClickOmnibox/1
+-All/OmniboxPopupContentsViewTest.PopupMatchesLocationBarBackground/0
+-All/OmniboxPopupContentsViewTest.PopupMatchesLocationBarBackground/1
 -OutOfProcessPPAPITest.FlashClipboard
 -PPAPINaClNewlibTest.TrueTypeFont
 -PPAPINaClPNaClNonSfiTest.TrueTypeFont

testing/buildbot/filters/ozone-linux.x11_browser_tests.filter

@@ -15,7 +15,8 @@
 -AutofillProviderBrowserTestWithSkipFlagOn.LabelTagChangeImpactFormComparing
 -ExecuteScriptApiTest/DestructiveScriptTest.SynchronousRemoval/0
 -ExtensionInstallDialogViewTest.InstallButtonDelay
--OmniboxPopupContentsViewTest.ClickOmnibox
+-All/OmniboxPopupContentsViewTest.ClickOmnibox/0
+-All/OmniboxPopupContentsViewTest.ClickOmnibox/1
 -PageInfoBubbleViewBrowserTest.FocusDoesNotReturnToContentsOnReloadPrompt
 -PaymentRequestCreditCardEditorTest.EditingExpiredCard
 -PaymentRequestShippingAddressEditorTest.FocusFirstField_Name