Fix omnibox suggestion button a11y events

Ensure value change, selection and active descendants are fired
appropriately as omnibox suggestion button focus hint is set/cleared.

Bug: 1109780
Change-Id: I9ac05eff463eca1af29cdf9c9a7a816c21e84440
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2337234
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795013}

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

@@ -268,7 +268,10 @@ void OmniboxPopupContentsView::OnSelectionChanged(
     return;
   }
 
-  if (old_selection.line != OmniboxPopupModel::kNoMatch) {
+  // Do not invalidate the same line twice, in order to avoid redundant
+  // accessibility events.
+  if (old_selection.line != OmniboxPopupModel::kNoMatch &&
+      old_selection.line != new_selection.line) {
     InvalidateLine(old_selection.line);
   }
 
@@ -465,10 +468,15 @@ void OmniboxPopupContentsView::OnGestureEvent(ui::GestureEvent* event) {
 
 void OmniboxPopupContentsView::FireAXEventsForNewActiveDescendant(
     View* descendant_view) {
-  if (descendant_view)
+  if (descendant_view) {
     descendant_view->NotifyAccessibilityEvent(ax::mojom::Event::kSelection,
                                               true);
-  NotifyAccessibilityEvent(ax::mojom::Event::kActiveDescendantChanged, true);
+  }
+  // Selected children changed is fired on the popup.
+  NotifyAccessibilityEvent(ax::mojom::Event::kSelectedChildrenChanged, true);
+  // Active descendant changed is fired on the focused text field.
+  omnibox_view_->NotifyAccessibilityEvent(
+      ax::mojom::Event::kActiveDescendantChanged, true);
 }
 
 void OmniboxPopupContentsView::OnWidgetBoundsChanged(

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

@@ -98,13 +98,23 @@ class TestAXEventObserver : public views::AXEventObserver {
       return;
     ui::AXNodeData node_data;
     view->GetAccessibleNodeData(&node_data);
+    ax::mojom::Role role = node_data.role;
     if (event_type == ax::mojom::Event::kTextChanged &&
-        node_data.role == ax::mojom::Role::kListBoxOption)
+        role == ax::mojom::Role::kListBoxOption) {
       text_changed_on_listboxoption_count_++;
-    else if (event_type == ax::mojom::Event::kSelectedChildrenChanged)
+    } else if (event_type == ax::mojom::Event::kSelectedChildrenChanged &&
+               role == ax::mojom::Role::kListBox) {
       selected_children_changed_count_++;
-    else if (event_type == ax::mojom::Event::kActiveDescendantChanged)
+    } else if (event_type == ax::mojom::Event::kSelection &&
+               role == ax::mojom::Role::kListBoxOption) {
+      selection_changed_count_++;
+    } else if (event_type == ax::mojom::Event::kValueChanged &&
+               role == ax::mojom::Role::kTextField) {
+      value_changed_count_++;
+    } else if (event_type == ax::mojom::Event::kActiveDescendantChanged &&
+               role == ax::mojom::Role::kTextField) {
       active_descendant_changed_count_++;
+    }
   }
 
   int text_changed_on_listboxoption_count() {
@@ -113,6 +123,8 @@ class TestAXEventObserver : public views::AXEventObserver {
   int selected_children_changed_count() {
     return selected_children_changed_count_;
   }
+  int selection_changed_count() { return selection_changed_count_; }
+  int value_changed_count() { return value_changed_count_; }
   int active_descendant_changed_count() {
     return active_descendant_changed_count_;
   }
@@ -120,6 +132,8 @@ class TestAXEventObserver : public views::AXEventObserver {
  private:
   int text_changed_on_listboxoption_count_ = 0;
   int selected_children_changed_count_ = 0;
+  int selection_changed_count_ = 0;
+  int value_changed_count_ = 0;
   int active_descendant_changed_count_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(TestAXEventObserver);
@@ -365,8 +379,7 @@ IN_PROC_BROWSER_TEST_F(OmniboxPopupContentsViewTest,
   EXPECT_EQ(color_before_focus, omnibox_view()->GetBackgroundColor());
 }
 
-IN_PROC_BROWSER_TEST_F(OmniboxPopupContentsViewTest,
-                       EmitTextChangedAccessibilityEvent) {
+IN_PROC_BROWSER_TEST_F(OmniboxPopupContentsViewTest, EmitAccessibilityEvents) {
   // Creation and population of the popup should not result in a text/name
   // change accessibility event.
   TestAXEventObserver observer;
@@ -389,15 +402,62 @@ IN_PROC_BROWSER_TEST_F(OmniboxPopupContentsViewTest,
   edit_model()->StartAutocomplete(false, false);
   popup_view()->UpdatePopupAppearance();
   EXPECT_EQ(observer.text_changed_on_listboxoption_count(), 0);
+  EXPECT_EQ(observer.selected_children_changed_count(), 1);
+  EXPECT_EQ(observer.selection_changed_count(), 1);
+  EXPECT_EQ(observer.active_descendant_changed_count(), 1);
+  EXPECT_EQ(observer.value_changed_count(), 2);
 
   // Each time the selection changes, we should have a text/name change event.
   // This makes it possible for screen readers to have the updated match content
   // when they are notified the selection changed.
   popup_view()->model()->SetSelection(OmniboxPopupModel::Selection(1));
   EXPECT_EQ(observer.text_changed_on_listboxoption_count(), 1);
+  EXPECT_EQ(observer.selected_children_changed_count(), 2);
+  EXPECT_EQ(observer.selection_changed_count(), 2);
+  EXPECT_EQ(observer.active_descendant_changed_count(), 2);
+  EXPECT_EQ(observer.value_changed_count(), 3);
 
   popup_view()->model()->SetSelection(OmniboxPopupModel::Selection(2));
   EXPECT_EQ(observer.text_changed_on_listboxoption_count(), 2);
+  EXPECT_EQ(observer.selected_children_changed_count(), 3);
+  EXPECT_EQ(observer.selection_changed_count(), 3);
+  EXPECT_EQ(observer.active_descendant_changed_count(), 3);
+  EXPECT_EQ(observer.value_changed_count(), 4);
+}
+
+IN_PROC_BROWSER_TEST_F(OmniboxPopupContentsViewTest,
+                       EmitAccessibilityEventsOnButtonFocusHint) {
+  TestAXEventObserver observer;
+  CreatePopupForTestQuery();
+  ACMatches matches;
+  AutocompleteMatch match(nullptr, 500, false,
+                          AutocompleteMatchType::HISTORY_TITLE);
+  AutocompleteController* controller = popup_model()->autocomplete_controller();
+  match.contents = base::ASCIIToUTF16("https://foobar.com");
+  match.has_tab_match = true;
+  matches.push_back(match);
+  controller->result_.AppendMatches(controller->input_, matches);
+  popup_view()->UpdatePopupAppearance();
+
+  popup_view()->model()->SetSelection(OmniboxPopupModel::Selection(1));
+  EXPECT_EQ(observer.selected_children_changed_count(), 2);
+  EXPECT_EQ(observer.selection_changed_count(), 2);
+  EXPECT_EQ(observer.active_descendant_changed_count(), 2);
+  EXPECT_EQ(observer.value_changed_count(), 2);
+
+  popup_view()->model()->SetSelection(OmniboxPopupModel::Selection(
+      1, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH));
+  EXPECT_EQ(observer.selected_children_changed_count(), 3);
+  EXPECT_EQ(observer.selection_changed_count(), 3);
+  EXPECT_EQ(observer.active_descendant_changed_count(), 3);
+  EXPECT_EQ(observer.value_changed_count(), 3);
+
+  popup_view()->model()->SetSelection(
+      OmniboxPopupModel::Selection(1, OmniboxPopupModel::NORMAL));
+  EXPECT_EQ(observer.selected_children_changed_count(), 4);
+  EXPECT_EQ(observer.selection_changed_count(), 4);
+  EXPECT_EQ(observer.active_descendant_changed_count(), 4);
+  EXPECT_EQ(observer.value_changed_count(), 4);
 }
 
 IN_PROC_BROWSER_TEST_F(OmniboxPopupContentsViewTest,
@@ -434,16 +494,22 @@ IN_PROC_BROWSER_TEST_F(OmniboxPopupContentsViewTest,
   // Lets check that arrowing up and down emits the event.
   TestAXEventObserver observer;
   EXPECT_EQ(observer.selected_children_changed_count(), 0);
+  EXPECT_EQ(observer.selection_changed_count(), 0);
+  EXPECT_EQ(observer.value_changed_count(), 0);
   EXPECT_EQ(observer.active_descendant_changed_count(), 0);
 
   // This is equiverlent of the user arrowing down in the omnibox.
   popup_view()->model()->SetSelection(OmniboxPopupModel::Selection(1));
   EXPECT_EQ(observer.selected_children_changed_count(), 1);
+  EXPECT_EQ(observer.selection_changed_count(), 1);
+  EXPECT_EQ(observer.value_changed_count(), 1);
   EXPECT_EQ(observer.active_descendant_changed_count(), 1);
 
   // This is equivalent of the user arrowing up in the omnibox.
   popup_view()->model()->SetSelection(OmniboxPopupModel::Selection(0));
   EXPECT_EQ(observer.selected_children_changed_count(), 2);
+  EXPECT_EQ(observer.selection_changed_count(), 2);
+  EXPECT_EQ(observer.value_changed_count(), 2);
   EXPECT_EQ(observer.active_descendant_changed_count(), 2);
 
   // TODO(accessibility) Test that closing the popup fires an activedescendant

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

@@ -230,18 +230,23 @@ void OmniboxResultView::OnSelectionStateChanged() {
     // any cached values get updated prior to the selection change.
     EmitTextChangedAccessiblityEvent();
 
-    // Send accessibility event on the popup box that its selection has changed.
-    EmitSelectedChildrenChangedAccessibilityEvent();
+    auto selection_state = popup_contents_view_->model()->selection().state;
 
     // The text is also accessible via text/value change events in the omnibox
     // but this selection event allows the screen reader to get more details
     // about the list and the user's position within it.
-    popup_contents_view_->FireAXEventsForNewActiveDescendant(this);
+    // Limit which selection states fire the events, in order to avoid duplicate
+    // events. Specifically, OmniboxPopupContentsView::ProvideButtonFocusHint()
+    // already fires the correct events when the user tabs to an attached button
+    // in the current row.
+    if (selection_state == OmniboxPopupModel::FOCUSED_BUTTON_HEADER ||
+        selection_state == OmniboxPopupModel::NORMAL) {
+      popup_contents_view_->FireAXEventsForNewActiveDescendant(this);
+    }
 
     // TODO(orinj): Eventually the deep digging in this class should get
     //  replaced with a single local point of access to all selection state.
-    ShowKeyword(popup_contents_view_->model()->selection().state ==
-                OmniboxPopupModel::KEYWORD_MODE);
+    ShowKeyword(selection_state == OmniboxPopupModel::KEYWORD_MODE);
   } else {
     ShowKeyword(false);
   }
@@ -497,11 +502,6 @@ void OmniboxResultView::EmitTextChangedAccessiblityEvent() {
   }
 }
 
-void OmniboxResultView::EmitSelectedChildrenChangedAccessibilityEvent() {
-  popup_contents_view_->NotifyAccessibilityEvent(
-      ax::mojom::Event::kSelectedChildrenChanged, true);
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxResultView, private:
 

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

@@ -92,7 +92,6 @@ class OmniboxResultView : public views::View,
 
   // Helper to emit accessibility events (may only emit if conditions are met).
   void EmitTextChangedAccessiblityEvent();
-  void EmitSelectedChildrenChangedAccessibilityEvent();
 
   // views::View:
   void Layout() override;

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

@@ -916,7 +916,11 @@ void OmniboxViewViews::OnTemporaryTextMaybeChanged(
     bool notify_text_changed) {
   if (save_original_selection)
     saved_temporary_selection_ = GetRenderText()->GetAllSelections();
-  SetAccessibilityLabel(display_text, match);
+
+  // SetWindowTextAndCaretPos will fire the acesssibility notification,
+  // so do not also generate redundant notification here.
+  SetAccessibilityLabel(display_text, match, false);
+
   SetWindowTextAndCaretPos(display_text, display_text.length(), false,
                            notify_text_changed);
   SetAdditionalText(match.fill_into_edit_additional_text);
@@ -951,16 +955,14 @@ void OmniboxViewViews::OnInlineAutocompleteTextCleared() {
 
 void OmniboxViewViews::OnRevertTemporaryText(const base::string16& display_text,
                                              const AutocompleteMatch& match) {
-  SetAccessibilityLabel(display_text, match);
-  SetSelectedRanges(saved_temporary_selection_);
-
   // We got here because the user hit the Escape key. We explicitly don't call
   // TextChanged(), since OmniboxPopupModel::ResetToDefaultMatch() has already
   // been called by now, and it would've called TextChanged() if it was
   // warranted.
   // However, it's important to notify accessibility that the value has changed,
   // otherwise the screen reader will use the old accessibility label text.
-  NotifyAccessibilityEvent(ax::mojom::Event::kValueChanged, true);
+  SetAccessibilityLabel(display_text, match, true);
+  SetSelectedRanges(saved_temporary_selection_);
 }
 
 void OmniboxViewViews::ClearAccessibilityLabel() {
@@ -972,7 +974,8 @@ void OmniboxViewViews::ClearAccessibilityLabel() {
 }
 
 void OmniboxViewViews::SetAccessibilityLabel(const base::string16& display_text,
-                                             const AutocompleteMatch& match) {
+                                             const AutocompleteMatch& match,
+                                             bool notify_text_changed) {
   if (model()->popup_model()->selected_line() == OmniboxPopupModel::kNoMatch) {
     // If nothing is selected in the popup, we are in the no-default-match edge
     // case, and |match| is a synthetically generated match. In that case,
@@ -986,6 +989,9 @@ void OmniboxViewViews::SetAccessibilityLabel(const base::string16& display_text,
             display_text, &friendly_suggestion_text_prefix_length_);
   }
 
+  if (notify_text_changed)
+    NotifyAccessibilityEvent(ax::mojom::Event::kValueChanged, true);
+
 #if defined(OS_MAC)
   // On macOS, the only way to get VoiceOver to speak the friendly suggestion
   // text (for example, "how to open a pdf, search suggestion, 4 of 8") is

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

@@ -348,7 +348,8 @@ class OmniboxViewViews : public OmniboxView,
   void ClearAccessibilityLabel();
 
   void SetAccessibilityLabel(const base::string16& display_text,
-                             const AutocompleteMatch& match) override;
+                             const AutocompleteMatch& match,
+                             bool notify_text_changed) override;
 
   // Returns true if the user text was updated with the full URL (without
   // steady-state elisions).  |gesture| is the user gesture causing unelision.

components/omnibox/browser/autocomplete_controller.h

@@ -176,7 +176,9 @@ class AutocompleteController : public AutocompleteProviderListener,
                            RedundantKeywordsIgnoredInResult);
   FRIEND_TEST_ALL_PREFIXES(AutocompleteProviderTest, UpdateAssistedQueryStats);
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupContentsViewTest,
-                           EmitTextChangedAccessibilityEvent);
+                           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);

components/omnibox/browser/omnibox_edit_model.cc

@@ -1509,7 +1509,7 @@ const char OmniboxEditModel::kCutOrCopyAllTextHistogram[] =
     "Omnibox.CutOrCopyAllText";
 
 void OmniboxEditModel::SetAccessibilityLabel(const AutocompleteMatch& match) {
-  view_->SetAccessibilityLabel(view_->GetText(), match);
+  view_->SetAccessibilityLabel(view_->GetText(), match, true);
 }
 
 bool OmniboxEditModel::PopupIsOpen() const {

components/omnibox/browser/omnibox_view.h

@@ -165,7 +165,8 @@ class OmniboxView {
 
   // Updates the accessibility state by enunciating any on-focus text.
   virtual void SetAccessibilityLabel(const base::string16& display_text,
-                                     const AutocompleteMatch& match) {}
+                                     const AutocompleteMatch& match,
+                                     bool notify_text_changed) {}
 
   // Called when the temporary text in the model may have changed.
   // |display_text| is the new text to show; |match_type| is the type of the