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..098b04b860832a0ea07ed09cc4a4d4edca8c46bb 100644
--- a/ui/views/accessibility/native_view_accessibility.cc
+++ b/ui/views/accessibility/native_view_accessibility.cc
@@ -13,6 +13,25 @@
 
 namespace views {
 
+#if defined(OS_MACOSX)
+namespace {
+
+bool IsAccessibilityFocusableWhenEnabled(View* view) {
+  return view->focus_behavior() != View::FocusBehavior::NEVER &&
+         view->IsDrawn();
+}
+
+bool ViewHasFocusableAncestor(View* view) {
+  while ((view = view->parent())) {
+    if (IsAccessibilityFocusableWhenEnabled(view))
+      return true;
+  }
+  return false;
+}
+
+}  // namespace
+#endif
+
 #if !defined(PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL)
 // static
 NativeViewAccessibility* NativeViewAccessibility::Create(View* view) {
@@ -80,13 +99,22 @@ 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);
+
+#if defined(OS_MACOSX)

[dmazzoni, 2016/11/30 23:12:44]

I'd prefer to find a solution that works for all
platforms. In general hiding children of focusable
views sounds reasonable.

[Patti Lor, 2016/12/01 01:12:07]

See next comment.

+  // Make sure this element is ignored if there's a focusable parent. All
+  // keyboard focusable elements should be leaf nodes in the accessibility tree.
+  // Exceptions to this rule will themselves be accessibility focusable.
+  if (!IsAccessibilityFocusableWhenEnabled(view_) &&
+      ViewHasFocusableAncestor(view_))
+    data_.role = ui::AX_ROLE_UNKNOWN;

[dmazzoni, 2016/11/30 23:12:44]

Rather than setting the role to unknown, what if we just remove them
from the accessibility tree by modifying GetChildCount(), below?

[Patti Lor, 2016/12/01 01:12:07]

Yes - this was my approach in patchset 8. Removing things in the accessibility
tree has a couple implications, if we want to be consistent with the results
returned from GetChildCount(), ChildAtIndex(), GetParent(), etc, which I will
summarise here:
- Children of deleted elements will need to be moved up the tree.
- Hit tests will need to ignore those deleted elements.

The diagram in
https://codereview.chromium.org/2119413004/diff/140001/ui/views/accessibility...
(you'll need to scroll a bit) is probably the most useful in illustrating this.
(We can avoid having to implement this ourselves in this particular patchset
because Mac does this for us with its NSAccessibilityUnignoredAncestor()
function.) To make it platform independent, I could merge this patchset with
patchset 8, but the issue was that it also requires a way to retrieve a
NativeViewAccessibility from a View. I'd originally solved this by adding
View::GetNativeViewAccessibility(), but tapted pointed out this might be
difficult/confusing especially with GetNativeViewAccessible() there as well.
What do you think?

+#endif
 
   return data_;
 }
@@ -140,7 +168,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) {
@@ -163,9 +191,9 @@ gfx::NativeViewAccessible NativeViewAccessibility::HitTestSync(int x, int y) {
   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, 2016/11/30 23:12:44]

Is this specific only to AXPlatformNodeWin?

[Patti Lor, 2016/12/01 01:12:07]

Yes, AFAICT, HitTestSync is only used for Windows (searching it in codesearch
only turns up 1 override in the Linux code which returns nullptr).

+  // 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())