Ensure nodes hosting child trees prune their claimed descendants

R=dmazzoni@chromium.org Bug: 1185764 Change-Id: I6983fe34d4ab76b06769747e22e0faecbdc7d7ce Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2742297 Commit-Queue: David Tseng <dtseng@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Jun Mukai <mukai@chromium.org> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Commit-Position: refs/heads/master@{#861968}
FairyWorld · Mar 11, 2021 · ce36653 · ce36653
1 parent 5153f06
commit ce36653
Show file tree

Hide file tree

Showing 13 changed files with 79 additions and 41 deletions.
diff --git a/ash/public/cpp/external_arc/message_center/arc_notification_content_view.cc b/ash/public/cpp/external_arc/message_center/arc_notification_content_view.cc
@@ -26,6 +26,7 @@
 #include "ui/message_center/public/cpp/message_center_constants.h"
 #include "ui/message_center/public/cpp/notification.h"
 #include "ui/strings/grit/ui_strings.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/focus/focus_manager.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
 #include "ui/views/widget/root_view.h"
@@ -764,8 +765,7 @@ void ArcNotificationContentView::GetAccessibleNodeData(
     ui::AXNodeData* node_data) {
   if (surface_ && surface_->GetAXTreeId() != ui::AXTreeIDUnknown()) {
     node_data->role = ax::mojom::Role::kClient;
-    node_data->AddStringAttribute(ax::mojom::StringAttribute::kChildTreeId,
-                                  surface_->GetAXTreeId().ToString());
+    GetViewAccessibility().OverrideChildTreeID(surface_->GetAXTreeId());
   } else {
     node_data->role = ax::mojom::Role::kButton;
     node_data->AddStringAttribute(

diff --git a/components/exo/fullscreen_shell_surface.cc b/components/exo/fullscreen_shell_surface.cc
@@ -14,6 +14,7 @@
 #include "ui/aura/window_occlusion_tracker.h"
 #include "ui/aura/window_targeter.h"
 #include "ui/compositor/compositor.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/metadata/metadata_header_macros.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
 #include "ui/views/view.h"
@@ -33,20 +34,11 @@ class FullscreenShellSurface::FullscreenShellView : public views::View {
   // views::View:
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
     node_data->role = ax::mojom::Role::kClient;
-
-    if (child_ax_tree_id_ == ui::AXTreeIDUnknown())
-      return;
-
-    node_data->AddStringAttribute(ax::mojom::StringAttribute::kChildTreeId,
-                                  child_ax_tree_id_.ToString());
   }
 
   void SetChildAxTreeId(ui::AXTreeID child_ax_tree_id) {
-    child_ax_tree_id_ = child_ax_tree_id;
+    GetViewAccessibility().OverrideChildTreeID(child_ax_tree_id);
   }
-
- private:
-  ui::AXTreeID child_ax_tree_id_ = ui::AXTreeIDUnknown();
 };
 
 BEGIN_METADATA(FullscreenShellSurface, FullscreenShellView, views::View)
@@ -253,11 +245,6 @@ void FullscreenShellSurface::SetEnabled(bool enabled) {
   contents_view_->SetEnabled(enabled);
 }
 
-void FullscreenShellSurface::GetAccessibleNodeData(ui::AXNodeData* node_data) {
-  DCHECK(contents_view_);
-  contents_view_->GetAccessibleNodeData(node_data);
-}
-
 void FullscreenShellSurface::UpdateHostWindowBounds() {
   // This method applies multiple changes to the window tree. Use ScopedPause
   // to ensure that occlusion isn't recomputed before all changes have been

diff --git a/components/exo/fullscreen_shell_surface.h b/components/exo/fullscreen_shell_surface.h
@@ -79,7 +79,6 @@ class FullscreenShellSurface : public SurfaceTreeHost,
 
   void SetChildAxTreeId(ui::AXTreeID child_ax_tree_id);
   void SetEnabled(bool enabled);
-  void GetAccessibleNodeData(ui::AXNodeData* node_data);
 
  private:
   class FullscreenShellView;

diff --git a/components/exo/fullscreen_shell_surface_unittest.cc b/components/exo/fullscreen_shell_surface_unittest.cc
@@ -20,6 +20,7 @@
 #include "ui/aura/window.h"
 #include "ui/compositor/compositor.h"
 #include "ui/gfx/buffer_types.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/widget/widget.h"
 
 namespace exo {
@@ -232,14 +233,16 @@ TEST_F(FullscreenShellSurfaceTest, SetAXChildTree) {
   std::unique_ptr<FullscreenShellSurface> fullscreen_surface(
       new FullscreenShellSurface());
   fullscreen_surface->SetSurface(surface.get());
+  const views::ViewAccessibility& view_accessibility =
+      fullscreen_surface->GetContentsView()->GetViewAccessibility();
   ui::AXNodeData node_data;
-  fullscreen_surface->GetAccessibleNodeData(&node_data);
+  view_accessibility.GetAccessibleNodeData(&node_data);
   EXPECT_FALSE(
       node_data.HasStringAttribute(ax::mojom::StringAttribute::kChildTreeId));
 
   ui::AXTreeID tree_id = ui::AXTreeID::CreateNewAXTreeID();
   fullscreen_surface->SetChildAxTreeId(tree_id);
-  fullscreen_surface->GetAccessibleNodeData(&node_data);
+  view_accessibility.GetAccessibleNodeData(&node_data);
   EXPECT_TRUE(
       node_data.HasStringAttribute(ax::mojom::StringAttribute::kChildTreeId));
 }

diff --git a/components/exo/shell_surface_base.cc b/components/exo/shell_surface_base.cc
@@ -52,6 +52,7 @@
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/gfx/geometry/vector2d_conversions.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/core/coordinate_conversion.h"
@@ -486,10 +487,7 @@ void ShellSurfaceBase::UnsetSnap() {
 }
 
 void ShellSurfaceBase::SetChildAxTreeId(ui::AXTreeID child_ax_tree_id) {
-  if (child_ax_tree_id_ == child_ax_tree_id)
-    return;
-
-  child_ax_tree_id_ = child_ax_tree_id;
+  GetViewAccessibility().OverrideChildTreeID(child_ax_tree_id);
   this->NotifyAccessibilityEvent(ax::mojom::Event::kChildrenChanged, false);
 }
 
@@ -899,12 +897,6 @@ gfx::Size ShellSurfaceBase::GetMaximumSize() const {
 
 void ShellSurfaceBase::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->role = ax::mojom::Role::kClient;
-
-  if (child_ax_tree_id_ == ui::AXTreeIDUnknown())
-    return;
-
-  node_data->AddStringAttribute(ax::mojom::StringAttribute::kChildTreeId,
-                                child_ax_tree_id_.ToString());
 }
 
 views::FocusTraversable* ShellSurfaceBase::GetFocusTraversable() {

diff --git a/components/exo/shell_surface_base.h b/components/exo/shell_surface_base.h
@@ -347,7 +347,6 @@ class ShellSurfaceBase : public SurfaceTreeHost,
   gfx::Size maximum_size_;
   gfx::Size pending_maximum_size_;
   gfx::SizeF pending_aspect_ratio_;
-  ui::AXTreeID child_ax_tree_id_ = ui::AXTreeIDUnknown();
 
   bool skip_ime_processing_ = false;
   std::unique_ptr<views::Widget> overlay_widget_;

diff --git a/ui/views/accessibility/ax_tree_source_views_unittest.cc b/ui/views/accessibility/ax_tree_source_views_unittest.cc
@@ -152,5 +152,20 @@ TEST_F(AXTreeSourceViewsTest, IgnoredView) {
   EXPECT_TRUE(tree.IsValid(cache.GetOrCreate(ignored_view)));
 }
 
+TEST_F(AXTreeSourceViewsTest, ViewWithChildTreeHasNoChildren) {
+  View* contents_view = widget_->GetContentsView();
+  contents_view->GetViewAccessibility().OverrideChildTreeID(
+      ui::AXTreeID::CreateNewAXTreeID());
+
+  AXAuraObjCache cache;
+  TestAXTreeSourceViews tree(cache.GetOrCreate(widget_.get()), &cache);
+  auto* ax_obj = cache.GetOrCreate(contents_view);
+  EXPECT_TRUE(tree.IsValid(ax_obj));
+  std::vector<AXAuraObjWrapper*> children;
+  ax_obj->GetChildren(&children);
+  EXPECT_TRUE(children.empty());
+  EXPECT_EQ(nullptr, cache.GetOrCreate(textfield_)->GetParent());
+}
+
 }  // namespace
 }  // namespace views
diff --git a/ui/views/accessibility/ax_view_obj_wrapper.cc b/ui/views/accessibility/ax_view_obj_wrapper.cc
@@ -30,8 +30,13 @@ AXAuraObjWrapper* AXViewObjWrapper::GetParent() {
   if (!view_)
     return nullptr;
 
-  if (view_->parent())
+  if (view_->parent()) {
+    if (view_->parent()->GetViewAccessibility().GetChildTreeID() !=
+        ui::AXTreeIDUnknown())
+      return nullptr;
+
     return aura_obj_cache_->GetOrCreate(view_->parent());
+  }
 
   if (view_->GetWidget())
     return aura_obj_cache_->GetOrCreate(view_->GetWidget());
@@ -45,6 +50,11 @@ void AXViewObjWrapper::GetChildren(
     return;
 
   const ViewAccessibility& view_accessibility = view_->GetViewAccessibility();
+
+  // Ignore this view's descendants if it has a child tree.
+  if (view_accessibility.GetChildTreeID() != ui::AXTreeIDUnknown())
+    return;
+
   if (view_accessibility.IsLeaf())
     return;
 

diff --git a/ui/views/accessibility/ax_window_obj_wrapper.cc b/ui/views/accessibility/ax_window_obj_wrapper.cc
@@ -103,11 +103,24 @@ AXAuraObjWrapper* AXWindowObjWrapper::GetParent() {
   if (!parent)
     return nullptr;
 
+  if (parent->GetProperty(ui::kChildAXTreeID) &&
+      ui::AXTreeID::FromString(*(parent->GetProperty(ui::kChildAXTreeID))) !=
+          ui::AXTreeIDUnknown()) {
+    return nullptr;
+  }
+
   return aura_obj_cache_->GetOrCreate(parent);
 }
 
 void AXWindowObjWrapper::GetChildren(
     std::vector<AXAuraObjWrapper*>* out_children) {
+  // Ignore this window's descendants if it has a child tree.
+  if (window_->GetProperty(ui::kChildAXTreeID) &&
+      ui::AXTreeID::FromString(*(window_->GetProperty(ui::kChildAXTreeID))) !=
+          ui::AXTreeIDUnknown()) {
+    return;
+  }
+
   for (auto* child : window_->children())
     out_children->push_back(aura_obj_cache_->GetOrCreate(child));
 

diff --git a/ui/views/accessibility/view_accessibility.cc b/ui/views/accessibility/view_accessibility.cc
@@ -270,6 +270,14 @@ void ViewAccessibility::GetAccessibleNodeData(ui::AXNodeData* data) const {
 
   if (view_->context_menu_controller())
     data->AddAction(ax::mojom::Action::kShowContextMenu);
+
+  DCHECK(!data->HasStringAttribute(ax::mojom::StringAttribute::kChildTreeId))
+      << "Please annotate child tree ids using "
+         "ViewAccessibility::OverrideChildTreeID.";
+  if (child_tree_id_) {
+    data->AddStringAttribute(ax::mojom::StringAttribute::kChildTreeId,
+                             child_tree_id_->ToString());
+  }
 }
 
 void ViewAccessibility::OverrideFocus(AXVirtualView* virtual_view) {
@@ -427,6 +435,17 @@ Widget* ViewAccessibility::GetPreviousFocus() const {
   return previous_focus_;
 }
 
+void ViewAccessibility::OverrideChildTreeID(ui::AXTreeID tree_id) {
+  if (tree_id == ui::AXTreeIDUnknown())
+    child_tree_id_ = base::nullopt;
+  else
+    child_tree_id_ = tree_id;
+}
+
+ui::AXTreeID ViewAccessibility::GetChildTreeID() const {
+  return child_tree_id_ ? *child_tree_id_ : ui::AXTreeIDUnknown();
+}
+
 gfx::NativeViewAccessible ViewAccessibility::GetNativeObject() const {
   return nullptr;
 }

diff --git a/ui/views/accessibility/view_accessibility.h b/ui/views/accessibility/view_accessibility.h
@@ -157,6 +157,10 @@ class VIEWS_EXPORT ViewAccessibility {
   Widget* GetNextFocus() const;
   Widget* GetPreviousFocus() const;
 
+  // Override the child tree id.
+  void OverrideChildTreeID(ui::AXTreeID tree_id);
+  ui::AXTreeID GetChildTreeID() const;
+
   // Returns the accessibility object that represents the View whose
   // accessibility is managed by this instance. This may be an AXPlatformNode or
   // it may be a native accessible object implemented by another class.
@@ -264,6 +268,9 @@ class VIEWS_EXPORT ViewAccessibility {
   Widget* next_focus_ = nullptr;
   Widget* previous_focus_ = nullptr;
 
+  // This view's child tree id.
+  base::Optional<ui::AXTreeID> child_tree_id_;
+
 #if defined(USE_AURA) && !BUILDFLAG(IS_CHROMEOS_ASH)
   // Each instance of ViewAccessibility that's associated with a root View
   // owns an ViewsAXTreeManager. For other Views, this should be nullptr.

diff --git a/ui/views/controls/webview/webview.cc b/ui/views/controls/webview/webview.cc
@@ -20,6 +20,7 @@
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/accessibility/platform/ax_platform_node.h"
 #include "ui/events/event.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/focus/focus_manager.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
 #include "ui/views/views_delegate.h"
@@ -250,10 +251,6 @@ void WebView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   // provided via other means. Providing it here would be redundant.
   // Mark the name as explicitly empty so that accessibility_checks pass.
   node_data->SetNameExplicitlyEmpty();
-  if (child_ax_tree_id_ != ui::AXTreeIDUnknown()) {
-    node_data->AddStringAttribute(ax::mojom::StringAttribute::kChildTreeId,
-                                  child_ax_tree_id_.ToString());
-  }
 }
 
 void WebView::AddedToWidget() {
@@ -409,7 +406,8 @@ void WebView::UpdateCrashedOverlayView() {
 void WebView::NotifyAccessibilityWebContentsChanged() {
   content::RenderFrameHost* rfh =
       web_contents() ? web_contents()->GetMainFrame() : nullptr;
-  child_ax_tree_id_ = rfh ? rfh->GetAXTreeID() : ui::AXTreeIDUnknown();
+  GetViewAccessibility().OverrideChildTreeID(rfh ? rfh->GetAXTreeID()
+                                                 : ui::AXTreeIDUnknown());
   NotifyAccessibilityEvent(ax::mojom::Event::kChildrenChanged, false);
 }
 

diff --git a/ui/views/controls/webview/webview.h b/ui/views/controls/webview/webview.h
@@ -197,10 +197,6 @@ class WEBVIEW_EXPORT WebView : public View,
   gfx::Size min_size_;
   gfx::Size max_size_;
 
-  // Tracks the child accessibility tree id which is associated with the
-  // WebContents's main RenderFrameHost.
-  ui::AXTreeID child_ax_tree_id_;
-
   DISALLOW_COPY_AND_ASSIGN(WebView);
 };