Clean up bad popup focus states after menu usage

Previous code rerouted accessibility focus reporting when any menu item
or list item was focused/selected, and the current menu depth > 0.
However, the menu depth calculation has shown to easily break.

Since the focus override is only necessary for autofill menus, this CL
changes the focus override so that it is is only turned on when needed
by a popup that is overriding focus in the web page.

Bug: 1039422
Change-Id: I49bd0db88bd945a6fc7be3309701d394670d7099
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2247039
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779433}

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

@@ -948,8 +948,14 @@ void AutofillPopupRowView::SetSelected(bool is_selected) {
     return;
 
   is_selected_ = is_selected;
-  if (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);
+  }
   RefreshStyle();
 }
 
@@ -1021,8 +1027,10 @@ void AutofillPopupViewNativeViews::VisibilityChanged(View* starting_from,
   // 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_)
+    if (is_ax_menu_start_event_fired_) {
       NotifyAccessibilityEvent(ax::mojom::Event::kMenuEnd, true);
+      GetViewAccessibility().EndPopupFocusOverride();
+    }
     is_ax_menu_start_event_fired_ = false;
   }
 }

ui/views/accessibility/view_accessibility.cc

@@ -234,6 +234,10 @@ void ViewAccessibility::OverrideFocus(AXVirtualView* virtual_view) {
   }
 }
 
