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

ui/views/accessibility/native_view_accessibility.cc

Index: ui/views/accessibility/native_view_accessibility.cc
diff --git a/ui/views/accessibility/native_view_accessibility.cc b/ui/views/accessibility/native_view_accessibility.cc
index 409ac5a80e802e46f6db09e2a8bbbb0cbce44b65..3e199dd32f24b99e0c7f7b01de860c2e984382aa 100644
--- a/ui/views/accessibility/native_view_accessibility.cc
+++ b/ui/views/accessibility/native_view_accessibility.cc
@@ -5,6 +5,7 @@
 #include "ui/views/accessibility/native_view_accessibility.h"
 
 #include "base/strings/utf_string_conversions.h"
+#include "ui/accessibility/platform/ax_platform_node.h"
 #include "ui/events/event_utils.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/views/controls/native/native_view_host.h"
@@ -13,12 +14,75 @@
 
 namespace views {
 
-#if !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
+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.
+bool IsViewUnfocusableChildOfFocusableAncestor(View* view) {
+  if (IsAccessibilityFocusableWhenEnabled(view))
+    return false;
+
+  while (view->parent()) {
+    view = view->parent();
+    if (IsAccessibilityFocusableWhenEnabled(view))
+      return true;
+  }
+  return false;
+}
+
+// Convenience method for checking if a View should be ignored by a11y.
+bool IsViewA11yIgnored(View* view) {
+  return NativeViewAccessibility::GetForView(view)->GetData().role ==
+         ui::AX_ROLE_IGNORED;
+}
+
+// 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 (IsViewA11yIgnored(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
-NativeViewAccessibility* NativeViewAccessibility::Create(View* view) {
-  return new NativeViewAccessibility(view);
+std::unique_ptr<NativeViewAccessibility> NativeViewAccessibility::CreateForView(
+    View* view) {
+  return base::WrapUnique(Create(view));
+}
+
+// static
+NativeViewAccessibility* NativeViewAccessibility::GetForView(View* view) {
+#if defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
+  // 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<NativeViewAccessibility*>(node->GetDelegate());
+#else
+  // Platforms (currently ChromeOS) without a native implementation also have no
+  // gfx::NativeViewAccessible, so just create a new instance.
+  return Create(view);

[tapted, 2017/02/21 06:01:11]

I think this means it will be leaked. I think we need that friend, so we can do

view->GetNativeViewAccessible();
return view_->native_view_accessibility_;

If we are only a friend on OS_CHROMEOS, maybe that will allow us to sell it to
sky

[Patti Lor, 2017/02/27 05:31:04]

Done.

+#endif
 }
-#endif  // !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
 
 NativeViewAccessibility::NativeViewAccessibility(View* view)
     : view_(view),
@@ -38,10 +102,6 @@ gfx::NativeViewAccessible NativeViewAccessibility::GetNativeObject() {
   return ax_node_ ? ax_node_->GetNativeViewAccessible() : nullptr;
 }
 
-void NativeViewAccessibility::Destroy() {
-  delete this;
-}
-
 void NativeViewAccessibility::NotifyAccessibilityEvent(ui::AXEvent event_type) {
   if (ax_node_)
     ax_node_->NotifyAccessibilityEvent(event_type);
@@ -80,19 +140,25 @@ const ui::AXNodeData& NativeViewAccessibility::GetData() {
                            base::UTF16ToUTF8(description));
 
   if (view_->IsAccessibilityFocusable())
-    data_.state |= (1 << ui::AX_STATE_FOCUSABLE);
+    data_.AddStateFlag(ui::AX_STATE_FOCUSABLE);
 
   if (!view_->enabled())
-    data_.state |= (1 << ui::AX_STATE_DISABLED);
+    data_.AddStateFlag(ui::AX_STATE_DISABLED);
 
   if (!view_->IsDrawn())
-    data_.state |= (1 << ui::AX_STATE_INVISIBLE);
+    data_.AddStateFlag(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 NativeViewAccessibility::GetChildCount() {
-  int child_count = view_->child_count();
+  int child_count = GetUnignoredA11yChildren(view_).size();
 
   std::vector<Widget*> child_widgets;
   PopulateChildWidgetVector(&child_widgets);
@@ -102,19 +168,19 @@ int NativeViewAccessibility::GetChildCount() {
 }
 
 gfx::NativeViewAccessible NativeViewAccessibility::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 NativeViewAccessibility::GetTopLevelWidget() {
@@ -124,8 +190,12 @@ gfx::NativeWindow NativeViewAccessibility::GetTopLevelWidget() {
 }
 
 gfx::NativeViewAccessible NativeViewAccessibility::GetParent() {
-  if (view_->parent())
-    return view_->parent()->GetNativeViewAccessible();
+  View* parent_view = view_->parent();
+  if (parent_view) {
+    if (IsViewA11yIgnored(parent_view))
+      return GetForView(parent_view)->GetParent();
+    return parent_view->GetNativeViewAccessible();
+  }
 
   if (parent_widget_)
     return parent_widget_->GetRootView()->GetNativeViewAccessible();
@@ -134,7 +204,7 @@ gfx::NativeViewAccessible NativeViewAccessibility::GetParent() {
 }
 
 gfx::Vector2d NativeViewAccessibility::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 NativeViewAccessibility::HitTestSync(int x, int y) {
@@ -152,14 +222,20 @@ gfx::NativeViewAccessible NativeViewAccessibility::HitTestSync(int x, int y) {
       return child_root_view->GetNativeViewAccessible();
   }
 
+  if (GetChildCount() == 0) {
+    if (IsViewA11yIgnored(view_))
+      return nullptr;
+    return GetNativeObject();
+  }

[tapted, 2017/02/21 06:01:11]

Can you comment about this - I'm not sure it looks right. E.g.

 - We may return an object when we didn't previously since the HitTestPoint on
line 233 is skipped
 - It seems inconsistent that GetChildCount() above checks for unignored
children, and the loop below checks all child views without the ignored-check.
(e.g. what if there is one unignored child and one ignored child:
GetChildCount() returns 1, but the loop below might hit the ignored child?)

[Patti Lor, 2017/02/27 05:31:04]

Yeah, it's kinda confusing (I had to work through it again). You're right about
both points - for the first, I just moved that check down below line 233.

For the second, I had initially thought this was OK because sometimes we want to
return an ignored child (if it possesses unignored children) to recurse down in,
but turns out this is also the case for ignored leaves. Following it through
completely with AXPlatformodeWin::accHitTest() (its caller), turns out it will
actually conclude there is nothing there and return false. I've refactored this
now (including swapping the visible() check on line 241 to IsDrawn()), and the
tests make more sense now (previously, I was assuming that callers of
HitTestSync() would continue looking if I returned nullptr for an ignored leaf,
which doesn't make sense because even the comment explicitly says it doesn't do
this).

Thanks for picking that up :)

+
   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.
+  // 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

[tapted, 2017/02/21 06:01:11]

was there a comment earlier about how this applies on all platforms (not just
AXPlatformNodeWin)?

[Patti Lor, 2017/02/27 05:31:04]

Yeah, see
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility....

I don't think this is true, so am leaving it as is.

+  // hit test if we return anything other than GetNativeObject() or nullptr.
   for (int i = view_->child_count() - 1; i >= 0; --i) {
     View* child_view = view_->child_at(i);
     if (!child_view->visible())
@@ -241,6 +317,13 @@ void NativeViewAccessibility::SetParentWidget(Widget* parent_widget) {
   parent_widget_->AddObserver(this);
 }
 
+#if !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
+// static
+NativeViewAccessibility* NativeViewAccessibility::Create(View* view) {
+  return new NativeViewAccessibility(view);
+}
+#endif  // !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
+
 void NativeViewAccessibility::PopulateChildWidgetVector(
     std::vector<Widget*>* result_child_widgets) {
   // Only attach child widgets to the root view.