Defer AX events to avoid problems with transient focus events.

Some dialog boxes, like the accelerator confirmation dialog that appears
when you press Search+Control+M, focus the dialog and then immediately
after focus one of the buttons. This was causing ChromeVox to suppress
information about the context of the dialog.

Fix this by using PostTask to defer all events from Views. This doesn't
even add any delay, it just pushes the event firing to after the current
call stack so that transient changes get suppressed.

Manually confirmed that the experience with this particular dialog is
improved, other dialogs and views seem to continue to work fine.

Bug: 729449
Change-Id: I278c0c0b1dbdd26a654799fb5b314341942c1373
Reviewed-on: https://chromium-review.googlesource.com/1176207
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589201}

chrome/browser/ui/aura/accessibility/automation_manager_aura.cc

@@ -18,6 +18,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/render_frame_host.h"
 #include "ui/accessibility/ax_action_data.h"
+#include "ui/accessibility/ax_enum_util.h"
 #include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_tree_id_registry.h"
 #include "ui/accessibility/platform/aura_window_properties.h"
@@ -97,7 +98,8 @@ void AutomationManagerAura::Enable(BrowserContext* context) {
     if (active_window) {
       views::AXAuraObjWrapper* focus =
           views::AXAuraObjCache::GetInstance()->GetOrCreate(active_window);
-      SendEvent(context, focus, ax::mojom::Event::kChildrenChanged);
+      if (focus)
+        SendEvent(context, focus, ax::mojom::Event::kChildrenChanged);
     }
   }
   // Gain access to out-of-process native windows.
