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 e523a61bf250ccd428bf483159e831f3c525c308..babe226e727d603033dd5aa9d8c80a697c8ef8aa 100644
--- a/ui/views/accessibility/native_view_accessibility_base.cc
+++ b/ui/views/accessibility/native_view_accessibility_base.cc
@@ -14,6 +14,56 @@
 
 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. E.g.,
+// LabelButtons contain Labels, but a11y should just show that there's a button.
+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::IsIgnoredView(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::IsIgnoredView(View* view) {

[tapted, 2017/04/05 06:03:33]

Would would happen if we did something like

(ax_platform_node_mac.mm)
// static
bool AXPlatformNode::IsIgnoredNode(gfx::NativeViewAccessible accessible) {
  return [accessible accessibilityIsIgnored];
}

(ax_platform_node_win.cc)
bool AXPlatformNode::IsIgnoredNode(gfx::NativeViewAccessible accessible) {
  return accessible->get_accState() & STATE_SYSTEM_INVISIBLE;
}


(you'd need to update AXPlatformNodeWin::MSAAState() to treat
ui::AX_STATE_INVISIBLE and ui::AX_STATE_IGNORED the same)

[Patti Lor, 2017/05/24 07:29:11]

So the invisible stuff works, but I think since then we've come to the
conclusion that just marking ignore elements as invisible (Win) / redundant
(Linux) / ignored (Mac) is better? (I.e. not try and ignore the elements in
Views' a11y). Let me know if there was a specific reason you wanted Views a11y
to know if a views::View is ignored.

+  NativeViewAccessibilityBase* nva = GetForView(view);
+  if (nva)
+    return nva->GetData().role == ui::AX_ROLE_IGNORED;
+  return false;

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

Comment here like

// If the native accessibility object isn't backed by NativeViewAccessibility
(e.g. when the View wraps a native object), assume it should not be ignored.

[Patti Lor, 2017/04/11 05:40:45]

Done.

+}
+
 NativeViewAccessibilityBase::NativeViewAccessibilityBase(View* view)
     : view_(view),
       parent_widget_(nullptr),
@@ -77,11 +127,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 +146,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 +168,15 @@ 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();
+  while (parent_view && IsIgnoredView(parent_view))
+    parent_view = parent_view->parent();
+  return parent_view ? parent_view->GetNativeViewAccessible()
+                     : 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 +195,36 @@ 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 IsIgnoredView(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;
-
-    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();
+    NativeViewAccessibilityBase* child_nva = GetForView(child_view);
+    if (child_nva) {

[tapted, 2017/04/05 06:03:33]

I think this means that any webview child can't be hit?

AXPlatformNodeWin::accHitTest(..) seems to rely on the NativeViewAccessible
above a webview to return the non-NativeViewAccessible native node from its
webview child in order to do a recursive hit test on the webview.

With this change, I think hit-testing a the NativeViewAccessible above a webview
will now return `this` and prevent the recursion into the webview's native
accessibility object.

That is, previously when doing

if (child_view->HitTestPoint(point_in_child_coords))
   return child_view->GetNativeViewAccessible();

that could return the result of WebView::GetNativeViewAccessible()

but now, since WebView::GetNativeViewAccessible() doesn't return a
NativeViewAccessibilityBase*, the method will return GetNativeObject() instead.

+      gfx::Point point_in_child_coords(point);
+      view_->ConvertPointToTarget(view_, child_view, &point_in_child_coords);
+      if (!child_nva->GetData().HasStateFlag(ui::AX_STATE_INVISIBLE) &&
+          child_view->HitTestPoint(point_in_child_coords)) {
+        if (child_nva->GetChildCount() == 0 && IsIgnoredView(child_view)) {
+          // Multiple levels might be jumped here.
+          return child_nva->GetParent();
+        }
+        return child_nva->GetNativeObject();
+      }
+    }
   }
 
   // If it's not inside any of our children, it's inside this view.
@@ -225,6 +291,28 @@ 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);
+  // WebViews with web contents have their own NativeViewAccessible
+  // implementation, so |node| will be null if |view| is a WebView.
+  if (node)
+    return static_cast<NativeViewAccessibilityBase*>(node->GetDelegate());
+  return nullptr;
+}
+
+gfx::NativeViewAccessible
+NativeViewAccessibilityBase::GetNativeViewAccessibleForWidget() {

[tapted, 2017/04/05 06:03:33]

I think this needs an override in NativeViewAccessiblilityWin as well?

[Patti Lor, 2017/04/11 05:40:45]

I'm confused, can you explain why? A call to NVAWin::GetParent() would call
NVABase::GetParent(), which looks for the View parent, then calls
NVABase::GetNVAccessibleForWidget() to look for the Widget RootView, compared to
having previously called NVABase::GetParent() to firstly look for the View
parent, then the Widget RootView (which is the same)?

+  if (parent_widget_)
+    return parent_widget_->GetRootView()->GetNativeViewAccessible();
+  return nullptr;
+}
+
 gfx::RectF NativeViewAccessibilityBase::GetBoundsInScreen() const {
   return gfx::RectF(view_->GetBoundsInScreen());
 }
@@ -250,17 +338,11 @@ void NativeViewAccessibilityBase::PopulateChildWidgetVector(
     if (widget->GetNativeWindowProperty(kWidgetNativeViewHostKey))
       continue;
 
-    gfx::NativeViewAccessible child_widget_accessible =
-        child_widget->GetRootView()->GetNativeViewAccessible();
-    ui::AXPlatformNode* child_widget_platform_node =
-        ui::AXPlatformNode::FromNativeViewAccessible(child_widget_accessible);
-    if (child_widget_platform_node) {
-      NativeViewAccessibilityBase* child_widget_view_accessibility =
-          static_cast<NativeViewAccessibilityBase*>(
-              child_widget_platform_node->GetDelegate());
-      if (child_widget_view_accessibility->parent_widget() != widget)
-        child_widget_view_accessibility->SetParentWidget(widget);
-    }
+    NativeViewAccessibilityBase* child_widget_view_accessibility =
+        GetForView(child_widget->GetRootView());
+    if (child_widget_view_accessibility &&
+        child_widget_view_accessibility->parent_widget() != widget)
+      child_widget_view_accessibility->SetParentWidget(widget);
 
     result_child_widgets->push_back(child_widget);
   }