a11y: Exclude children of nested keyboard accessible controls from a11y tree.

ui/views/accessibility/native_view_accessibility_base.cc

Index: ui/views/accessibility/native_view_accessibility_base.cc
diff --git a/ui/views/accessibility/native_view_accessibility_base.cc b/ui/views/accessibility/native_view_accessibility_base.cc
index 524419bcb8df0948469cf16e36023dbad0791d62..6c58541c226d83c1ce11bd39669261f21cb07e9e 100644
--- a/ui/views/accessibility/native_view_accessibility_base.cc
+++ b/ui/views/accessibility/native_view_accessibility_base.cc
@@ -14,6 +14,52 @@
 
 namespace views {
 
+namespace {
+
+bool IsAccessibilityFocusableWhenEnabled(View* view) {
+  return view->focus_behavior() != View::FocusBehavior::NEVER &&
+         view->IsDrawn();
+}
+
+// Used to determine if a View should be ignored by accessibility clients by
+// being a non-keyboard-focusable child of a keyboard-focusable ancestor.

[tapted, 2017/04/03 05:50:36]

An example might be good here in the comment to clarify what this is used for
(i.e. button labels)

[Patti Lor, 2017/04/04 02:46:28]

Done.

+bool IsViewUnfocusableChildOfFocusableAncestor(View* view) {
+  if (IsAccessibilityFocusableWhenEnabled(view))
+    return false;
+
+  while (view->parent()) {
+    view = view->parent();
+    if (IsAccessibilityFocusableWhenEnabled(view))
+      return true;
+  }
+  return false;
+}
+
+// Recursively gets a list of unignored gfx::NativeViewAccessible children of a
+// given View. If a child is ignored, its children will be used instead.
+std::vector<gfx::NativeViewAccessible> GetUnignoredA11yChildren(View* view) {
+  std::vector<gfx::NativeViewAccessible> children;
+  for (int i = 0; i < view->child_count(); ++i) {
+    View* child = view->child_at(i);
+    if (NativeViewAccessibilityBase::Ignored(child)) {
+      std::vector<gfx::NativeViewAccessible> grandchildren =
+          GetUnignoredA11yChildren(child);
+      children.insert(children.end(), grandchildren.begin(),
+                      grandchildren.end());
+    } else {
+      children.push_back(child->GetNativeViewAccessible());
+    }
+  }
+  return children;
+}
+
+}  // namespace
+
+// static
+bool NativeViewAccessibilityBase::Ignored(View* view) {
+  return GetForView(view)->GetData().role == ui::AX_ROLE_IGNORED;
+}
+
 NativeViewAccessibilityBase::NativeViewAccessibilityBase(View* view)
     : view_(view),
       parent_widget_(nullptr),
@@ -77,11 +123,16 @@ const ui::AXNodeData& NativeViewAccessibilityBase::GetData() const {
   if (!view_->IsDrawn())
     data_.state |= (1 << ui::AX_STATE_INVISIBLE);
 
+  // Make sure this element is excluded from the a11y tree if there's a
+  // focusable parent. All keyboard focusable elements should be leaf nodes.
+  // Exceptions to this rule will themselves be accessibility focusable.
+  if (IsViewUnfocusableChildOfFocusableAncestor(view_))
+    data_.role = ui::AX_ROLE_IGNORED;
   return data_;
 }
 
 int NativeViewAccessibilityBase::GetChildCount() {
-  int child_count = view_->child_count();
+  int child_count = GetUnignoredA11yChildren(view_).size();
 
   std::vector<Widget*> child_widgets;
   PopulateChildWidgetVector(&child_widgets);
@@ -91,19 +142,19 @@ int NativeViewAccessibilityBase::GetChildCount() {
 }
 
 gfx::NativeViewAccessible NativeViewAccessibilityBase::ChildAtIndex(int index) {
-  // If this is a root view, our widget might have child widgets. Include
+  std::vector<gfx::NativeViewAccessible> a11y_children =
+      GetUnignoredA11yChildren(view_);
+  // Include the child widgets that may be present if this is a RootView.
   std::vector<Widget*> child_widgets;
   PopulateChildWidgetVector(&child_widgets);
-  int child_widget_count = static_cast<int>(child_widgets.size());
 
-  if (index < view_->child_count()) {
-    return view_->child_at(index)->GetNativeViewAccessible();
-  } else if (index < view_->child_count() + child_widget_count) {
-    Widget* child_widget = child_widgets[index - view_->child_count()];
-    return child_widget->GetRootView()->GetNativeViewAccessible();
-  }
+  if (index >= static_cast<int>(a11y_children.size() + child_widgets.size()))
+    return nullptr;
+  if (index < static_cast<int>(a11y_children.size()))
+    return a11y_children[index];
 
-  return nullptr;
+  Widget* child_widget = child_widgets[index - a11y_children.size()];
+  return child_widget->GetRootView()->GetNativeViewAccessible();
 }
 
 gfx::NativeWindow NativeViewAccessibilityBase::GetTopLevelWidget() {
@@ -113,17 +164,17 @@ gfx::NativeWindow NativeViewAccessibilityBase::GetTopLevelWidget() {
 }
 
 gfx::NativeViewAccessible NativeViewAccessibilityBase::GetParent() {
-  if (view_->parent())
-    return view_->parent()->GetNativeViewAccessible();
-
-  if (parent_widget_)
-    return parent_widget_->GetRootView()->GetNativeViewAccessible();
-
-  return nullptr;
+  View* parent_view = view_->parent();
+  if (parent_view) {
+    if (Ignored(parent_view))
+      return GetForView(parent_view)->GetParent();

[tapted, 2017/04/03 05:50:36]

(assuming it works..) things might be simpler if we don't recurse here. E.g.

View* parent_view = view_->parent();
while (parent_view && IsIgnoredView(parent_view))
  parent_view = parent_view->parent();

return parent_view ? parent_view->GetNativeViewAccessible() :
GetNativeViewAccessibleForWidget();

[Patti Lor, 2017/04/04 02:46:28]

Oh, that's way better. It works, thanks!

+    return parent_view->GetNativeViewAccessible();
+  }
+  return GetNativeViewAccessibleForWidget();
 }
 
 gfx::Vector2d NativeViewAccessibilityBase::GetGlobalCoordinateOffset() {
-  return gfx::Vector2d(0, 0);  // location is already in screen coordinates.
+  return gfx::Vector2d(0, 0);  // Location is already in screen coordinates.
 }
 
 gfx::NativeViewAccessible NativeViewAccessibilityBase::HitTestSync(int x,
@@ -142,23 +193,35 @@ gfx::NativeViewAccessible NativeViewAccessibilityBase::HitTestSync(int x,
       return child_root_view->GetNativeViewAccessible();
   }
 
+  // Check if the given point is inside |view_|.
   gfx::Point point(x, y);
   View::ConvertPointFromScreen(view_, &point);
   if (!view_->HitTestPoint(point))
     return nullptr;
 
-  // Check if the point is within any of the immediate children of this
-  // view. We don't have to search further because AXPlatformNode will
-  // do a recursive hit test if we return anything other than |this| or NULL.
+  // Early return if this node is a leaf, depending on whether it's ignored.
+  if (GetChildCount() == 0)
+    return Ignored(view_) ? nullptr : GetNativeObject();
+
+  // Check if the point is within any of the immediate children of this view. We
+  // don't have to search further because AXPlatformNodeWin will do a recursive
+  // hit test if we return anything other than GetNativeObject() or nullptr.
+  // The exception to this is if the recursion needs to travel up the tree,
+  // which may return a non-direct ancestor of the given node.
   for (int i = view_->child_count() - 1; i >= 0; --i) {
     View* child_view = view_->child_at(i);
-    if (!child_view->visible())
-      continue;
+    NativeViewAccessibilityBase* child_nva = GetForView(child_view);
 
     gfx::Point point_in_child_coords(point);
     view_->ConvertPointToTarget(view_, child_view, &point_in_child_coords);
-    if (child_view->HitTestPoint(point_in_child_coords))
-      return child_view->GetNativeViewAccessible();
+    if (!child_nva->GetData().HasStateFlag(ui::AX_STATE_INVISIBLE) &&
+        child_view->HitTestPoint(point_in_child_coords)) {
+      if (child_nva->GetChildCount() == 0 && Ignored(child_view)) {
+        // Multiple levels might be jumped here.
+        return child_nva->GetParent();

[tapted, 2017/04/03 05:50:36]

Can this get stuck in a loop?

The comment above suggests to me that AXPlatformNodeWin will test again if
something other than GetNativeObject() or nullptr is returned.

So, what happens when this *does* jump multiple levels, to a parent - can it
come back down, only to jump back up again?

[Patti Lor, 2017/04/04 02:46:28]

I think this is covered in the tests - if you look at
NVABaseIgnoredElementsTest.HitTestSync, these two lines (451-452):

  EXPECT_EQ(b_nva_->GetNativeObject(), a_root_nva_->HitTestSync(curr_x, y));
  EXPECT_EQ(b_nva_->GetNativeObject(), b_nva_->HitTestSync(curr_x, y));

Where this is testing a non-ignored element (B) with an ignored child (X2).
AXPlatformNodeWin::accHitTest() has a base case for when the hit target returned
is the same as itself, so as long as the second line above is passing, it won't
loop.

[tapted, 2017/04/04 08:13:13]

Acknowledged.

+      }
+      return child_nva->GetNativeObject();
+    }
   }
 
   // If it's not inside any of our children, it's inside this view.
@@ -225,6 +288,25 @@ void NativeViewAccessibilityBase::SetParentWidget(Widget* parent_widget) {
   parent_widget_->AddObserver(this);
 }
 
+// static
+NativeViewAccessibilityBase* NativeViewAccessibilityBase::GetForView(
+    View* view) {
+  // Retrieving the gfx::NativeViewAccessible also ensures that a
+  // NativeViewAccessibility exists for the given View.
+  gfx::NativeViewAccessible native_object = view->GetNativeViewAccessible();
+  ui::AXPlatformNode* node =
+      ui::AXPlatformNode::FromNativeViewAccessible(native_object);
+  DCHECK(node);
+  return static_cast<NativeViewAccessibilityBase*>(node->GetDelegate());
+}
+
+gfx::NativeViewAccessible
+NativeViewAccessibilityBase::GetNativeViewAccessibleForWidget() {
+  if (parent_widget_)
+    return parent_widget_->GetRootView()->GetNativeViewAccessible();
+  return nullptr;
+}
+
 void NativeViewAccessibilityBase::PopulateChildWidgetVector(
     std::vector<Widget*>* result_child_widgets) {
   // Only attach child widgets to the root view.