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

ui/views/accessibility/native_view_accessibility_unittest.cc

Index: ui/views/accessibility/native_view_accessibility_unittest.cc
diff --git a/ui/views/accessibility/native_view_accessibility_unittest.cc b/ui/views/accessibility/native_view_accessibility_unittest.cc
index c75ae19e1d3ce103c3c77b5b02647d47da023cb0..9a33434b67a338593583048671645c4cf0e17a44 100644
--- a/ui/views/accessibility/native_view_accessibility_unittest.cc
+++ b/ui/views/accessibility/native_view_accessibility_unittest.cc
@@ -10,6 +10,7 @@
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/test/views_test_base.h"
+#include "ui/views/widget/widget.h"
 
 namespace views {
 namespace test {
@@ -23,6 +24,20 @@ class TestButton : public Button {
   TestButton() : Button(NULL) {}
 };
 
+// Adds a View with given bounds and focusability to a parent. Unused on Linux.
+View* AddNewChildWithBoundsAndFocusability(View& parent,
+                                           gfx::Rect bounds,
+                                           bool focusable) {
+  View* child = new View;
+  child->SetBounds(bounds.x(), bounds.y(), bounds.width(), bounds.height());
+  parent.AddChildView(child);
+  if (focusable)
+    child->SetFocusBehavior(ClientView::FocusBehavior::ACCESSIBLE_ONLY);
+  else
+    child->SetFocusBehavior(ClientView::FocusBehavior::NEVER);
+  return child;
+}
+
 }  // namespace
 
 class NativeViewAccessibilityTest : public ViewsTestBase {
@@ -33,43 +48,52 @@ class NativeViewAccessibilityTest : public ViewsTestBase {
   void SetUp() override {
     ViewsTestBase::SetUp();
 
-    widget_ = new views::Widget;
-    views::Widget::InitParams params =
-        CreateParams(views::Widget::InitParams::TYPE_WINDOW);
+    widget_ = new Widget;
+    Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
     params.bounds = gfx::Rect(0, 0, 200, 200);
     widget_->Init(params);
 
     button_ = new TestButton();
     button_->SetSize(gfx::Size(20, 20));
-    button_accessibility_ = NativeViewAccessibility::Create(button_);
+    button_accessibility_ = NativeViewAccessibility::GetOrCreate(button_);
 
     label_ = new Label();
     button_->AddChildView(label_);
-    label_accessibility_ = NativeViewAccessibility::Create(label_);
+    label_accessibility_ = NativeViewAccessibility::GetOrCreate(label_);
 
     widget_->GetContentsView()->AddChildView(button_);
   }
 
   void TearDown() override {
+    // Closing the Widget will destroy all its children and their associated
+    // NativeViewAccessibility instances.
     if (!widget_->IsClosed())
       widget_->Close();
-    button_accessibility_->Destroy();
-    button_accessibility_ = NULL;
-    label_accessibility_->Destroy();
-    label_accessibility_ = NULL;
+    button_accessibility_ = nullptr;
+    label_accessibility_ = nullptr;
     ViewsTestBase::TearDown();
   }
 
  protected:
-  views::Widget* widget_;
+  Widget* widget_;
   TestButton* button_;
   NativeViewAccessibility* button_accessibility_;
   Label* label_;
   NativeViewAccessibility* label_accessibility_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeViewAccessibilityTest);
 };
 
 TEST_F(NativeViewAccessibilityTest, RoleShouldMatch) {
   EXPECT_EQ(ui::AX_ROLE_BUTTON, button_accessibility_->GetData().role);
+  // Since the label is a subview of |button_|, and the button is keyboard
+  // focusable, the label is assumed to form part of the button and not have a
+  // role of its own.
+  EXPECT_EQ(ui::AX_ROLE_IGNORED, label_accessibility_->GetData().role);
+  // This will happen for all potentially keyboard-focusable Views with
+  // non-keyboard-focusable children, so if we make the button unfocusable, the
+  // label will be allowed to have its own role again.
+  button_->SetFocusBehavior(View::FocusBehavior::NEVER);
   EXPECT_EQ(ui::AX_ROLE_STATIC_TEXT, label_accessibility_->GetData().role);
 }
 
@@ -81,11 +105,16 @@ TEST_F(NativeViewAccessibilityTest, BoundsShouldMatch) {
 }
 
 TEST_F(NativeViewAccessibilityTest, LabelIsChildOfButton) {
+  EXPECT_EQ(0, button_accessibility_->GetChildCount());
+  EXPECT_EQ(nullptr, button_accessibility_->ChildAtIndex(0));
+  EXPECT_EQ(button_->GetNativeViewAccessible(),
+            label_accessibility_->GetParent());
+
+  // If |button_| is no longer focusable, |label_| should show up again.
+  button_->SetFocusBehavior(View::FocusBehavior::NEVER);
   EXPECT_EQ(1, button_accessibility_->GetChildCount());
   EXPECT_EQ(label_->GetNativeViewAccessible(),
             button_accessibility_->ChildAtIndex(0));
-  EXPECT_EQ(button_->GetNativeViewAccessible(),
-            label_accessibility_->GetParent());
 }
 
 TEST_F(NativeViewAccessibilityTest, WritableFocus) {
@@ -126,7 +155,7 @@ class TestNativeViewAccessibility : public NativeViewAccessibility {
 TEST_F(NativeViewAccessibilityTest, CrashOnWidgetDestroyed) {
   std::unique_ptr<Widget> parent_widget(new Widget);
   Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
-  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
   params.bounds = gfx::Rect(50, 50, 650, 650);
   parent_widget->Init(params);
 
@@ -143,5 +172,231 @@ TEST_F(NativeViewAccessibilityTest, CrashOnWidgetDestroyed) {
   parent_widget.reset();
 }
 
+// Separate test class for a complicated test case for ignored accessibility
+// elements which are excluded from the accessibility tree. On Linux, these
+// elements are not actually excluded, only marked as having an ignored role, so
+// don't run this test case on Linux.
+class NativeViewAccessibilityIgnoredElementsTest : public ViewsTestBase {
+ public:
+  NativeViewAccessibilityIgnoredElementsTest() {}
+  ~NativeViewAccessibilityIgnoredElementsTest() override {}
+
+  void SetUp() override {
+    ViewsTestBase::SetUp();
+    // Set up an accessibility tree where X-nodes should be ignored. A Views
+    // hierarchy as shown on the left should expose the accessibility tree in
+    // the middle, and respond to hit tests as shown on the right. This test
+    // is here for completeness (in practice, it's unlikely a keyboard-focusable
+    // View will be needed as a descendant of another keyboard-focusable View).
+    //
+    //           A                    A

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

I wonder if there's a naming scheme that could make the tests clearer. E.g.

 A -> root
 B -> b1a
X1 -> b2x
 C -> b1a_c1a
 D -> b1a_c2a
X2 -> b1a_c3x
X3 -> b2x_c4x
X4 -> b2x_c5x
 E -> b2x_c6a
X5 -> b1a_c1a_d1x
 F -> b2x_c4x_d2a

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

Yeah, that sounds like a good idea, but I'd like to preserve the single letters
as well so that the tree/hit test diagrams don't get cluttered. I've tried using
a compromise where all of them are prefixed with their letter, but also have the
parents written in. I didn't know what the "a" at the end of them meant though
(other than "not ignored") so I left them out. The numbers I also left out
because they are confusing when you still have nodes named x1, x2 etc.

Let me know if you think that's better or not :/ I'm not sure.

+    //         /   \                / | \        +++---+---+---++---+---+---+++
+    //       B       X1            B  F  E       ||| C | F | B || F | A | E |||

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

||| C | F | B || F | A | E |||

should that first F be a D?

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

Yes, thanks for picking that up :)

+    //     / | \    / | \         / \            +++---+---+---++---+---+---+++
+    //    C  D X2  X3 X4 E       C   D
+    //    |        |
+    //   X5        F
+    //
+    // Note: For hit tests, the parent's bounds will be divided equally amongst
+    // its children (including ignored elements). This means all Views belonging
+    // on the same level of the Views hierarchy will be the same size.
+    a_ = new View;
+    a_->SetSize(gfx::Size(400, 20));
+    a_->SetFocusBehavior(ClientView::FocusBehavior::ACCESSIBLE_ONLY);
+
+    // Create Views in the left subtree from root |a_|.
+    gfx::Rect new_bounds(0, 0, a_->width() / 2, a_->height());
+    b_ = AddNewChildWithBoundsAndFocusability(*a_, new_bounds, true);
+    // |a_| has 6 leaves, so divvy up space equally between them for hit tests.
+    new_bounds = gfx::Rect(0, 0, a_->width() / 6, a_->height());
+    c_ = AddNewChildWithBoundsAndFocusability(*b_, new_bounds, true);
+    x5_ = AddNewChildWithBoundsAndFocusability(*c_, new_bounds, false);
+    // Update the |new_bounds| origin to position leaves next to each other.
+    new_bounds.set_x(new_bounds.x() + a_->width() / 6);
+    d_ = AddNewChildWithBoundsAndFocusability(*b_, new_bounds, true);
+    new_bounds.set_x(new_bounds.x() + a_->width() / 6);
+    x2_ = AddNewChildWithBoundsAndFocusability(*b_, new_bounds, false);
+    new_bounds.set_x(new_bounds.x() + a_->width() / 6);
+
+    // Create Views in the right subtree from root |a_|.
+    new_bounds.set_size(b_->size());
+    x1_ = AddNewChildWithBoundsAndFocusability(*a_, new_bounds, false);
+    new_bounds.set_origin(gfx::Point());
+    x3_ = AddNewChildWithBoundsAndFocusability(*x1_, new_bounds, false);
+    f_ = AddNewChildWithBoundsAndFocusability(*x3_, new_bounds, true);
+    new_bounds.set_x(new_bounds.x() + a_->width() / 6);
+    x4_ = AddNewChildWithBoundsAndFocusability(*x1_, new_bounds, false);
+    new_bounds.set_x(new_bounds.x() + a_->width() / 6);
+    e_ = AddNewChildWithBoundsAndFocusability(*x1_, new_bounds, true);
+
+    // Populate NativeViewAccessibility instances for convenience.
+    a_ax_ = NativeViewAccessibility::GetOrCreate(a_);
+    b_ax_ = NativeViewAccessibility::GetOrCreate(b_);
+    c_ax_ = NativeViewAccessibility::GetOrCreate(c_);
+    d_ax_ = NativeViewAccessibility::GetOrCreate(d_);
+    e_ax_ = NativeViewAccessibility::GetOrCreate(e_);
+    f_ax_ = NativeViewAccessibility::GetOrCreate(f_);
+    x1_ax_ = NativeViewAccessibility::GetOrCreate(x1_);
+    x2_ax_ = NativeViewAccessibility::GetOrCreate(x2_);
+    x3_ax_ = NativeViewAccessibility::GetOrCreate(x3_);
+    x4_ax_ = NativeViewAccessibility::GetOrCreate(x4_);
+    x5_ax_ = NativeViewAccessibility::GetOrCreate(x5_);
+
+    // Add everything to a new widget, needed for hit testing and parent tests.
+    widget_ = new Widget;
+    Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+    widget_->Init(params);
+    widget_->GetContentsView()->AddChildView(a_);
+    widget_->Show();
+  }
+
+  void TearDown() override {
+    widget_->Close();
+    ViewsTestBase::TearDown();
+  }
+
+ protected:
+  // Don't use std::unique_ptr for the following Views - everything will be
+  // inside a widget, which will handle their deletion.

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

You can just say

// Owned by |widget_|.

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

Done.

+  View* a_;
+  View* b_;
+  View* c_;
+  View* d_;
+  View* e_;
+  View* f_;
+  View* x1_;
+  View* x2_;
+  View* x3_;
+  View* x4_;
+  View* x5_;
+
+  // Destroyed when the View they are attached to is destroyed.
+  NativeViewAccessibility* a_ax_;
+  NativeViewAccessibility* b_ax_;
+  NativeViewAccessibility* c_ax_;
+  NativeViewAccessibility* d_ax_;
+  NativeViewAccessibility* e_ax_;
+  NativeViewAccessibility* f_ax_;
+  NativeViewAccessibility* x1_ax_;
+  NativeViewAccessibility* x2_ax_;
+  NativeViewAccessibility* x3_ax_;
+  NativeViewAccessibility* x4_ax_;
+  NativeViewAccessibility* x5_ax_;
+
+  Widget* widget_ = nullptr;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NativeViewAccessibilityIgnoredElementsTest);
+};
+
+TEST_F(NativeViewAccessibilityIgnoredElementsTest, GetChildCount) {

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

nit: all these should have a brief comment before

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

Done.

+  EXPECT_EQ(3, a_ax_->GetChildCount());
+  EXPECT_EQ(2, b_ax_->GetChildCount());
+  EXPECT_EQ(0, c_ax_->GetChildCount());

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

Perhaps just comment that this is a leaf? Then I think the IsLeafElement test is
redundant - it's not adding anything new (unless I've missed something)

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

Done.

+  EXPECT_EQ(0, x5_ax_->GetChildCount());
+  EXPECT_EQ(0, d_ax_->GetChildCount());
+  EXPECT_EQ(0, x2_ax_->GetChildCount());
+  EXPECT_EQ(2, x1_ax_->GetChildCount());
+  EXPECT_EQ(1, x3_ax_->GetChildCount());
+  EXPECT_EQ(0, f_ax_->GetChildCount());
+  EXPECT_EQ(0, x4_ax_->GetChildCount());
+  EXPECT_EQ(0, e_ax_->GetChildCount());
+}
+
+TEST_F(NativeViewAccessibilityIgnoredElementsTest, ChildAtIndex) {
+  EXPECT_EQ(b_->GetNativeViewAccessible(), a_ax_->ChildAtIndex(0));
+  EXPECT_EQ(f_->GetNativeViewAccessible(), a_ax_->ChildAtIndex(1));
+  EXPECT_EQ(e_->GetNativeViewAccessible(), a_ax_->ChildAtIndex(2));
+  EXPECT_EQ(nullptr, a_ax_->ChildAtIndex(3));
+
+  EXPECT_EQ(c_->GetNativeViewAccessible(), b_ax_->ChildAtIndex(0));
+  EXPECT_EQ(d_->GetNativeViewAccessible(), b_ax_->ChildAtIndex(1));
+  EXPECT_EQ(nullptr, b_ax_->ChildAtIndex(2));
+
+  EXPECT_EQ(f_->GetNativeViewAccessible(), x1_ax_->ChildAtIndex(0));
+  EXPECT_EQ(e_->GetNativeViewAccessible(), x1_ax_->ChildAtIndex(1));
+  EXPECT_EQ(nullptr, x1_ax_->ChildAtIndex(2));
+
+  EXPECT_EQ(f_->GetNativeViewAccessible(), x3_ax_->ChildAtIndex(0));
+
+  // Node with ignored children.
+  EXPECT_EQ(nullptr, c_ax_->ChildAtIndex(0));
+  // Node with 0 children.
+  EXPECT_EQ(nullptr, d_ax_->ChildAtIndex(0));
+  // Ignored node with 0 children.
+  EXPECT_EQ(nullptr, x2_ax_->ChildAtIndex(0));
+}
+
+TEST_F(NativeViewAccessibilityIgnoredElementsTest, GetParent) {
+  EXPECT_EQ(a_->parent()->GetNativeViewAccessible(), a_ax_->GetParent());
+  EXPECT_EQ(a_->GetNativeViewAccessible(), b_ax_->GetParent());
+  EXPECT_EQ(b_->GetNativeViewAccessible(), c_ax_->GetParent());
+  EXPECT_EQ(c_->GetNativeViewAccessible(), x5_ax_->GetParent());
+  EXPECT_EQ(b_->GetNativeViewAccessible(), d_ax_->GetParent());
+  EXPECT_EQ(b_->GetNativeViewAccessible(), x2_ax_->GetParent());
+
+  EXPECT_EQ(a_->GetNativeViewAccessible(), x1_ax_->GetParent());
+  EXPECT_EQ(a_->GetNativeViewAccessible(), x3_ax_->GetParent());
+  EXPECT_EQ(a_->GetNativeViewAccessible(), x4_ax_->GetParent());
+  EXPECT_EQ(a_->GetNativeViewAccessible(), e_ax_->GetParent());
+  EXPECT_EQ(a_->GetNativeViewAccessible(), f_ax_->GetParent());

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

These need comments to describe what's happening. A better naming scheme may
help. Then you can just add something like

// Skips x3 and x1.

The name will help make it clear what the direct parent is.

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

Done.

+}
+
+TEST_F(NativeViewAccessibilityIgnoredElementsTest, IsLeafElement) {
+  EXPECT_FALSE(a_ax_->GetChildCount() == 0);

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

EXPECT_NE  (or _EQ with the actual number?)

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

Deleted this test as you suggested previously.

+  EXPECT_FALSE(b_ax_->GetChildCount() == 0);
+  EXPECT_TRUE(c_ax_->GetChildCount() == 0);

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

EXPECT_EQ

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

Acknowledged.

+  EXPECT_TRUE(d_ax_->GetChildCount() == 0);
+  EXPECT_TRUE(e_ax_->GetChildCount() == 0);
+  EXPECT_TRUE(f_ax_->GetChildCount() == 0);
+  EXPECT_FALSE(x1_ax_->GetChildCount() == 0);
+  EXPECT_TRUE(x2_ax_->GetChildCount() == 0);
+  EXPECT_FALSE(x3_ax_->GetChildCount() == 0);
+  EXPECT_TRUE(x4_ax_->GetChildCount() == 0);
+  EXPECT_TRUE(x5_ax_->GetChildCount() == 0);
+}
+
+TEST_F(NativeViewAccessibilityIgnoredElementsTest, HitTestSync) {
+  // Move the hit point horizontally, incrementing by the leaf View width.
+  int min_width = a_->GetBoundsInScreen().width() / 6;

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

const?

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

Done.

+  int curr_x = a_->GetBoundsInScreen().x() + min_width / 2;
+  const int y = a_->GetBoundsInScreen().CenterPoint().y();
+
+  // HitTestSync isn't recursive, so manually do the recursion until arriving at

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

How about a helper that calls HitTestSync until reaching a leaf?

Also, in case it wasn't clear, the reason it's not recursive is
because sometimes the hit testing will lead you to something
other than a View - for example if you hit test in web contents,
we first return the WebView, but then the WebView's accessible is
a totally different object like a BrowserAccessibilityCocoa,
which implements its own hit testing from then on.

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

I'm not sure the helper is a good idea - it still needs a list of expected NVAs
it will pass through on trying to get to the leaf because every call to
HitTestSync() returns a NativeViewAccessible, and there's no way to retrieve a
NativeViewAccessibility from a NativeViewAccessible on all platforms. You'd have
to check at each step that the NVA->NativeObject() returned from the expected
NVA list matches up with each NativeViewAccessible returned by HitTestSync.

Ah, thanks for the clarification, I had just assumed it was that way because of
a Windows specific implementation of hit testing or something.

+  // the correct answer. I.e., the last NativeViewAccessible being tested for
+  // each |curr_x|, |y| pair below would be the correct return value if
+  // HitTestSync were recursive.
+  EXPECT_EQ(b_->GetNativeViewAccessible(), a_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(c_->GetNativeViewAccessible(), b_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(c_->GetNativeViewAccessible(), c_ax_->HitTestSync(curr_x, y));
+  curr_x += min_width;
+  EXPECT_EQ(b_->GetNativeViewAccessible(), a_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(d_->GetNativeViewAccessible(), b_ax_->HitTestSync(curr_x, y));
+  curr_x += min_width;
+  EXPECT_EQ(b_->GetNativeViewAccessible(), a_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(x2_->GetNativeViewAccessible(), b_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(b_->GetNativeViewAccessible(), x2_ax_->HitTestSync(curr_x, y));
+  curr_x += min_width;
+  EXPECT_EQ(x1_->GetNativeViewAccessible(), a_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(x3_->GetNativeViewAccessible(), x1_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(f_->GetNativeViewAccessible(), x3_ax_->HitTestSync(curr_x, y));
+  curr_x += min_width;
+  EXPECT_EQ(x1_->GetNativeViewAccessible(), a_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(x4_->GetNativeViewAccessible(), x1_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(a_->GetNativeViewAccessible(), x4_ax_->HitTestSync(curr_x, y));
+  curr_x += min_width;
+  EXPECT_EQ(x1_->GetNativeViewAccessible(), a_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(e_->GetNativeViewAccessible(), x1_ax_->HitTestSync(curr_x, y));
+
+  // If |e_| and |a_| are no longer focusable, |a_|, |x1_|, and |e_| become
+  // are no longer ignored and |x1_| is revealed in hit testing (covers up the
+  // the hit coordinates occupied by |a_|).
+  a_->SetFocusBehavior(ClientView::FocusBehavior::NEVER);
+  e_->SetFocusBehavior(ClientView::FocusBehavior::NEVER);
+  EXPECT_EQ(x1_->GetNativeViewAccessible(), a_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(e_->GetNativeViewAccessible(), x1_ax_->HitTestSync(curr_x, y));
+  EXPECT_EQ(e_->GetNativeViewAccessible(), e_ax_->HitTestSync(curr_x, y));
+}
+
 }  // namespace test
 }  // namespace views