Remove ignored views from the accessibility tree visible to platform APIs

This patch is the first, (not user visible), fix for a bug with the reading of lines in NVDA.
When the screen reader focus is in the accessibility tree for Views, such as in a context menu, a toolbar
or the main application's menu, the "Read Line" command (NVDAKey+Up) doesn't always read the currently focused item.

We were exposing ignored views in the accessibility tree that was exposed to platform APIs.
As a result, platform specific code under ui/accessibility was building IA2 and ATK hypertext for ignored nodes,
while text navigation code in the same component was trying to locate non-existent line boundaries in that hypertext.

A followup patch will ensure that the names of Views that come from their contents
are clearly marked as such.

R=dmazzoni@chromium.org, aleventhal@chromium.org

AX-Relnotes: n/a.

Change-Id: I5fdae5714b10b10db3dc5b5dcf22d118f322e9e7
Bug: 1098528
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2435645
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828873}

ui/views/accessibility/ax_virtual_view.cc

@@ -256,32 +256,34 @@ int AXVirtualView::GetChildCount() const {
   for (const std::unique_ptr<AXVirtualView>& child : children_) {
     if (child->IsIgnored()) {
       count += child->GetChildCount();
-      continue;
+    } else {
+      ++count;
     }
-    count++;
   }
   return count;
 }
 
 gfx::NativeViewAccessible AXVirtualView::ChildAtIndex(int index) {
-  DCHECK_GE(index, 0) << "Child indices should be greater or equal to 0.";
+  DCHECK_GE(index, 0) << "|index| should be greater or equal to 0.";
   DCHECK_LT(index, GetChildCount())
-      << "Child indices should be less than the child count.";
-  int i = 0;
+      << "|index| should be less than the child count.";
+
   for (const std::unique_ptr<AXVirtualView>& child : children_) {
     if (child->IsIgnored()) {
-      if (index - i < child->GetChildCount()) {
-        gfx::NativeViewAccessible result = child->ChildAtIndex(index - i);
-        if (result)
-          return result;
-      }
-      i += child->GetChildCount();
-      continue;
-    }
-    if (i == index)
+      int child_count = child->GetChildCount();
+      if (index < child_count)
+        return child->ChildAtIndex(index);
+      index -= child_count;
+    } else {
+      if (index == 0)
         return child->GetNativeObject();
-    i++;
+      --index;
     }
+
+    DCHECK_GE(index, 0) << "|index| should be less than the child count.";
+  }
+
+  NOTREACHED() << "|index| should be less than the child count.";
   return nullptr;
 }
 