@@ -120,10 +122,32 @@ void AutomationManagerAura::HandleEvent(BrowserContext* context,
   if (!enabled_)
     return;
 
-  views::AXAuraObjWrapper* aura_obj = view ?
-      views::AXAuraObjCache::GetInstance()->GetOrCreate(view) :
-      current_tree_->GetRoot();
-  SendEvent(nullptr, aura_obj, event_type);
+  if (!view) {
+    SendEvent(nullptr, current_tree_->GetRoot(), event_type);
+    return;
+  }
+
+  views::AXAuraObjWrapper* obj =
+      views::AXAuraObjCache::GetInstance()->GetOrCreate(view);
+  if (!obj)
+    return;
+
+  // Post a task to handle the event at the end of the current call stack.
+  // This helps us avoid firing accessibility events for transient changes.
+  // because there's a chance that the underlying object being wrapped could
+  // be deleted, pass the ID of the object rather than the object pointer.
+  int32_t id = obj->GetUniqueId().Get();
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&AutomationManagerAura::SendEventOnObjectById,
+                     weak_ptr_factory_.GetWeakPtr(), id, event_type));
+}
+
+void AutomationManagerAura::SendEventOnObjectById(int32_t id,
+                                                  ax::mojom::Event event_type) {
+  views::AXAuraObjWrapper* obj = views::AXAuraObjCache::GetInstance()->Get(id);
+  if (obj)
+    SendEvent(nullptr, obj, event_type);
 }
 
 void AutomationManagerAura::HandleAlert(content::BrowserContext* context,
@@ -169,7 +193,8 @@ void AutomationManagerAura::OnEvent(views::AXAuraObjWrapper* aura_obj,
 AutomationManagerAura::AutomationManagerAura()
     : AXHostDelegate(extensions::api::automation::kDesktopTreeID),
       enabled_(false),
-      processing_events_(false) {}
+      processing_events_(false),
+      weak_ptr_factory_(this) {}
 
 AutomationManagerAura::~AutomationManagerAura() {
 }
@@ -185,6 +210,9 @@ void AutomationManagerAura::Reset(bool reset_serializer) {
 void AutomationManagerAura::SendEvent(BrowserContext* context,
                                       views::AXAuraObjWrapper* aura_obj,
                                       ax::mojom::Event event_type) {
+  if (!enabled_)
+    return;
+
   if (!current_tree_serializer_)
     return;
 
@@ -230,6 +258,9 @@ void AutomationManagerAura::SendEvent(BrowserContext* context,
   AutomationEventRouter* router = AutomationEventRouter::GetInstance();
   router->DispatchAccessibilityEvents(event_bundle);
 
+  if (event_bundle_callback_for_testing_)
+    event_bundle_callback_for_testing_.Run(event_bundle);
+
   processing_events_ = false;
   auto pending_events_copy = pending_events_;
   pending_events_.clear();

chrome/browser/ui/aura/accessibility/automation_manager_aura.h

@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/ui/aura/accessibility/ax_tree_source_aura.h"
 #include "ui/accessibility/ax_host_delegate.h"
 #include "ui/accessibility/ax_tree_serializer.h"
@@ -37,6 +38,8 @@ using AuraAXTreeSerializer =
                          ui::AXNodeData,
                          ui::AXTreeData>;
 
+struct ExtensionMsg_AccessibilityEventBundleParams;
+
 // Manages a tree of automation nodes.
 class AutomationManagerAura : public ui::AXHostDelegate,
                               views::AXAuraObjCache::Delegate {
@@ -65,6 +68,12 @@ class AutomationManagerAura : public ui::AXHostDelegate,
   void OnEvent(views::AXAuraObjWrapper* aura_obj,
                ax::mojom::Event event_type) override;
 
+  void set_event_bundle_callback_for_testing(
+      base::RepeatingCallback<void(ExtensionMsg_AccessibilityEventBundleParams)>
+          callback) {
+    event_bundle_callback_for_testing_ = callback;
+  }
+
  protected:
   AutomationManagerAura();
   ~AutomationManagerAura() override;
@@ -74,6 +83,8 @@ class AutomationManagerAura : public ui::AXHostDelegate,
 
   FRIEND_TEST_ALL_PREFIXES(AutomationManagerAuraBrowserTest, WebAppearsOnce);
 
+  void SendEventOnObjectById(int32_t id, ax::mojom::Event event_type);
+
   // Reset state in this manager. If |reset_serializer| is true, reset the
   // serializer to save memory.
   void Reset(bool reset_serializer);
@@ -101,6 +112,11 @@ class AutomationManagerAura : public ui::AXHostDelegate,
   std::vector<std::pair<views::AXAuraObjWrapper*, ax::mojom::Event>>
       pending_events_;
 
+  base::RepeatingCallback<void(ExtensionMsg_AccessibilityEventBundleParams)>
+      event_bundle_callback_for_testing_;
+
+  base::WeakPtrFactory<AutomationManagerAura> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(AutomationManagerAura);
 };
 

chrome/browser/ui/aura/accessibility/automation_manager_aura_browsertest.cc

@@ -6,13 +6,18 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/extensions/chrome_extension_messages.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "chrome/test/views/accessibility_checker.h"
 #include "content/public/browser/browser_accessibility_state.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "ui/accessibility/ax_node_data.h"
+#include "ui/views/accessibility/view_accessibility.h"
+#include "ui/views/view.h"
+#include "ui/views/widget/widget.h"
 
 namespace {
 
@@ -39,6 +44,57 @@ void FindAllHostsOfWebContentsWithAXTreeID(
   }
 }
 
+// A class that installs a test-only callback to handle automation events
+// from AutomationManagerAura, then waits until an automation event indicates
+// that a given node ID is focused.
+class AutomationEventWaiter {
+ public:
+  explicit AutomationEventWaiter(AutomationManagerAura* manager)
+      : run_loop_(std::make_unique<base::RunLoop>()), weak_factory_(this) {
+    manager->set_event_bundle_callback_for_testing(
+        base::BindRepeating(&AutomationEventWaiter::EventBundleCallback,
+                            weak_factory_.GetWeakPtr()));
+  }
+
+  // Returns immediately if the node with AXAuraObjCache ID |node_id|
+  // has ever been focused, otherwise spins a loop until that node is
+  // focused.
+  void WaitForNodeIdToBeFocused(int node_id) {
+    if (WasNodeIdFocused(node_id))
+      return;
+
+    node_id_to_wait_for_ = node_id;
+    run_loop_->Run();
+    run_loop_.reset(new base::RunLoop);
+  }
+
+  bool WasNodeIdFocused(int node_id) {
+    for (size_t i = 0; i < focused_node_ids_.size(); i++)
+      if (node_id == focused_node_ids_[i])
+        return true;
+    return false;
+  }
+
+ private:
+  // Callback to intercept messages sent by AutomationManagerAura.
+  void EventBundleCallback(
+      ExtensionMsg_AccessibilityEventBundleParams event_bundle) {
+    for (size_t i = 0; i < event_bundle.updates.size(); ++i) {
+      int focused_node_id = event_bundle.updates[i].tree_data.focus_id;
+      focused_node_ids_.push_back(focused_node_id);
+      if (focused_node_id == node_id_to_wait_for_)
+        run_loop_->QuitClosure().Run();
+    }
+  }
+
+  std::unique_ptr<base::RunLoop> run_loop_;
+  int node_id_to_wait_for_ = 0;
+  std::vector<int> focused_node_ids_;
+  base::WeakPtrFactory<AutomationEventWaiter> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutomationEventWaiter);
+};
+
 }  // namespace
 
 typedef InProcessBrowserTest AutomationManagerAuraBrowserTest;
@@ -83,3 +139,60 @@ IN_PROC_BROWSER_TEST_F(AutomationManagerAuraBrowserTest, WebAppearsOnce) {
     }
   }
 }
+
+IN_PROC_BROWSER_TEST_F(AutomationManagerAuraBrowserTest,
+                       TransientFocusChangesAreSuppressed) {
+  AutomationManagerAura* manager = AutomationManagerAura::GetInstance();
+  manager->Enable(browser()->profile());
+
+  views::Widget* widget = new views::Widget;
+  views::Widget::InitParams params(views::Widget::InitParams::TYPE_WINDOW);
+  params.bounds = {0, 0, 200, 200};
+  widget->Init(params);
+  widget->Show();
+  widget->Activate();
+
+  views::AXAuraObjCache* cache = views::AXAuraObjCache::GetInstance();
+  cache->set_focused_widget_for_testing(widget);
+
+  views::View* view1 = new views::View();
+  view1->GetViewAccessibility().OverrideName("view1");
+  view1->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
+  widget->GetRootView()->AddChildView(view1);
+  views::AXAuraObjWrapper* wrapper1 = cache->GetOrCreate(view1);
+  views::View* view2 = new views::View();
+  view2->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
+  view2->GetViewAccessibility().OverrideName("view2");
+  widget->GetRootView()->AddChildView(view2);
+  views::AXAuraObjWrapper* wrapper2 = cache->GetOrCreate(view2);
+  views::View* view3 = new views::View();
+  view3->GetViewAccessibility().OverrideName("view3");
+  view3->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
+  widget->GetRootView()->AddChildView(view3);
+  views::AXAuraObjWrapper* wrapper3 = cache->GetOrCreate(view3);
+
+  AutomationEventWaiter waiter(manager);
+
+  // Focus view1, then block until we get an accessibility event that
+  // shows this view is focused.
+  view1->RequestFocus();
+  waiter.WaitForNodeIdToBeFocused(wrapper1->GetUniqueId().Get());
+
+  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper1->GetUniqueId().Get()));
+  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper2->GetUniqueId().Get()));
+  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper3->GetUniqueId().Get()));
+
+  // Now focus view2 and then view3. We shouldn't ever get an event
+  // showing view2 as focused, just view3.
+  view2->RequestFocus();
+  view3->RequestFocus();
+  waiter.WaitForNodeIdToBeFocused(wrapper3->GetUniqueId().Get());
+
+  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper1->GetUniqueId().Get()));
+  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper2->GetUniqueId().Get()));
+  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper3->GetUniqueId().Get()));
+
+  cache->set_focused_widget_for_testing(nullptr);
+
+  AddFailureOnWidgetAccessibilityError(widget);
+}

