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 1d5c11a415de6507e384cb15a305141282dbc192..bb7b61c87470d17da09aacc6a31b1c20e08e8e00 100644
--- a/ui/views/accessibility/native_view_accessibility.cc
+++ b/ui/views/accessibility/native_view_accessibility.cc
@@ -13,12 +13,71 @@
 
 namespace views {
 
-#if !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
+namespace {
+
+typedef std::map<View*, NativeViewAccessibility*> NativeViewAccessibilityMap;

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

Although this feels natural for me... I think the preference is for `using` now.
i.e.

using NativeViewAccessibilityMap = std::map<View*, NativeViewAccessibility*>;

[Patti Lor, 2017/02/21 03:29:16]

Done - sorry, you've told me this before ><

+
 // static

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

remove comment

[Patti Lor, 2017/02/21 03:29:16]

Done.

-NativeViewAccessibility* NativeViewAccessibility::Create(View* view) {
-  return new NativeViewAccessibility(view);
+NativeViewAccessibilityMap& GetNativeViewAccessibilityMap() {
+  CR_DEFINE_STATIC_LOCAL(NativeViewAccessibilityMap, views_to_nva, ());
+  return views_to_nva;
+}
+
+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;
+}
+
+// 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::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 = Create(view);
+  views_to_nva.insert(std::make_pair(view, nva));
+  return nva;
 }

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

since the mapped_type is a primitive type, I think you can just do:

  NativeViewAccessibility*& nva = GetNativeViewAccessibilityMap()[view];
  if (!nva)
    nva = Create(view);
  return nva;

[Patti Lor, 2017/02/21 03:29:16]

Deleted, not needed any more.

-#endif  // !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
 
 NativeViewAccessibility::NativeViewAccessibility(View* view)
     : view_(view),
@@ -32,16 +91,17 @@ NativeViewAccessibility::~NativeViewAccessibility() {
     ax_node_->Destroy();
   if (parent_widget_)
     parent_widget_->RemoveObserver(this);
+
+  // Remove this instance from the map when it's destroyed. Don't bother
+  // checking if the erase was successful because tests may create an instance
+  // directly instead of using GetOrCreate(), so it wouldn't exist in the map.
+  GetNativeViewAccessibilityMap().erase(view_);
 }
 
 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,15 +168,19 @@ 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());

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

i don't know why this static_cast is here - I don't think it's needed

[Patti Lor, 2017/02/21 03:29:16]

Done.

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

We can avoid multiple calls to GetUnignoredA11yChildren here by not calling
GetChildCount() above and below. PopulateChildWidgetVector is already repeated,
so I think it's fine to repeat GetUnignoredA11yChildren as well.

e.g., here, do

std::vector<gfx::NVA> a11y_children = GetUnignored..
int child_view_count = a11y_children.size();
if (index >= child_view_count + child_widget_count)
  return nullptr;  (note: also covers GetChildCount() == 0)

if (index < child_view_count)
  return a11y_children[index];

Widget* child_widget = child_widgets[index - child_view_count];
return child_widget->GetRootView()->GetNativeViewAccessible();

[Patti Lor, 2017/02/21 03:29:16]

Done.

-  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()];
+  int child_view_count = GetChildCount() - child_widget_count;
+  if (index < child_view_count) {
+    return GetUnignoredA11yChildren(view_)[index];
+  } else if (index < (child_view_count + child_widget_count)) {
+    Widget* child_widget = child_widgets[index - child_view_count];
     return child_widget->GetRootView()->GetNativeViewAccessible();
   }
 
@@ -124,8 +194,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 GetOrCreate(parent_view)->GetParent();
+    return parent_view->GetNativeViewAccessible();
+  }
 
   // TODO: move this to NativeViewAccessibilityMac.
 #if defined(OS_MACOSX)
@@ -140,7 +214,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 +232,20 @@ gfx::NativeViewAccessible NativeViewAccessibility::HitTestSync(int x, int y) {
       return child_root_view->GetNativeViewAccessible();
   }
 
+  if (GetChildCount() == 0) {
+    if (IsViewA11yIgnored(view_))
+      return nullptr;
+    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

[dmazzoni, 2017/01/11 07:39:37]

I think all platforms should be doing this, not just AXPlatformNodeWin.
I just checked AXPlatformNodeMac and it seems to be calling
hit test recursively on each child that matches.

[Patti Lor, 2017/02/21 03:29:16]

Do you mean -accessibilityHitTest? It doesn't seem as if it's calling
NVA::HitTestSync though, I think it just has its own implementation which is
recursive. I'm not sure if it should be changed to use HitTestSync though, I
think the difference would be that on Mac Widgets aren't included in the hit
test.

+  // 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())
@@ -247,6 +327,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.