@@ -297,8 +299,11 @@ gfx::NativeViewAccessible AXVirtualView::GetNativeViewAccessible() {
 }
 
 gfx::NativeViewAccessible AXVirtualView::GetParent() {
-  if (parent_view_)
+  if (parent_view_) {
+    if (!parent_view_->IsIgnored())
       return parent_view_->GetNativeObject();
+    return GetDelegate()->GetParent();
+  }
 
   if (virtual_parent_view_) {
     if (virtual_parent_view_->IsIgnored())
@@ -417,8 +422,8 @@ const ui::AXUniqueId& AXVirtualView::GetUniqueId() const {
   return unique_id_;
 }
 
-// Virtual views need to implement this function in order for A11Y events
-// to be routed correctly.
+// Virtual views need to implement this function in order for accessibility
+// events to be routed correctly.
 gfx::AcceleratedWidget AXVirtualView::GetTargetForNativeAccessibilityEvent() {
 #if defined(OS_WIN)
   if (GetOwnerView())

ui/views/accessibility/ax_virtual_view.h

@@ -165,14 +165,19 @@ class VIEWS_EXPORT AXVirtualView : public ui::AXPlatformNodeDelegateBase {
   // Gets the real View that owns our shallowest virtual ancestor,, if any.
   View* GetOwnerView() const;
 
-  // Gets the view platform delegate if exists, otherwise nullptr.
+  // Gets the delegate for our owning View; if we are on a platform that exposes
+  // Views directly to platform APIs instead of serializing them into an AXTree.
+  // Otherwise, returns nullptr.
   ViewAXPlatformNodeDelegate* GetDelegate() const;
 
   // Gets or creates a wrapper suitable for use with tree sources.
   AXVirtualViewWrapper* GetOrCreateWrapper(views::AXAuraObjCache* cache);
 
   // Returns true if this node is ignored and should be hidden from the
-  // accessibility tree. This does not impact the node's descendants.
+  // accessibility tree. Methods that are used to navigate the accessibility
+  // tree, such as "ChildAtIndex", "GetParent", and "GetChildCount", among
+  // others, also skip ignored nodes. This does not impact the node's
+  // descendants.
   bool IsIgnored() const;
 
   // Handle a request from assistive technology to perform an action on this

ui/views/accessibility/view_accessibility.h

@@ -97,7 +97,11 @@ class VIEWS_EXPORT ViewAccessibility {
   Widget* GetNextFocus();
   Widget* GetPreviousFocus();
 
+  // Returns the accessibility object that represents the View whose
+  // accessibility is managed by this instance. This may be an AXPlatformNode or
+  // it may be a native accessible object implemented by another class.
   virtual gfx::NativeViewAccessible GetNativeObject() const;
+
   virtual void NotifyAccessibilityEvent(ax::mojom::Event event_type);
 
   // Causes the screen reader to announce |text|. If the current user is not

ui/views/accessibility/view_ax_platform_node_delegate.h

@@ -61,6 +61,9 @@ class ViewAXPlatformNodeDelegate : public ViewAccessibility,
   gfx::NativeViewAccessible ChildAtIndex(int index) override;
   bool HasModalDialog() const override;
   gfx::NativeViewAccessible GetNSWindow() override;
+  // TODO(nektar): Make "GetNativeViewAccessible" a const method throughout the
+  // codebase.
+  gfx::NativeViewAccessible GetNativeViewAccessible() const;
   gfx::NativeViewAccessible GetNativeViewAccessible() override;
   gfx::NativeViewAccessible GetParent() override;
   bool IsChildOfLeaf() const override;
@@ -104,15 +107,35 @@ class ViewAXPlatformNodeDelegate : public ViewAccessibility,
   ui::AXPlatformNode* ax_platform_node() { return ax_platform_node_; }
 
  private:
+  struct ChildWidgetsResult final {
+    ChildWidgetsResult();
+    ChildWidgetsResult(std::vector<Widget*> child_widgets,
+                       bool is_tab_modal_showing);
+    ChildWidgetsResult(const ChildWidgetsResult& other);
+    virtual ~ChildWidgetsResult();
+    ChildWidgetsResult& operator=(const ChildWidgetsResult& other);
+
+    std::vector<Widget*> child_widgets;
+
+    // When the focus is within a child widget, |child_widgets| contains only
+    // that widget. Otherwise, |child_widgets| contains all child widgets.
+    //
+    // The former arises when a modal dialog is showing. In order to support the
+    // "read title (NVDAKey+T)" and "read window (NVDAKey+B)" commands in the
+    // NVDA screen reader, we need to hide the rest of the UI from the
+    // accessibility tree for these commands to work properly.
+    bool is_tab_modal_showing = false;
+  };
+
   // Uses Views::GetViewsInGroup to find nearby Views in the same group.
   // Searches from the View's parent to include siblings within that group.
   void GetViewsInGroupForSet(std::vector<View*>* views_in_group) const;
 
-  struct ChildWidgetsResult;
-
+  // If this delegate is attached to the root view, returns all the child
+  // widgets of this view's owning widget.
   ChildWidgetsResult GetChildWidgets() const;
 
-  // Gets the real TableView, otherwise nullptr.
+  // Gets the real (non-virtual) TableView, otherwise nullptr.
   TableView* GetAncestorTableView() const;
 
   // We own this, but it is reference-counted on some platforms so we can't use

ui/views/accessibility/view_ax_platform_node_delegate_auralinux.cc

@@ -187,8 +187,9 @@ ViewAXPlatformNodeDelegateAuraLinux::ViewAXPlatformNodeDelegateAuraLinux(
 
 gfx::NativeViewAccessible ViewAXPlatformNodeDelegateAuraLinux::GetParent() {
   if (gfx::NativeViewAccessible parent =
-          ViewAXPlatformNodeDelegate::GetParent())
+          ViewAXPlatformNodeDelegate::GetParent()) {
     return parent;
+  }
 
   Widget* parent_widget =
       GetWidgetOfParentWindowIncludingTransient(view()->GetWidget());

ui/views/accessibility/view_ax_platform_node_delegate_mac.mm

@@ -37,7 +37,7 @@ gfx::NativeViewAccessible ViewAXPlatformNodeDelegateMac::GetNSWindow() {
 
 gfx::NativeViewAccessible ViewAXPlatformNodeDelegateMac::GetParent() {
   if (view()->parent())
-    return view()->parent()->GetNativeViewAccessible();
+    return ViewAXPlatformNodeDelegate::GetParent();
 
   auto* widget = view()->GetWidget();
   if (!widget)

ui/views/accessibility/view_ax_platform_node_delegate_unittest.cc

@@ -423,35 +423,139 @@ TEST_F(ViewAXPlatformNodeDelegateTest, SetSizeAndPosition) {
   EXPECT_EQ(view_accessibility(group_ids[4])->GetPosInSet(), 1);
 }
 
-TEST_F(ViewAXPlatformNodeDelegateTest, Navigation) {
-  View::Views view_ids = SetUpExtraViews();
+TEST_F(ViewAXPlatformNodeDelegateTest, TreeNavigation) {
+  // Adds one extra parent view with four child views to our widget. The parent
+  // view is added as the next sibling of the already present button view.
+  //
+  // Widget
+  // ++Button
+  // ++++Label
+  // 0 = ++ParentView
+  // 1 = ++++ChildView1
+  // 2 = ++++ChildView2
+  // 3 = ChildView3
+  // 4 = ChildView4
+  View::Views extra_views = SetUpExtraViews();
+  ViewAXPlatformNodeDelegate* parent_view = view_accessibility(extra_views[0]);
+  ViewAXPlatformNodeDelegate* child_view_1 = view_accessibility(extra_views[1]);
+  ViewAXPlatformNodeDelegate* child_view_2 = view_accessibility(extra_views[2]);
+  ViewAXPlatformNodeDelegate* child_view_3 = view_accessibility(extra_views[3]);
+  ViewAXPlatformNodeDelegate* child_view_4 = view_accessibility(extra_views[4]);
+
+  EXPECT_EQ(view_accessibility(widget_->GetContentsView())->GetNativeObject(),
+            parent_view->GetParent());
+  EXPECT_EQ(4, parent_view->GetChildCount());
+
+  EXPECT_EQ(2, button_accessibility()->GetIndexInParent());
+  EXPECT_EQ(3, parent_view->GetIndexInParent());
+
+  EXPECT_EQ(child_view_1->GetNativeObject(), parent_view->ChildAtIndex(0));
+  EXPECT_EQ(child_view_2->GetNativeObject(), parent_view->ChildAtIndex(1));
+  EXPECT_EQ(child_view_3->GetNativeObject(), parent_view->ChildAtIndex(2));
+  EXPECT_EQ(child_view_4->GetNativeObject(), parent_view->ChildAtIndex(3));
+
+  EXPECT_EQ(nullptr, parent_view->GetNextSibling());
+  EXPECT_EQ(button_accessibility()->GetNativeObject(),
+            parent_view->GetPreviousSibling());
+
+  EXPECT_EQ(parent_view->GetNativeObject(), child_view_1->GetParent());
+  EXPECT_EQ(0, child_view_1->GetChildCount());
+  EXPECT_EQ(0, child_view_1->GetIndexInParent());
+  EXPECT_EQ(child_view_2->GetNativeObject(), child_view_1->GetNextSibling());
+  EXPECT_EQ(nullptr, child_view_1->GetPreviousSibling());
+
+  EXPECT_EQ(parent_view->GetNativeObject(), child_view_2->GetParent());
+  EXPECT_EQ(0, child_view_2->GetChildCount());
+  EXPECT_EQ(1, child_view_2->GetIndexInParent());
+  EXPECT_EQ(child_view_3->GetNativeObject(), child_view_2->GetNextSibling());
+  EXPECT_EQ(child_view_1->GetNativeObject(),
+            child_view_2->GetPreviousSibling());
+
+  EXPECT_EQ(parent_view->GetNativeObject(), child_view_3->GetParent());
+  EXPECT_EQ(0, child_view_3->GetChildCount());
+  EXPECT_EQ(2, child_view_3->GetIndexInParent());
+  EXPECT_EQ(child_view_4->GetNativeObject(), child_view_3->GetNextSibling());
+  EXPECT_EQ(child_view_2->GetNativeObject(),
+            child_view_3->GetPreviousSibling());
+
+  EXPECT_EQ(parent_view->GetNativeObject(), child_view_4->GetParent());
+  EXPECT_EQ(0, child_view_4->GetChildCount());
+  EXPECT_EQ(3, child_view_4->GetIndexInParent());
+  EXPECT_EQ(nullptr, child_view_4->GetNextSibling());
+  EXPECT_EQ(child_view_3->GetNativeObject(),
+            child_view_4->GetPreviousSibling());
+}
 
-  EXPECT_EQ(view_accessibility(view_ids[0])->GetNextSibling(), nullptr);
-  EXPECT_EQ(view_accessibility(view_ids[0])->GetPreviousSibling(),
-            view_accessibility(button_)->GetNativeObject());
-  EXPECT_EQ(view_accessibility(view_ids[0])->GetIndexInParent(), 3);
-
-  EXPECT_EQ(view_accessibility(view_ids[1])->GetNextSibling(),
-            view_accessibility(view_ids[2])->GetNativeObject());
-  EXPECT_EQ(view_accessibility(view_ids[1])->GetPreviousSibling(), nullptr);
-  EXPECT_EQ(view_accessibility(view_ids[1])->GetIndexInParent(), 0);
-
-  EXPECT_EQ(view_accessibility(view_ids[2])->GetNextSibling(),
-            view_accessibility(view_ids[3])->GetNativeObject());
-  EXPECT_EQ(view_accessibility(view_ids[2])->GetPreviousSibling(),
-            view_accessibility(view_ids[1])->GetNativeObject());
-  EXPECT_EQ(view_accessibility(view_ids[2])->GetIndexInParent(), 1);
-
-  EXPECT_EQ(view_accessibility(view_ids[3])->GetNextSibling(),
-            view_accessibility(view_ids[4])->GetNativeObject());
-  EXPECT_EQ(view_accessibility(view_ids[3])->GetPreviousSibling(),
-            view_accessibility(view_ids[2])->GetNativeObject());
-  EXPECT_EQ(view_accessibility(view_ids[3])->GetIndexInParent(), 2);
-
-  EXPECT_EQ(view_accessibility(view_ids[4])->GetNextSibling(), nullptr);
-  EXPECT_EQ(view_accessibility(view_ids[4])->GetPreviousSibling(),
-            view_accessibility(view_ids[3])->GetNativeObject());
-  EXPECT_EQ(view_accessibility(view_ids[4])->GetIndexInParent(), 3);
+TEST_F(ViewAXPlatformNodeDelegateTest, TreeNavigationWithIgnoredViews) {
+  // Adds one extra parent view with four child views to our widget. The parent
+  // view is added as the next sibling of the already present button view.
+  //
+  // Widget
+  // ++Button
+  // ++++Label
+  // 0 = ++ParentView
+  // 1 = ++++ChildView1
+  // 2 = ++++ChildView2
+  // 3 = ChildView3
+  // 4 = ChildView4
+  View::Views extra_views = SetUpExtraViews();
+  ViewAXPlatformNodeDelegate* contents_view =
+      view_accessibility(widget_->GetContentsView());
+  ViewAXPlatformNodeDelegate* parent_view = view_accessibility(extra_views[0]);
+  ViewAXPlatformNodeDelegate* child_view_1 = view_accessibility(extra_views[1]);
+  ViewAXPlatformNodeDelegate* child_view_2 = view_accessibility(extra_views[2]);
+  ViewAXPlatformNodeDelegate* child_view_3 = view_accessibility(extra_views[3]);
+  ViewAXPlatformNodeDelegate* child_view_4 = view_accessibility(extra_views[4]);
+
+  // Mark the parent view and the second child view as ignored.
+  parent_view->OverrideIsIgnored(true);
+  child_view_2->OverrideIsIgnored(true);
+
+  EXPECT_EQ(contents_view->GetNativeObject(), parent_view->GetParent());
+  EXPECT_EQ(3, parent_view->GetChildCount());
+
+  EXPECT_EQ(2, button_accessibility()->GetIndexInParent());
+  EXPECT_EQ(-1, parent_view->GetIndexInParent());
+
+  EXPECT_EQ(child_view_1->GetNativeObject(), parent_view->ChildAtIndex(0));
+  EXPECT_EQ(child_view_3->GetNativeObject(), parent_view->ChildAtIndex(1));
+  EXPECT_EQ(child_view_4->GetNativeObject(), parent_view->ChildAtIndex(2));
+
+  EXPECT_EQ(button_accessibility()->GetNativeObject(),
+            contents_view->ChildAtIndex(2));
+  EXPECT_EQ(child_view_1->GetNativeObject(), contents_view->ChildAtIndex(3));
+  EXPECT_EQ(child_view_3->GetNativeObject(), contents_view->ChildAtIndex(4));
+  EXPECT_EQ(child_view_4->GetNativeObject(), contents_view->ChildAtIndex(5));
+
+  EXPECT_EQ(nullptr, parent_view->GetNextSibling());
+  EXPECT_EQ(nullptr, parent_view->GetPreviousSibling());
+
+  EXPECT_EQ(contents_view->GetNativeObject(), child_view_1->GetParent());
+  EXPECT_EQ(0, child_view_1->GetChildCount());
+  EXPECT_EQ(3, child_view_1->GetIndexInParent());
+  EXPECT_EQ(child_view_3->GetNativeObject(), child_view_1->GetNextSibling());
+  EXPECT_EQ(button_accessibility()->GetNativeObject(),
+            child_view_1->GetPreviousSibling());
+
+  EXPECT_EQ(contents_view->GetNativeObject(), child_view_2->GetParent());
+  EXPECT_EQ(0, child_view_2->GetChildCount());
+  EXPECT_EQ(-1, child_view_2->GetIndexInParent());
+  EXPECT_EQ(nullptr, child_view_2->GetNextSibling());
+  EXPECT_EQ(nullptr, child_view_2->GetPreviousSibling());
+
+  EXPECT_EQ(contents_view->GetNativeObject(), child_view_3->GetParent());
+  EXPECT_EQ(0, child_view_3->GetChildCount());
+  EXPECT_EQ(4, child_view_3->GetIndexInParent());
+  EXPECT_EQ(child_view_4->GetNativeObject(), child_view_3->GetNextSibling());
+  EXPECT_EQ(child_view_1->GetNativeObject(),
+            child_view_3->GetPreviousSibling());
+
+  EXPECT_EQ(contents_view->GetNativeObject(), child_view_4->GetParent());
+  EXPECT_EQ(0, child_view_4->GetChildCount());
+  EXPECT_EQ(5, child_view_4->GetIndexInParent());
+  EXPECT_EQ(nullptr, child_view_4->GetNextSibling());
+  EXPECT_EQ(child_view_3->GetNativeObject(),
+            child_view_4->GetPreviousSibling());
 }
 
 TEST_F(ViewAXPlatformNodeDelegateTest, OverrideHasPopup) {

ui/views/accessibility/view_ax_platform_node_delegate_win.cc

@@ -46,7 +46,7 @@ ViewAXPlatformNodeDelegateWin::~ViewAXPlatformNodeDelegateWin() = default;
 gfx::NativeViewAccessible ViewAXPlatformNodeDelegateWin::GetParent() {
   // If the View has a parent View, return that View's IAccessible.
   if (view()->parent())
-    return view()->parent()->GetNativeViewAccessible();
+    return ViewAXPlatformNodeDelegate::GetParent();
 
   // Otherwise we must be the RootView, get the corresponding Widget
   // and Window.
@@ -105,4 +105,5 @@ gfx::Rect ViewAXPlatformNodeDelegateWin::GetBoundsRect(
       return gfx::Rect();
   }
 }
+
 }  // namespace views