[omnibox] Add kSelection accessibility event for Header collapse button

This CL standardizes the kSelection accessibility event for all the
omnibox popup auxiliary buttons.

This also gives the Header collapse button this accessibility event.

Bug: 1078183, 1052522
Change-Id: Ia56b2e15b47c299bb55f74aa16bb1648b47b322c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240298Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777555}

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

@@ -392,10 +392,16 @@ void OmniboxPopupContentsView::UpdatePopupAppearance() {
 }
 
 void OmniboxPopupContentsView::ProvideButtonFocusHint(size_t line) {
+  DCHECK(model()->selection().IsButtonFocused());
   if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup))
     return;  // TODO(tommycli): Not implemented yet for WebUI.
 
-  result_view_at(line)->ProvideButtonFocusHint();
+  views::View* active_button = static_cast<OmniboxRowView*>(children()[line])
+                                   ->GetActiveAuxiliaryButtonForAccessibility();
+  // TODO(tommycli): |active_button| can sometimes be nullptr, because the
+  // suggestion button row is not completely implemented.
+  if (active_button)
+    FireAXEventsForNewActiveDescendant(active_button);
 }
 
 void OmniboxPopupContentsView::OnMatchIconUpdated(size_t match_index) {

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

@@ -247,6 +247,7 @@ bool OmniboxResultView::IsMatchSelected() const {
          popup_contents_view_->model()->selected_line_state() !=
              OmniboxPopupModel::HEADER_BUTTON_FOCUSED;
 }
+
 views::Button* OmniboxResultView::GetSecondaryButton() {
   if (suggestion_tab_switch_button_->GetVisible())
     return suggestion_tab_switch_button_;
@@ -483,17 +484,6 @@ void OmniboxResultView::OnThemeChanged() {
   ApplyThemeAndRefreshIcons(true);
 }
 
-void OmniboxResultView::ProvideButtonFocusHint() {
-  if (suggestion_tab_switch_button_->GetVisible()) {
-    suggestion_tab_switch_button_->ProvideFocusHint();
-    // TODO(tommycli) Why isn't this using the same AX code as for the
-    // remove suggestion button below?
-  } else if (remove_suggestion_button_->GetVisible()) {
-    popup_contents_view_->FireAXEventsForNewActiveDescendant(
-        remove_suggestion_button_);
-  }
-}
-
 void OmniboxResultView::RemoveSuggestion() const {
   popup_contents_view_->model()->TryDeletingLine(model_index_);
 }

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

@@ -93,9 +93,6 @@ class OmniboxResultView : public views::View,
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
-  // Called to indicate tab switch button has been focused.
-  void ProvideButtonFocusHint();
-
   // Removes the shown |match_| from history, if possible.
   void RemoveSuggestion() const;
 

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

@@ -48,11 +48,13 @@ class OmniboxRowView::HeaderView : public views::View,
     header_text_->SetFontList(font);
     header_text_->SetEnabledColor(gfx::kGoogleGrey700);
 
-    hide_button_ = AddChildView(views::CreateVectorToggleImageButton(this));
-    views::InstallCircleHighlightPathGenerator(hide_button_);
+    header_toggle_button_ =
+        AddChildView(views::CreateVectorToggleImageButton(this));
+    views::InstallCircleHighlightPathGenerator(header_toggle_button_);
 
-    hide_button_focus_ring_ = views::FocusRing::Install(hide_button_);
-    hide_button_focus_ring_->SetHasFocusPredicate([&](View* view) {
+    header_toggle_button_focus_ring_ =
+        views::FocusRing::Install(header_toggle_button_);
+    header_toggle_button_focus_ring_->SetHasFocusPredicate([&](View* view) {
       return view->GetVisible() &&
              row_view_->popup_model_->selection() ==
                  OmniboxPopupModel::Selection(
@@ -111,7 +113,7 @@ class OmniboxRowView::HeaderView : public views::View,
 
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override {
-    DCHECK_EQ(sender, hide_button_);
+    DCHECK_EQ(sender, header_toggle_button_);
     row_view_->popup_model_->TriggerSelectionAction(
         OmniboxPopupModel::Selection(row_view_->line_,
                                      OmniboxPopupModel::HEADER_BUTTON_FOCUSED));
@@ -131,7 +133,7 @@ class OmniboxRowView::HeaderView : public views::View,
 
     SkColor icon_color = GetOmniboxColor(GetThemeProvider(),
                                          OmniboxPart::RESULTS_ICON, part_state);
-    hide_button_->set_ink_drop_base_color(icon_color);
+    header_toggle_button_->set_ink_drop_base_color(icon_color);
 
     int dip_size = GetLayoutConstant(LOCATION_BAR_ICON_SIZE);
     const gfx::ImageSkia arrow_down =
@@ -142,16 +144,17 @@ class OmniboxRowView::HeaderView : public views::View,
 
     // The "untoggled" button state corresponds with the group being shown.
     // The button's action is therefore to Hide the group, when clicked.
-    hide_button_->SetImage(views::Button::STATE_NORMAL, arrow_up);
-    hide_button_->SetTooltipText(
+    header_toggle_button_->SetImage(views::Button::STATE_NORMAL, arrow_up);
+    header_toggle_button_->SetTooltipText(
         l10n_util::GetStringUTF16(IDS_TOOLTIP_HEADER_HIDE_SUGGESTIONS_BUTTON));
 
     // The "toggled" button state corresponds with the group being hidden.
     // The button's action is therefore to Show the group, when clicked.
-    hide_button_->SetToggledImage(views::Button::STATE_NORMAL, &arrow_down);
-    hide_button_->SetToggledTooltipText(
+    header_toggle_button_->SetToggledImage(views::Button::STATE_NORMAL,
+                                           &arrow_down);
+    header_toggle_button_->SetToggledTooltipText(
         l10n_util::GetStringUTF16(IDS_TOOLTIP_HEADER_SHOW_SUGGESTIONS_BUTTON));
-    hide_button_focus_ring_->SchedulePaint();
+    header_toggle_button_focus_ring_->SchedulePaint();
 
     // It's a little hokey that we're stealing the logic for the background
     // color from OmniboxResultView. If we start doing this is more than just
@@ -159,11 +162,13 @@ class OmniboxRowView::HeaderView : public views::View,
     SetBackground(OmniboxResultView::GetPopupCellBackground(this, part_state));
   }
 
+  views::Button* header_toggle_button() const { return header_toggle_button_; }
+
  private:
   // Updates the hide button's toggle state.
   void UpdateHideButtonToggleState() {
     DCHECK(row_view_->pref_service_);
-    hide_button_->SetToggled(omnibox::IsSuggestionGroupIdHidden(
+    header_toggle_button_->SetToggled(omnibox::IsSuggestionGroupIdHidden(
         row_view_->pref_service_, suggestion_group_id_));
   }
 
@@ -175,8 +180,8 @@ class OmniboxRowView::HeaderView : public views::View,
   views::Label* header_text_;
 
   // The button used to toggle hiding suggestions with this header.
-  views::ToggleImageButton* hide_button_;
-  views::FocusRing* hide_button_focus_ring_ = nullptr;
+  views::ToggleImageButton* header_toggle_button_;
+  views::FocusRing* header_toggle_button_focus_ring_ = nullptr;
 
   // The group ID associated with this header.
   int suggestion_group_id_ = 0;
@@ -220,6 +225,18 @@ void OmniboxRowView::OnSelectionStateChanged() {
     header_view_->UpdateUI();
 }
 
+views::View* OmniboxRowView::GetActiveAuxiliaryButtonForAccessibility() const {
+  DCHECK(popup_model_->selection().IsButtonFocused());
+  if (popup_model_->selected_line_state() ==
+      OmniboxPopupModel::HEADER_BUTTON_FOCUSED) {
+    return header_view_->header_toggle_button();
+  }
+
+  // TODO(tommycli): This needs to be updated to properly support the
+  // suggestion button row. The name would need to be updated too.
+  return result_view_->GetSecondaryButton();
+}
+
 gfx::Insets OmniboxRowView::GetInsets() const {
   // A visible header means this is the start of a new section. Give the section
   // that just ended an extra 4dp of padding. https://crbug.com/1076646

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

@@ -39,6 +39,10 @@ class OmniboxRowView : public views::View {
   // Invoked when the model's selection state has changed.
   void OnSelectionStateChanged();
 
+  // Fetches the active descendant button for accessibility purposes.
+  // Returns nullptr if no descendant auxiliary button is active.
+  views::View* GetActiveAuxiliaryButtonForAccessibility() const;
+
   // views::View:
   gfx::Insets GetInsets() const override;
 

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

@@ -105,10 +105,6 @@ void OmniboxTabSwitchButton::ProvideWidthHint(int parent_width) {
   SetPreferredSize({preferred_width, GetPreferredSize().height()});
 }
 
-void OmniboxTabSwitchButton::ProvideFocusHint() {
-  NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
-}
-
 bool OmniboxTabSwitchButton::IsSelected() const {
   // Is this result selected and is button selected?
   return result_view_->IsMatchSelected() &&

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

@@ -33,9 +33,6 @@ class OmniboxTabSwitchButton : public views::MdTextButton {
   // so the button can adjust its size or even presence.
   void ProvideWidthHint(int width);
 
-  // Called to indicate button has been focused.
-  void ProvideFocusHint();
-
  private:
   // Consults the parent views to see if the button is selected.
   bool IsSelected() const;

components/omnibox/browser/omnibox_popup_model.cc

@@ -181,6 +181,8 @@ void OmniboxPopupModel::SetSelection(Selection new_selection,
   if (selection_.IsButtonFocused()) {
     old_focused_url_ = match.destination_url;
     edit_model_->SetAccessibilityLabel(match);
+    // TODO(tommycli): Fold the focus hint into view_->OnSelectionChanged().
+    // Caveat: We must update the accessibility label before notifying the View.
     view_->ProvideButtonFocusHint(selected_line());
   }