Correct a11y events and focus handling in suggested password popup

AX-Relnotes: screen reader support for Use Suggested Password popup
Bug: 786147
Change-Id: Ic59b7c0d01da7c7258ada0761dfdbb4204bed962
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300184
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789529}

chrome/browser/ui/views/autofill/autofill_popup_base_view.cc

@@ -24,6 +24,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/native_theme/native_theme.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/border.h"
 #include "ui/views/bubble/bubble_border.h"
 #include "ui/views/focus/focus_manager.h"
@@ -142,6 +143,37 @@ void AutofillPopupBaseView::DoHide() {
   }
 }
 
+void AutofillPopupBaseView::VisibilityChanged(View* starting_from,
+                                              bool is_visible) {
+  if (!is_visible) {
+    if (is_ax_menu_start_event_fired_) {
+      // Fire menu end event.
+      // The menu start event is delayed until the user
+      // navigates into the menu, otherwise some screen readers will ignore
+      // any focus events outside of the menu, including a focus event on
+      // the form control itself.
+      NotifyAccessibilityEvent(ax::mojom::Event::kMenuEnd, true);
+      GetViewAccessibility().EndPopupFocusOverride();
+    }
+    is_ax_menu_start_event_fired_ = false;
+  }
+}
+
+void AutofillPopupBaseView::NotifyAXSelection(View* selected_view) {
+  DCHECK(selected_view);
+  if (!is_ax_menu_start_event_fired_) {
+    // Fire the menu start event once, right before the first item is selected.
+    // By firing these and the matching kMenuEnd events, we are telling screen
+    // readers that the focus is only changing temporarily, and the screen
+    // reader will restore the focus back to the appropriate textfield when the
+    // menu closes.
+    NotifyAccessibilityEvent(ax::mojom::Event::kMenuStart, true);
+    is_ax_menu_start_event_fired_ = true;
+  }
+  selected_view->GetViewAccessibility().SetPopupFocusOverride();
+  selected_view->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
+}
+
 void AutofillPopupBaseView::OnWidgetBoundsChanged(views::Widget* widget,
                                                   const gfx::Rect& new_bounds) {
   DCHECK(widget == parent_widget_ || widget == GetWidget());

chrome/browser/ui/views/autofill/autofill_popup_base_view.h

@@ -36,6 +36,12 @@ class AutofillPopupBaseView : public views::WidgetDelegateView,
 
   static int GetCornerRadius();
 
+  // views::View:
+  void VisibilityChanged(View* starting_from, bool is_visible) override;
+
+  // Notify accessibility that an item has been selected.
+  void NotifyAXSelection(View*);
+
   // Get colors used throughout various popup UIs, based on the current native
   // theme.
   SkColor GetBackgroundColor();
@@ -105,6 +111,9 @@ class AutofillPopupBaseView : public views::WidgetDelegateView,
   // The time when the popup was shown.
   base::Time show_time_;
 
+  // Ensures that the menu start event is not fired redundantly.
+  bool is_ax_menu_start_event_fired_ = false;
+
   base::WeakPtrFactory<AutofillPopupBaseView> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(AutofillPopupBaseView);

chrome/browser/ui/views/autofill/autofill_popup_view_native_views.cc

@@ -948,14 +948,8 @@ void AutofillPopupRowView::SetSelected(bool is_selected) {
     return;
 
   is_selected_ = is_selected;
-  if (is_selected) {
-    // Before firing the selection event, ensure that focus appears to be
-    // within the popup. This is helpful for ATs on some platforms,
-    // specifically on Windows, where selection events in a list are mapped
-    // to focus events. Without this call, the focus appears to be in content.
-    GetViewAccessibility().SetPopupFocusOverride();
-    NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
-  }
+  if (is_selected)
+    popup_view_->NotifyAXSelection(this);
   RefreshStyle();
 }
 
@@ -1020,21 +1014,6 @@ void AutofillPopupViewNativeViews::GetAccessibleNodeData(
       l10n_util::GetStringUTF16(IDS_AUTOFILL_POPUP_ACCESSIBLE_NODE_DATA));
 }
 
-void AutofillPopupViewNativeViews::VisibilityChanged(View* starting_from,
-                                                     bool is_visible) {
-  // Fire menu end event. The menu start event is delayed until the user
-  // navigates into the menu, otherwise some screen readers will ignore
-  // any focus events outside of the menu, including a focus event on
-  // the form control itself.
-  if (!is_visible) {
-    if (is_ax_menu_start_event_fired_) {
-      NotifyAccessibilityEvent(ax::mojom::Event::kMenuEnd, true);
-      GetViewAccessibility().EndPopupFocusOverride();
-    }
-    is_ax_menu_start_event_fired_ = false;
-  }
-}
-
 void AutofillPopupViewNativeViews::OnThemeChanged() {
   AutofillPopupBaseView::OnThemeChanged();
   SetBackground(views::CreateSolidBackground(GetBackgroundColor()));
@@ -1064,15 +1043,6 @@ void AutofillPopupViewNativeViews::Hide() {
 void AutofillPopupViewNativeViews::OnSelectedRowChanged(
     base::Optional<int> previous_row_selection,
     base::Optional<int> current_row_selection) {
-  if (!is_ax_menu_start_event_fired_) {
-    // By firing these and the matching kMenuEnd events, we are telling screen
-    // readers that the focus is only changing temporarily, and the screen
-    // reader will restore the focus back to the appropriate textfield when the
-    // menu closes.
-    NotifyAccessibilityEvent(ax::mojom::Event::kMenuStart, true);
-    is_ax_menu_start_event_fired_ = true;
-  }
-
   if (previous_row_selection) {
     rows_[*previous_row_selection]->SetSelected(false);
   }

chrome/browser/ui/views/autofill/autofill_popup_view_native_views.h

@@ -83,7 +83,6 @@ class AutofillPopupViewNativeViews : public AutofillPopupBaseView,
   // views::View:
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
   void OnThemeChanged() override;
-  void VisibilityChanged(View* starting_from, bool is_visible) override;
 
   // AutofillPopupView:
   void Show() override;
@@ -120,8 +119,6 @@ class AutofillPopupViewNativeViews : public AutofillPopupBaseView,
   views::View* body_container_ = nullptr;
   views::View* footer_container_ = nullptr;
 
-  bool is_ax_menu_start_event_fired_ = false;
-
   DISALLOW_COPY_AND_ASSIGN(AutofillPopupViewNativeViews);
 };
 

chrome/browser/ui/views/passwords/password_generation_popup_view_views.cc

@@ -195,7 +195,7 @@ void PasswordGenerationPopupViewViews::UpdateBoundsAndRedrawPopup() {
 
 void PasswordGenerationPopupViewViews::PasswordSelectionUpdated() {
   if (controller_->password_selected())
-    NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
+    NotifyAXSelection(this);
 
   password_view_->UpdateBackground(controller_->password_selected()
                                        ? GetSelectedBackgroundColor()