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 7c960348478b020d7b0bfeed5adf0f50f45e5d70..9ca21840db8c6abfc22b9a660acabff361951fed 100644
--- a/ui/views/accessibility/native_view_accessibility.cc
+++ b/ui/views/accessibility/native_view_accessibility.cc
@@ -13,12 +13,47 @@
 
 namespace views {
 
-#if !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
+namespace {
+
+typedef std::map<View*, NativeViewAccessibility*> NativeViewAccessibilityMap;
+
+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::GetOrCreate(view)->GetData().role ==
+         ui::AX_ROLE_IGNORED;
+}
+
+}  // namespace
+
 // static
-NativeViewAccessibility* NativeViewAccessibility::Create(View* view) {
-  return new NativeViewAccessibility(view);
+NativeViewAccessibility* NativeViewAccessibility::GetOrCreate(View* view) {
+  NativeViewAccessibilityMap* views_to_nva = GetNativeViewAccessibilityMap();
+  NativeViewAccessibilityMap::iterator it = views_to_nva->find(view);
+  if (it != views_to_nva->end())
+    return (*it).second;
+  NativeViewAccessibility* nva = CreateNewNativeViewAccessibilityImpl(view);
+  views_to_nva->insert(NativeViewAccessibilityMap::value_type(view, nva));
+  return nva;
 }
-#endif  // !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
 
 NativeViewAccessibility::NativeViewAccessibility(View* view)
     : view_(view),
@@ -32,6 +67,16 @@ NativeViewAccessibility::~NativeViewAccessibility() {
     ax_node_->Destroy();
   if (parent_widget_)
     parent_widget_->RemoveObserver(this);
+
+  // Remove this instance from the map when it's destroyed.

[tapted, 2016/12/22 02:56:36]

In theory this should occur in NativeViewAccessibility::Destroy() -- that's what
~View() calls, and ~NativeViewAccessibility() may not be invoked immediately.
But I don't think we need NativeViewAccessibility::Destroy() any more.

I think the first step is to remove NativeViewAccessibility::Destroy().
views::View should have a std::unique_ptr to NativeViewAccessibility rather than
a raw pointer.

[Patti Lor, 2017/01/11 02:01:48]

Done.

+  NativeViewAccessibilityMap* views_to_nva = GetNativeViewAccessibilityMap();

[tapted, 2016/12/22 02:56:36]

We can just call 

GetNativeViewAccessibilityMap()->erase(view_);

That will return 0 if it's not found, but since we don't guarantee that, we can
just add a note above.

[Patti Lor, 2017/01/11 02:01:47]

Oops, thanks for the tip!

+  NativeViewAccessibilityMap::iterator it = views_to_nva->find(view_);
+  if (it != views_to_nva->end()) {
+    views_to_nva->erase(it);
+  } else {
+    // This should only ever happen in tests where the NativeViewAccessibility

[dmazzoni, 2016/12/28 18:02:35]

Are there any such tests? It might be nice to fix them and add
NOTREACHED here, unless it's a major pain to refactor.

[Patti Lor, 2017/01/11 02:01:47]

Yes, NativeViewAccessibilityTest.CrashOnWidgetDestroyed uses a custom
NativeViewAccessibility subclass and won't be able to use GetOrCreate() to get
an instance of that. I did try to fix it, but I don't think there's an easy
solution, since NVA::Create() returns a platform specific subclass of NVA, and
is guarded on the PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL hash define. To
give NVATest its own constructor that avoids inserting it into the map would
mean we'd have to define PLATFORM_HAS_NVA... for unit tests somehow.

+    // instance was not created via GetOrCreate().
+  }
 }
 
 gfx::NativeViewAccessible NativeViewAccessibility::GetNativeObject() {
@@ -80,19 +125,34 @@ 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_->visible())
-    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 = 0;
+  for (int i = 0; i < view_->child_count(); ++i) {
+    View* child = view_->child_at(i);
+    if (IsViewA11yIgnored(child)) {
+      // If not visible to accessibility clients, use its child count instead.
+      child_count += GetOrCreate(child)->GetChildCount();
+    } else {
+      ++child_count;
+    }
+  }
 
   std::vector<Widget*> child_widgets;
   PopulateChildWidgetVector(&child_widgets);
@@ -102,14 +162,30 @@ int NativeViewAccessibility::GetChildCount() {
 }
 
 gfx::NativeViewAccessible NativeViewAccessibility::ChildAtIndex(int index) {
-  // If this is a root view, our widget might have child widgets. Include
+  if (GetChildCount() == 0)
+    return nullptr;
+
+  // 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) {
+  int ax_child_count = GetChildCount();
+  if (index < ax_child_count - child_widget_count) {

[tapted, 2016/12/22 02:56:36]

This logic is pretty complicated.. I think we effectively want
`NSAccessibilityUnignoredChildren`
https://developer.apple.com/reference/appkit/1524571-nsaccessibilityunignored...

I think my preference would be to create a helper function that returns a
freshly-populated std::vector<gfx::NativeViewAccessible> of unignored children
each time.

It feels slow.. but it shouldn't be too bad, and the existence of
NSAccessibilityUnignoredChildren justifies it to some extent.

GetChildCount() then just returns GetUnignoredChildren().size(). and things are
a lot simpler here.

::HitTestSync() can be updated to use this helper too (be sure to put child
widgets in first)

[Patti Lor, 2017/01/11 02:01:47]

Done. I'm not sure if HitTestSync can be updated to use this though, since it
uses View::HitTestPoint() to do hit testing and there isn't a way to get the
View from a gfx::NativeViewAccessible.

+    int curr_child_count = 0;
+    for (int i = 0; i < view_->child_count(); ++i) {
+      View* child = view_->child_at(i);
+      bool ignored = IsViewA11yIgnored(child);
+      int pending_children = ignored ? GetOrCreate(child)->GetChildCount() : 1;
+      if (index < curr_child_count + pending_children) {
+        if (ignored) {
+          return GetOrCreate(child)->ChildAtIndex(index - curr_child_count);
+        }
+        return child->GetNativeViewAccessible();
+      }
+      curr_child_count += pending_children;
+    }
+  } else if (index < ax_child_count) {
     Widget* child_widget = child_widgets[index - view_->child_count()];

[dmazzoni, 2016/12/28 18:02:35]

This isn't correct anymore - taking [index - view_->child_count()] assumes that
the number of children other than widgets is equal to view_->child_count() but
it could be greater or less than that now.

[Patti Lor, 2017/01/11 02:01:47]

Thanks - updated to index - ax_child_count.

     return child_widget->GetRootView()->GetNativeViewAccessible();
   }
@@ -124,8 +200,13 @@ 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 GetOrCreate(parent_view)->GetParent();
+    else

[tapted, 2016/12/22 02:56:36]

nit: "no else after return" -
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[Patti Lor, 2017/01/11 02:01:48]

Done, thanks for always digging up the style guide links!

+      return parent_view->GetNativeViewAccessible();
+  }
 
   // TODO: move this to NativeViewAccessibilityMac.
 #if defined(OS_MACOSX)
@@ -140,7 +221,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) {
@@ -158,14 +239,20 @@ gfx::NativeViewAccessible NativeViewAccessibility::HitTestSync(int x, int y) {
       return child_root_view->GetNativeViewAccessible();
   }
 
+  if (GetChildCount() == 0) {
+    if (IsViewA11yIgnored(view_))
+      return GetParent();

[dmazzoni, 2016/12/28 18:02:35]

I think you should return nullptr here.

HitTest is sometimes called repeatedly - do a hit test
on the top-level window, get a child - then call HitTest
on that again to get its child. Returning a parent could
result in an endless loop.

[Patti Lor, 2017/01/11 02:01:47]

Done.

+    return GetNativeObject();
+  }
+
   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
+  // hit test if we return anything other than GetNativeObject() or nullptr.
   for (int i = view_->child_count() - 1; i >= 0; --i) {

[tapted, 2016/12/22 02:56:36]

Does this need to consider whether the child is ignored?

[Patti Lor, 2017/01/11 02:01:48]

No - skipping ignored children at this point of the hit test will skip the
entire subtree. You'd fix that by making this method recursive, but since this
was designed not to recurse I'm leaving it that way. The hunk just above this
(line 242) should take care of ignored elements I think.

     View* child_view = view_->child_at(i);
     if (!child_view->visible())
@@ -247,6 +334,21 @@ void NativeViewAccessibility::SetParentWidget(Widget* parent_widget) {
   parent_widget_->AddObserver(this);
 }
 
+// static
+NativeViewAccessibilityMap*

[tapted, 2016/12/22 02:56:36]

nit: Return a reference (sometimes returning a pointer suggests that it might
return nullptr, but it never will)

[Patti Lor, 2017/01/11 02:01:47]

Done.

+NativeViewAccessibility::GetNativeViewAccessibilityMap() {

[tapted, 2016/12/22 02:56:36]

can this go in an anonymous namespace?

[Patti Lor, 2017/01/11 02:01:47]

Done - I assumed you also meant to make it a function as well (as opposed to a
method), so let me know if that was wrong.

[tapted, 2017/01/11 18:27:39]

yep

+  CR_DEFINE_STATIC_LOCAL(NativeViewAccessibilityMap, views_to_nva, ());
+  return &views_to_nva;
+}
+
+#if !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
+// static
+NativeViewAccessibility*
+NativeViewAccessibility::CreateNewNativeViewAccessibilityImpl(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.