+void ViewAccessibility::SetPopupFocusOverride() {}
+
+void ViewAccessibility::EndPopupFocusOverride() {}
+
 void ViewAccessibility::OverrideRole(const ax::mojom::Role role) {
   DCHECK(IsValidRoleForViews(role)) << "Invalid role for Views.";
   custom_data_.role = role;

ui/views/accessibility/view_accessibility.h

@@ -144,6 +144,16 @@ class VIEWS_EXPORT ViewAccessibility {
   // native accessibility object associated with this view.
   gfx::NativeViewAccessible GetFocusedDescendant();
 
+  // Call when this is the active descendant of a popup view that temporarily
+  // takes over focus. It is only necessary to use this for menus like autofill,
+  // where the actual focus is in content.
+  // When the popup closes, call EndPopupFocusOverride().
+  virtual void SetPopupFocusOverride();
+
+  // Call when popup closes, if it used SetPopupFocusOverride().
+  virtual void EndPopupFocusOverride();
+
+  // Call when a menu closes, to restore focus to where it was previously.
   virtual void FireFocusAfterMenuClose();
 
   // Used for testing. Allows a test to watch accessibility events.

ui/views/accessibility/view_ax_platform_node_delegate.cc

@@ -122,9 +122,6 @@ struct ViewAXPlatformNodeDelegate::ChildWidgetsResult {
   bool is_tab_modal_showing;
 };
 
-// static
-int ViewAXPlatformNodeDelegate::menu_depth_ = 0;
-
 ViewAXPlatformNodeDelegate::ViewAXPlatformNodeDelegate(View* view)
     : ViewAccessibility(view) {
   ax_platform_node_ = ui::AXPlatformNode::Create(this);
@@ -140,7 +137,7 @@ ViewAXPlatformNodeDelegate::ViewAXPlatformNodeDelegate(View* view)
 
 ViewAXPlatformNodeDelegate::~ViewAXPlatformNodeDelegate() {
   if (ui::AXPlatformNode::GetPopupFocusOverride() == GetNativeObject())
-    ui::AXPlatformNode::SetPopupFocusOverride(nullptr);
+    EndPopupFocusOverride();
   ax_platform_node_->Destroy();
 }
 
@@ -149,6 +146,14 @@ gfx::NativeViewAccessible ViewAXPlatformNodeDelegate::GetNativeObject() const {
   return ax_platform_node_->GetNativeViewAccessible();
 }
 
+void ViewAXPlatformNodeDelegate::SetPopupFocusOverride() {
+  ui::AXPlatformNode::SetPopupFocusOverride(GetNativeObject());
+}
+
+void ViewAXPlatformNodeDelegate::EndPopupFocusOverride() {
+  ui::AXPlatformNode::SetPopupFocusOverride(nullptr);
+}
+
 void ViewAXPlatformNodeDelegate::NotifyAccessibilityEvent(
     ax::mojom::Event event_type) {
   DCHECK(ax_platform_node_);
@@ -161,16 +166,15 @@ void ViewAXPlatformNodeDelegate::NotifyAccessibilityEvent(
 
   // Some events have special handling.
   switch (event_type) {
-    case ax::mojom::Event::kMenuStart:
-      OnMenuStart();
-      break;
-    case ax::mojom::Event::kMenuEnd:
-      OnMenuEnd();
-      break;
-    case ax::mojom::Event::kSelection: {
-      ax::mojom::Role role = GetData().role;
-      if (menu_depth_ && (ui::IsMenuItem(role) || ui::IsListItem(role)))
-        OnMenuItemActive();
+    case ax::mojom::Event::kFocus: {
+      if (ui::AXPlatformNode::GetPopupFocusOverride()) {
+        DCHECK_EQ(ui::AXPlatformNode::GetPopupFocusOverride(),
+                  GetNativeObject())
+            << "If the popup focus override is on, then the kFocus event must "
+               "match it. Most likely the popup has closed, but did not call "
+               "ViewAccessibility::FireFocusAfterMenuClose(), and focus has "
+               "now moved on.";
+      }
       break;
     }
     case ax::mojom::Event::kFocusContext: {
@@ -202,27 +206,6 @@ void ViewAXPlatformNodeDelegate::AnnounceText(const base::string16& text) {
 }
 #endif
 
-void ViewAXPlatformNodeDelegate::OnMenuItemActive() {
-  // When a native menu is shown and has an item selected, treat it and the
-  // currently selected item as focused, even though the actual focus is in the
-  // browser's currently focused textfield.
-  ui::AXPlatformNode::SetPopupFocusOverride(
-      ax_platform_node_->GetNativeViewAccessible());
-}
-
-void ViewAXPlatformNodeDelegate::OnMenuStart() {
-  ++menu_depth_;
-}
-
-void ViewAXPlatformNodeDelegate::OnMenuEnd() {
-  // When a native menu is hidden, restore accessibility focus to the current
-  // focus in the document.
-  if (menu_depth_ >= 1)
-    --menu_depth_;
-  if (menu_depth_ == 0)
-    ui::AXPlatformNode::SetPopupFocusOverride(nullptr);
-}
-
 void ViewAXPlatformNodeDelegate::FireFocusAfterMenuClose() {
   ui::AXPlatformNodeBase* focused_node =
       static_cast<ui::AXPlatformNodeBase*>(ax_platform_node_);

ui/views/accessibility/view_ax_platform_node_delegate.h

@@ -86,6 +86,9 @@ class ViewAXPlatformNodeDelegate : public ViewAccessibility,
   base::Optional<int> GetPosInSet() const override;
   base::Optional<int> GetSetSize() const override;
 
+  void SetPopupFocusOverride() override;
+  void EndPopupFocusOverride() override;
+
  protected:
   explicit ViewAXPlatformNodeDelegate(View* view);
 
@@ -100,18 +103,11 @@ class ViewAXPlatformNodeDelegate : public ViewAccessibility,
 
   ChildWidgetsResult GetChildWidgets() const;
 
-  void OnMenuItemActive();
-  void OnMenuStart();
-  void OnMenuEnd();
-
   // We own this, but it is reference-counted on some platforms so we can't use
   // a unique_ptr. It is destroyed in the destructor.
   ui::AXPlatformNode* ax_platform_node_;
 
   mutable ui::AXNodeData data_;
-
-  // Levels of menu are currently open, e.g. 0: none, 1: top, 2: submenu ...
-  static int32_t menu_depth_;
 };
 
 }  // namespace views