Fix regression where Mac menus are not read by VoiceOver

Ensure that the focus is reported as the current menu item in all menus,
not just in autofill popups, by using the popup focus override, similar
to what's used for autofill popups.

AX-Relnotes: n/a
Bug: 1100230
Change-Id: I80c0414aa5b675c966aab8953547fa36e0bac8f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2284050
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#787340}

chrome/browser/ui/views/menu_interactive_uitest.cc

@@ -19,15 +19,30 @@
 #include "ui/views/accessibility/ax_event_manager.h"
 #include "ui/views/accessibility/ax_event_observer.h"
 #include "ui/views/accessibility/view_accessibility.h"
+#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/submenu_view.h"
+#include "ui/views/focus/focus_manager.h"
+#include "ui/views/test/widget_test.h"
 #include "ui/views/widget/widget.h"
 
+#if !defined(OS_CHROMEOS)
+#include "ui/accessibility/platform/ax_platform_node.h"
+#endif
+
 namespace views {
 namespace test {
 
 namespace {
 
+class TestButton : public Button {
+ public:
+  TestButton() : Button(nullptr) {}
+  TestButton(const TestButton&) = delete;
+  TestButton& operator=(const TestButton&) = delete;
+  ~TestButton() override = default;
+};
+
 class TestAXEventObserver : public views::AXEventObserver {
  public:
   TestAXEventObserver() { views::AXEventManager::Get()->AddObserver(this); }
@@ -105,6 +120,10 @@ class MenuControllerUITest : public InProcessBrowserTest {
     ui::AXNodeData item_node_data;
     first_item_->GetViewAccessibility().GetAccessibleNodeData(&item_node_data);
     EXPECT_EQ(item_node_data.role, ax::mojom::Role::kMenuItem);
+
+#if !defined(OS_CHROMEOS)  // ChromeOS does not use popup focus override.
+    EXPECT_TRUE(first_item_->GetViewAccessibility().IsFocusedForTesting());
+#endif
     ui::AXNodeData menu_node_data;
     menu_item->GetSubmenu()->GetViewAccessibility().GetAccessibleNodeData(
         &menu_node_data);
@@ -130,6 +149,10 @@ class MenuControllerUITest : public InProcessBrowserTest {
 };
 
 IN_PROC_BROWSER_TEST_F(MenuControllerUITest, TestMouseOverShownMenu) {
+#if !defined(OS_CHROMEOS)
+  ui::AXPlatformNode::NotifyAddAXModeFlags(ui::kAXModeComplete);
+#endif
+
   // Create a parent widget.
   Widget* widget = new views::Widget;
   Widget::InitParams params(Widget::InitParams::TYPE_WINDOW);
@@ -140,7 +163,19 @@ IN_PROC_BROWSER_TEST_F(MenuControllerUITest, TestMouseOverShownMenu) {
 #endif
   widget->Init(std::move(params));
   widget->Show();
+  views::test::WidgetActivationWaiter waiter(widget, true);
   widget->Activate();
+  waiter.Wait();
+
+  // Create a focused test button, used to assert that it has accessibility
+  // focus before and after menu item is active, but not during.
+  TestButton button;
+  widget->GetContentsView()->AddChildView(&button);
+  FocusManager* focus_manager = widget->GetFocusManager();
+  focus_manager->SetFocusedView(&button);
+  EXPECT_TRUE(button.HasFocus());
+  EXPECT_TRUE(button.GetViewAccessibility().IsFocusedForTesting());
+
   // SetupMenu leaves the mouse position where the first menu item will be
   // when we run the menu.
   TestAXEventObserver observer;
@@ -157,6 +192,9 @@ IN_PROC_BROWSER_TEST_F(MenuControllerUITest, TestMouseOverShownMenu) {
   EXPECT_EQ(observer.GetMenuPopupEndCount(), 1);
   EXPECT_EQ(observer.GetMenuEndCount(), 1);
   EXPECT_FALSE(first_item_->IsSelected());
+#if !defined(OS_CHROMEOS)  // ChromeOS does not use popup focus override.
+  EXPECT_FALSE(first_item_->GetViewAccessibility().IsFocusedForTesting());
+#endif
   menu_runner_->RunMenuAt(widget, nullptr, gfx::Rect(),
                           views::MenuAnchorPosition::kTopLeft,
                           ui::MENU_SOURCE_NONE);
@@ -169,6 +207,10 @@ IN_PROC_BROWSER_TEST_F(MenuControllerUITest, TestMouseOverShownMenu) {
   // Process event(s), and check what's selected in the menu.
   RunPendingMessages();
   EXPECT_FALSE(first_item_->IsSelected());
+#if !defined(OS_CHROMEOS)  // ChromeOS does not use popup focus override.
+  EXPECT_FALSE(first_item_->GetViewAccessibility().IsFocusedForTesting());
+  EXPECT_TRUE(button.GetViewAccessibility().IsFocusedForTesting());
+#endif
   // Move mouse one pixel to left and verify that the first menu item
   // is selected.
   mouse_pos_.Offset(-1, 0);
@@ -177,7 +219,15 @@ IN_PROC_BROWSER_TEST_F(MenuControllerUITest, TestMouseOverShownMenu) {
                                            run_loop2.QuitClosure());
   run_loop2.Run();
   EXPECT_TRUE(first_item_->IsSelected());
+#if !defined(OS_CHROMEOS)  // ChromeOS does not use popup focus override.
+  EXPECT_TRUE(first_item_->GetViewAccessibility().IsFocusedForTesting());
+  EXPECT_FALSE(button.GetViewAccessibility().IsFocusedForTesting());
+#endif
   menu_runner_->Cancel();
+#if !defined(OS_CHROMEOS)  // ChromeOS does not use popup focus override.
+  EXPECT_FALSE(first_item_->GetViewAccessibility().IsFocusedForTesting());
+  EXPECT_TRUE(button.GetViewAccessibility().IsFocusedForTesting());
+#endif
   EXPECT_EQ(observer.GetMenuStartCount(), 2);
   EXPECT_EQ(observer.GetMenuPopupStartCount(), 2);
   EXPECT_EQ(observer.GetMenuPopupEndCount(), 2);
@@ -195,6 +245,7 @@ IN_PROC_BROWSER_TEST_F(MenuControllerUITest, FocusOnOrphanMenu) {
   // Going into full screen mode prevents pre-test focus and mouse position
   // state from affecting test, and helps ui_controls function correctly.
   chrome::ToggleFullscreenMode(browser());
+  ui::AXPlatformNode::NotifyAddAXModeFlags(ui::kAXModeComplete);
   MenuDelegate menu_delegate;
   MenuItemView* menu_item = new MenuItemView(&menu_delegate);
   TestAXEventObserver observer;
@@ -221,7 +272,9 @@ IN_PROC_BROWSER_TEST_F(MenuControllerUITest, FocusOnOrphanMenu) {
       false, false, false, false, loop.QuitClosure()));
   loop.Run();
   EXPECT_TRUE(first_item->IsSelected());
+  EXPECT_TRUE(first_item->GetViewAccessibility().IsFocusedForTesting());
   menu_runner->Cancel();
+  EXPECT_FALSE(first_item->GetViewAccessibility().IsFocusedForTesting());
   EXPECT_EQ(observer.GetMenuStartCount(), 1);
   EXPECT_EQ(observer.GetMenuPopupStartCount(), 1);
   EXPECT_EQ(observer.GetMenuPopupEndCount(), 1);

ui/views/accessibility/view_accessibility.cc

@@ -242,6 +242,10 @@ void ViewAccessibility::SetPopupFocusOverride() {}
 
 void ViewAccessibility::EndPopupFocusOverride() {}
 
+bool ViewAccessibility::IsFocusedForTesting() {
+  return view_->HasFocus() && !focused_virtual_child_;
+}
+
 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

@@ -153,6 +153,9 @@ class VIEWS_EXPORT ViewAccessibility {
   // Call when popup closes, if it used SetPopupFocusOverride().
   virtual void EndPopupFocusOverride();
 
+  // Return true if this view is considered focused.
+  virtual bool IsFocusedForTesting();
+
   // Call when a menu closes, to restore focus to where it was previously.
   virtual void FireFocusAfterMenuClose();
 

ui/views/accessibility/view_ax_platform_node_delegate.cc

@@ -154,6 +154,13 @@ void ViewAXPlatformNodeDelegate::EndPopupFocusOverride() {
   ui::AXPlatformNode::SetPopupFocusOverride(nullptr);
 }
 
+bool ViewAXPlatformNodeDelegate::IsFocusedForTesting() {
+  if (ui::AXPlatformNode::GetPopupFocusOverride())
+    return ui::AXPlatformNode::GetPopupFocusOverride() == GetNativeObject();
+
+  return ViewAccessibility::IsFocusedForTesting();
+}
+
 void ViewAXPlatformNodeDelegate::NotifyAccessibilityEvent(
     ax::mojom::Event event_type) {
   DCHECK(ax_platform_node_);
@@ -166,13 +173,19 @@ void ViewAXPlatformNodeDelegate::NotifyAccessibilityEvent(
 
   // Some events have special handling.
   switch (event_type) {
+    case ax::mojom::Event::kFocusAfterMenuClose: {
+      DCHECK(!ui::AXPlatformNode::GetPopupFocusOverride())
+          << "Must call ViewAccessibility::EndPopupFocusOverride() as menu "
+             "closes.";
+      break;
+    }
     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 "
+               "ViewAccessibility::EndPopupFocusOverride(), and focus has "
                "now moved on.";
       }
       break;

ui/views/accessibility/view_ax_platform_node_delegate.h

@@ -91,6 +91,7 @@ class ViewAXPlatformNodeDelegate : public ViewAccessibility,
 
   void SetPopupFocusOverride() override;
   void EndPopupFocusOverride() override;
+  bool IsFocusedForTesting() override;
 
  protected:
   explicit ViewAXPlatformNodeDelegate(View* view);

ui/views/controls/menu/menu_controller.cc

@@ -1385,6 +1385,11 @@ void MenuController::SetSelection(MenuItemView* menu_item,
        menu_item->GetType() != MenuItemView::Type::kSubMenu ||
        (menu_item->GetType() == MenuItemView::Type::kActionableSubMenu &&
         (selection_types & SELECTION_OPEN_SUBMENU) == 0))) {
+    // 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 elsewhere.
+    menu_item->GetViewAccessibility().SetPopupFocusOverride();
     menu_item->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
     // Notify an accessibility selected children changed event on the parent
     // submenu.

ui/views/controls/menu/submenu_view.cc

@@ -16,6 +16,7 @@
 #include "ui/compositor/paint_recorder.h"
 #include "ui/events/event.h"
 #include "ui/gfx/canvas.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/controls/menu/menu_config.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/controls/menu/menu_host.h"
@@ -428,6 +429,7 @@ void SubmenuView::Hide() {
     if (!GetMenuItem()->GetParentMenuItem()) {
       GetScrollViewContainer()->NotifyAccessibilityEvent(
           ax::mojom::Event::kMenuEnd, true);
+      GetViewAccessibility().EndPopupFocusOverride();
     }
     // Fire these kMenuPopupEnd for each menu/submenu that closes/hides.
     if (host_->IsVisible())