ui/views/accessibility/ax_aura_obj_cache.cc

@@ -31,6 +31,8 @@ AXAuraObjCache* AXAuraObjCache::GetInstance() {
 }
 
 AXAuraObjWrapper* AXAuraObjCache::GetOrCreate(View* view) {
+  if (!view->GetWidget())
+    return nullptr;
   return CreateInternal<AXViewObjWrapper>(view, view_to_id_map_);
 }
 
@@ -122,24 +124,28 @@ AXAuraObjCache::~AXAuraObjCache() {
 }
 
 View* AXAuraObjCache::GetFocusedView() {
-  if (root_windows_.empty())
-    return nullptr;
-  aura::client::FocusClient* focus_client =
-      GetFocusClient(*root_windows_.begin());
-  if (!focus_client)
-    return nullptr;
-
-  aura::Window* focused_window = focus_client->GetFocusedWindow();
-  if (!focused_window)
-    return nullptr;
-
-  Widget* focused_widget = Widget::GetWidgetForNativeView(focused_window);
-  while (!focused_widget) {
-    focused_window = focused_window->parent();
+  Widget* focused_widget = focused_widget_for_testing_;
+  aura::Window* focused_window = nullptr;
+  if (!focused_widget) {
+    if (root_windows_.empty())
+      return nullptr;
+    aura::client::FocusClient* focus_client =
+        GetFocusClient(*root_windows_.begin());
+    if (!focus_client)
+      return nullptr;
+
+    focused_window = focus_client->GetFocusedWindow();
     if (!focused_window)
-      break;
+      return nullptr;
 
     focused_widget = Widget::GetWidgetForNativeView(focused_window);
+    while (!focused_widget) {
+      focused_window = focused_window->parent();
+      if (!focused_window)
+        break;
+
+      focused_widget = Widget::GetWidgetForNativeView(focused_window);
+    }
   }
 
   if (!focused_widget)
@@ -153,7 +159,8 @@ View* AXAuraObjCache::GetFocusedView() {
   if (focused_view)
     return focused_view;
 
-  if (focused_window->GetProperty(
+  if (focused_window &&
+      focused_window->GetProperty(
           aura::client::kAccessibilityFocusFallsbackToWidgetKey)) {
     // If focused widget has non client view, falls back to first child view of
     // its client view. We don't expect that non client view gets keyboard

ui/views/accessibility/ax_aura_obj_cache.h

@@ -92,6 +92,13 @@ class VIEWS_EXPORT AXAuraObjCache : public aura::client::FocusChangeObserver {
 
   void SetDelegate(Delegate* delegate) { delegate_ = delegate; }
 
+  // Changes the behavior of GetFocusedView() so that it only considers
+  // views within the given Widget, this enables making tests
+  // involving focus reliable.
+  void set_focused_widget_for_testing(views::Widget* widget) {
+    focused_widget_for_testing_ = widget;
+  }
+
  private:
   friend struct base::DefaultSingletonTraits<AXAuraObjCache>;
 
@@ -131,6 +138,8 @@ class VIEWS_EXPORT AXAuraObjCache : public aura::client::FocusChangeObserver {
 
   std::set<aura::Window*> root_windows_;
 
+  views::Widget* focused_widget_for_testing_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(AXAuraObjCache);
 };
 

ui/views/mus/ax_remote_host_unittest.cc

@@ -165,18 +165,6 @@ TEST_F(AXRemoteHostTest, AutomationEnabledTwice) {
   EXPECT_EQ(ax::mojom::Event::kLoadComplete, service.last_event_.event_type);
 }
 
-// Views can trigger accessibility events during Widget construction before the
-// AXRemoteHost starts monitoring the widget. This happens with the material
-// design focus ring on text fields. Verify we don't crash in this case.
-// https://crbug.com/862759
-TEST_F(AXRemoteHostTest, SendEventBeforeWidgetCreated) {
-  TestAXHostService service(true /*automation_enabled*/);
-  AXRemoteHost* remote = CreateRemote(&service);
-  views::View view;
-  remote->HandleEvent(&view, ax::mojom::Event::kLocationChanged);
-  // No crash.
-}
-
 // Verifies that the AXRemoteHost stops monitoring widgets that are closed
 // asynchronously, like when ash requests close via DesktopWindowTreeHostMus.
 // https://crbug.com/869608
@@ -230,6 +218,9 @@ TEST_F(AXRemoteHostTest, PerformAction) {
 
   // Create a view to sense the action.
   TestView view;
+  view.SetBounds(0, 0, 100, 100);
+  std::unique_ptr<Widget> widget = CreateTestWidget();
+  widget->GetRootView()->AddChildView(&view);
   AXAuraObjCache::GetInstance()->GetOrCreate(&view);
 
   // Request an action on the view.