Implement accessible actions for TreeView nodes

Implements virtual view targeting for accessibility actions for
|TreeView| nodes so that they may be activated using the accessibility
review cursor. Previously, all actions were routed to the selected node,
even if the review cursor was on a different element.

Since focus is an accessible action, this changelist also includes
improvements to how accessibility focus events get fired around virtual
views:

- If a focused virtual descendant is set when |FocusManager| gives focus
  to the owner view, the descendant (instead of the owner view's
  accessible object) now fires the accessibility focus event.

- Consequently, by setting the |TreeView|'s focused virtual descendant,
  ahead of time, TreeView::OnFocus() no longer needs to redirect focus
  to a virtual view via ViewAccessibility::OverrideFocus(). This avoids
  firing multiple accessibility focus events for a single focus change.

             inside tree views.

AX-Relnotes: In Views, assistive technologies can now select tree items
Bug: 811277
Change-Id: I232c2acf2f4fec79ea30dbe82b5f077a4f9c32b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135205
Commit-Queue: Brian Liu Xu <brx@microsoft.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770851}

chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc

@@ -169,7 +169,8 @@ void BookmarkEditorView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
 void BookmarkEditorView::ButtonPressed(views::Button* sender,
                                        const ui::Event& event) {
   DCHECK_EQ(new_folder_button_, sender);
-  NewFolder();
+  DCHECK(tree_view_->GetSelectedNode());
+  NewFolder(tree_model_->AsNode(tree_view_->GetSelectedNode()));
 }
 
 bool BookmarkEditorView::IsCommandIdChecked(int command_id) const {
@@ -196,11 +197,11 @@ bool BookmarkEditorView::GetAcceleratorForCommandId(
 }
 
 void BookmarkEditorView::ExecuteCommand(int command_id, int event_flags) {
-  DCHECK(tree_view_->GetSelectedNode());
+  DCHECK(tree_view_->GetActiveNode());
   if (command_id == IDS_EDIT) {
-    tree_view_->StartEditing(tree_view_->GetSelectedNode());
+    tree_view_->StartEditing(tree_view_->GetActiveNode());
   } else if (command_id == IDS_DELETE) {
-    EditorNode* node = tree_model_->AsNode(tree_view_->GetSelectedNode());
+    EditorNode* node = tree_model_->AsNode(tree_view_->GetActiveNode());
     if (!node)
       return;
     if (node->value != 0) {
@@ -217,7 +218,7 @@ void BookmarkEditorView::ExecuteCommand(int command_id, int event_flags) {
     tree_model_->Remove(node->parent(), node);
   } else {
     DCHECK_EQ(IDS_BOOKMARK_EDITOR_NEW_FOLDER_MENU_ITEM, command_id);
-    NewFolder();
+    NewFolder(tree_model_->AsNode(tree_view_->GetActiveNode()));
   }
 }
 
@@ -238,10 +239,10 @@ void BookmarkEditorView::ShowContextMenuForViewImpl(
     const gfx::Point& point,
     ui::MenuSourceType source_type) {
   DCHECK_EQ(tree_view_, source);
-  if (!tree_view_->GetSelectedNode())
+  if (!tree_view_->GetActiveNode())
     return;
   running_menu_for_root_ =
-      (tree_model_->GetParent(tree_view_->GetSelectedNode()) ==
+      (tree_model_->GetParent(tree_view_->GetActiveNode()) ==
        tree_model_->GetRoot());
 
   context_menu_runner_ = std::make_unique<views::MenuRunner>(
@@ -445,10 +446,8 @@ void BookmarkEditorView::UserInputChanged() {
   DialogModelChanged();
 }
 
-void BookmarkEditorView::NewFolder() {
-  // Create a new entry parented to the selected item, or the bookmark
-  // bar if nothing is selected.
-  EditorNode* parent = tree_model_->AsNode(tree_view_->GetSelectedNode());
+void BookmarkEditorView::NewFolder(EditorNode* parent) {
+  // Create a new entry parented to the given item.
   if (!parent) {
     NOTREACHED();
     return;

chrome/browser/ui/views/bookmarks/bookmark_editor_view.h

@@ -205,10 +205,9 @@ class BookmarkEditorView : public BookmarkEditor,
   // of Textfields and ok button appropriately.
   void UserInputChanged();
 
-  // Creates a new folder as a child of the selected node. If no node is
-  // selected, the new folder is added as a child of the bookmark node. Starts
-  // editing on the new group as well.
-  void NewFolder();
+  // Creates a new folder as a child of the given node. Starts editing on the
+  // new group as well.
+  void NewFolder(EditorNode* parent);
 
   // Creates a new EditorNode as the last child of parent. The new node is
   // added to the model and returned. This does NOT start editing. This is used

chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc

@@ -92,8 +92,8 @@ class BookmarkEditorViewTest : public testing::Test {
     return editor_->AddNewFolder(parent);
   }
 
-  void NewFolder() {
-    return editor_->NewFolder();
+  void NewFolder(BookmarkEditorView::EditorNode* node) {
+    return editor_->NewFolder(node);
   }
 
   bool URLTFHasParent() {
@@ -151,9 +151,9 @@ class BookmarkEditorViewTest : public testing::Test {
 
 // Makes sure the tree model matches that of the bookmark bar model.
 TEST_F(BookmarkEditorViewTest, ModelsMatch) {
-  CreateEditor(profile_.get(), NULL,
+  CreateEditor(profile_.get(), nullptr,
                BookmarkEditor::EditDetails::AddNodeInFolder(
-                   NULL, size_t{-1}, GURL(), base::string16()),
+                   nullptr, size_t{-1}, GURL(), base::string16()),
                BookmarkEditorView::SHOW_TREE);
   BookmarkEditorView::EditorNode* editor_root = editor_tree_model()->GetRoot();
   // The root should have two or three children: bookmark bar, other bookmarks
@@ -180,7 +180,7 @@ TEST_F(BookmarkEditorViewTest, ModelsMatch) {
 
 // Changes the title and makes sure parent/visual order doesn't change.
 TEST_F(BookmarkEditorViewTest, EditTitleKeepsPosition) {
-  CreateEditor(profile_.get(), NULL,
+  CreateEditor(profile_.get(), nullptr,
                BookmarkEditor::EditDetails::EditNode(GetNode("a")),
                BookmarkEditorView::SHOW_TREE);
   SetTitleText(ASCIIToUTF16("new_a"));
@@ -197,7 +197,7 @@ TEST_F(BookmarkEditorViewTest, EditTitleKeepsPosition) {
 TEST_F(BookmarkEditorViewTest, EditURLKeepsPosition) {
   base::Time node_time = base::Time::Now() + base::TimeDelta::FromDays(2);
   GetMutableNode("a")->set_date_added(node_time);
-  CreateEditor(profile_.get(), NULL,
+  CreateEditor(profile_.get(), nullptr,
                BookmarkEditor::EditDetails::EditNode(GetNode("a")),
                BookmarkEditorView::SHOW_TREE);
 
@@ -215,7 +215,7 @@ TEST_F(BookmarkEditorViewTest, EditURLKeepsPosition) {
 
 // Moves 'a' to be a child of the other node.
 TEST_F(BookmarkEditorViewTest, ChangeParent) {
-  CreateEditor(profile_.get(), NULL,
+  CreateEditor(profile_.get(), nullptr,
                BookmarkEditor::EditDetails::EditNode(GetNode("a")),
                BookmarkEditorView::SHOW_TREE);
 
@@ -230,7 +230,7 @@ TEST_F(BookmarkEditorViewTest, ChangeParent) {
 TEST_F(BookmarkEditorViewTest, ChangeParentAndURL) {
   base::Time node_time = base::Time::Now() + base::TimeDelta::FromDays(2);
   GetMutableNode("a")->set_date_added(node_time);
-  CreateEditor(profile_.get(), NULL,
+  CreateEditor(profile_.get(), nullptr,
                BookmarkEditor::EditDetails::EditNode(GetNode("a")),
                BookmarkEditorView::SHOW_TREE);
 
@@ -246,7 +246,7 @@ TEST_F(BookmarkEditorViewTest, ChangeParentAndURL) {
 
 // Creates a new folder and moves a node to it.
 TEST_F(BookmarkEditorViewTest, MoveToNewParent) {
-  CreateEditor(profile_.get(), NULL,
+  CreateEditor(profile_.get(), nullptr,
                BookmarkEditor::EditDetails::EditNode(GetNode("a")),
                BookmarkEditorView::SHOW_TREE);
 
@@ -301,7 +301,7 @@ TEST_F(BookmarkEditorViewTest, NewURL) {
 
 // Brings up the editor with no tree and modifies the url.
 TEST_F(BookmarkEditorViewTest, ChangeURLNoTree) {
-  CreateEditor(profile_.get(), NULL,
+  CreateEditor(profile_.get(), nullptr,
                BookmarkEditor::EditDetails::EditNode(
                    model_->other_node()->children().front().get()),
                BookmarkEditorView::NO_TREE);
@@ -309,7 +309,7 @@ TEST_F(BookmarkEditorViewTest, ChangeURLNoTree) {
   SetURLText(UTF8ToUTF16(GURL(base_path() + "a").spec()));
   SetTitleText(ASCIIToUTF16("new_a"));
 
-  ApplyEdits(NULL);
+  ApplyEdits(nullptr);
 
   const BookmarkNode* other_node = model_->other_node();
   ASSERT_EQ(2u, other_node->children().size());
@@ -322,14 +322,14 @@ TEST_F(BookmarkEditorViewTest, ChangeURLNoTree) {
 
 // Brings up the editor with no tree and modifies only the title.
 TEST_F(BookmarkEditorViewTest, ChangeTitleNoTree) {
-  CreateEditor(profile_.get(), NULL,
+  CreateEditor(profile_.get(), nullptr,
                BookmarkEditor::EditDetails::EditNode(
                    model_->other_node()->children().front().get()),
                BookmarkEditorView::NO_TREE);
 
   SetTitleText(ASCIIToUTF16("new_a"));
 
-  ApplyEdits(NULL);
+  ApplyEdits(nullptr);
 
   const BookmarkNode* other_node = model_->other_node();
   ASSERT_EQ(2u, other_node->children().size());
@@ -439,8 +439,8 @@ TEST_F(BookmarkEditorViewTest, NewFolderTitleUpdatedOnCommit) {
   SetURLText(UTF8ToUTF16(GURL(base_path() + "a").spec()));
   SetTitleText(ASCIIToUTF16("new_a"));
 
-  NewFolder();
-  ASSERT_TRUE(tree_view()->editor() != NULL);
+  NewFolder(editor_tree_model()->AsNode(tree_view()->GetSelectedNode()));
+  ASSERT_NE(nullptr, tree_view()->editor());
   tree_view()->editor()->SetText(ASCIIToUTF16("modified"));
   ApplyEdits();
 

ui/views/accessibility/ax_virtual_view.cc

@@ -395,7 +395,7 @@ bool AXVirtualView::AccessibilityPerformAction(const ui::AXActionData& data) {
   if (custom_data_.HasAction(data.action))
     result = HandleAccessibleAction(data);
   if (!result && GetOwnerView())
-    return GetOwnerView()->HandleAccessibleAction(data);
+    return HandleAccessibleActionInOwnerView(data);
   return result;
 }
 
@@ -449,7 +449,17 @@ bool AXVirtualView::HandleAccessibleAction(
       break;
   }
 
-  return GetOwnerView()->HandleAccessibleAction(action_data);
+  return HandleAccessibleActionInOwnerView(action_data);
+}
+
+bool AXVirtualView::HandleAccessibleActionInOwnerView(
+    const ui::AXActionData& action_data) {
+  DCHECK(GetOwnerView());
+  // Save the node id so that the owner view can determine which virtual view
+  // is being targeted for action.
+  ui::AXActionData forwarded_action_data = action_data;
+  forwarded_action_data.target_node_id = GetData().id;
+  return GetOwnerView()->HandleAccessibleAction(forwarded_action_data);
 }
 
 View* AXVirtualView::GetOwnerView() const {

ui/views/accessibility/ax_virtual_view.h

@@ -171,6 +171,11 @@ class VIEWS_EXPORT AXVirtualView : public ui::AXPlatformNodeDelegateBase {
   // via NotifyAccessibilityEvent().
   virtual bool HandleAccessibleAction(const ui::AXActionData& action_data);
 
+ protected:
+  // Forwards a request from assistive technology to perform an action on this
+  // virtual view to the owner view's accessible action handler.
+  bool HandleAccessibleActionInOwnerView(const ui::AXActionData& action_data);
+
  private:
   // Internal class name.
   static const char kViewClassName[];

ui/views/accessibility/ax_virtual_view_unittest.cc

@@ -487,6 +487,13 @@ TEST_F(AXVirtualViewTest, OverrideFocus) {
   ASSERT_NE(nullptr, virtual_label_->GetNativeObject());
   ExpectReceivedAccessibilityEvents({});
 
+  button_->SetFocusBehavior(View::FocusBehavior::ALWAYS);
+  button_->RequestFocus();
+  ExpectReceivedAccessibilityEvents(
+      {std::make_pair(GetButtonAccessibility(), ax::mojom::Event::kFocus),
+       std::make_pair(GetButtonAccessibility(),
+                      ax::mojom::Event::kChildrenChanged)});
+
   EXPECT_EQ(button_accessibility.GetNativeObject(),
             button_accessibility.GetFocusedDescendant());
   button_accessibility.OverrideFocus(virtual_label_);
@@ -536,6 +543,10 @@ TEST_F(AXVirtualViewTest, OverrideFocus) {
 
   // Test that calling GetFocus() while the owner view is not focused will
   // return nullptr.
+  button_->SetFocusBehavior(View::FocusBehavior::NEVER);
+  button_->RequestFocus();
+  ExpectReceivedAccessibilityEvents({std::make_pair(
+      GetButtonAccessibility(), ax::mojom::Event::kChildrenChanged)});
   EXPECT_EQ(nullptr, virtual_label_->GetFocus());
   EXPECT_EQ(nullptr, virtual_child_1->GetFocus());
   EXPECT_EQ(nullptr, virtual_child_2->GetFocus());
@@ -544,7 +555,7 @@ TEST_F(AXVirtualViewTest, OverrideFocus) {
   button_->SetFocusBehavior(View::FocusBehavior::ALWAYS);
   button_->RequestFocus();
   ExpectReceivedAccessibilityEvents(
-      {std::make_pair(GetButtonAccessibility(), ax::mojom::Event::kFocus),
+      {std::make_pair(virtual_child_3, ax::mojom::Event::kFocus),
        std::make_pair(GetButtonAccessibility(),
                       ax::mojom::Event::kChildrenChanged)});
 

ui/views/accessibility/view_accessibility.cc

@@ -206,7 +206,7 @@ void ViewAccessibility::GetAccessibleNodeData(ui::AXNodeData* data) const {
     return;
   }
 
-  if (view_->IsAccessibilityFocusable())
+  if (view_->IsAccessibilityFocusable() && !focused_virtual_child_)
     data->AddState(ax::mojom::State::kFocusable);
 
   if (!view_->GetEnabled())
@@ -224,11 +224,14 @@ void ViewAccessibility::OverrideFocus(AXVirtualView* virtual_view) {
       << "|virtual_view| must be nullptr or a descendant of this view.";
   focused_virtual_child_ = virtual_view;
 
+  if (view_->HasFocus()) {
     if (focused_virtual_child_) {
-    focused_virtual_child_->NotifyAccessibilityEvent(ax::mojom::Event::kFocus);
+      focused_virtual_child_->NotifyAccessibilityEvent(
+          ax::mojom::Event::kFocus);
     } else {
       view_->NotifyAccessibilityEvent(ax::mojom::Event::kFocus, true);
     }
+  }
 }
 
 void ViewAccessibility::OverrideRole(const ax::mojom::Role role) {

ui/views/controls/tree/tree_view.h

@@ -44,6 +44,13 @@ class TreeViewController;
 // can expand, collapse and edit the items. A Controller may be attached to
 // receive notification of selection changes and restrict editing.
 //
+// In addition to tracking selection, TreeView also tracks the active node,
+// which is the item that receives keyboard input when the tree has focus.
+// Active/focus is like a pointer for keyboard navigation, and operations such
+// as selection are performed at the point of focus. The active node is synced
+// to the selected node. When the active node is nullptr, the TreeView itself is
+// the target of keyboard input.
+//
 // Note on implementation. This implementation doesn't scale well. In particular
 // it does not store any row information, but instead calculates it as
 // necessary. But it's more than adequate for current uses.
@@ -103,6 +110,20 @@ class VIEWS_EXPORT TreeView : public View,
   }
   const ui::TreeModelNode* GetSelectedNode() const;
 
+  // Marks the specified node as active, scrolls it into view, and reports a
+  // keyboard focus update to ATs. Active node should be synced to the selected
+  // node and should be nullptr when the tree is empty.
+  // TODO(crbug.com/1080944): Decouple active node from selected node by adding
+  // new keyboard affordances.
+  void SetActiveNode(ui::TreeModelNode* model_node);
+
+  // Returns the active node, or nullptr if nothing is active.
+  ui::TreeModelNode* GetActiveNode() {
+    return const_cast<ui::TreeModelNode*>(
+        const_cast<const TreeView*>(this)->GetActiveNode());
+  }
+  const ui::TreeModelNode* GetActiveNode() const;
+
   // Marks |model_node| as collapsed. This only effects the UI if node and all
   // its parents are expanded (IsExpanded(model_node) returns true).
   void Collapse(ui::TreeModelNode* model_node);
@@ -193,6 +214,20 @@ class VIEWS_EXPORT TreeView : public View,
  private:
   friend class TreeViewTest;
 
+  // Enumeration of possible changes to tree view state when the UI is updated.
+  enum SelectionType {
+    // Active state is being set to a tree item.
+    kActive,
+
+    // Active and selected states are being set to a tree item.
+    kActiveAndSelected,
+  };
+
+  // Performs active node and selected node state transitions. Updates states
+  // and scrolling before notifying assistive technologies and the controller.
+  void UpdateSelection(ui::TreeModelNode* model_node,
+                       SelectionType selection_type);
+
   // Selects, expands or collapses nodes in the tree.  Consistent behavior for
   // tap gesture and click events.
   bool OnClickOrTap(const ui::LocatedEvent& event);
@@ -354,6 +389,9 @@ class VIEWS_EXPORT TreeView : public View,
       ui::TreeModelNode* model_node,
       GetInternalNodeCreateType create_type);
 
+  // Returns the InternalNode for a virtual view.
+  InternalNode* GetInternalNodeForVirtualView(AXVirtualView* ax_view);
+
   // Returns the bounds for a node. This rectangle contains the node's icon,
   // text, arrow, and auxiliary text (if any). All of the other bounding
   // rectangles computed by the functions below lie inside this rectangle.
@@ -435,6 +473,9 @@ class VIEWS_EXPORT TreeView : public View,
   // The selected node, may be null.
   InternalNode* selected_node_ = nullptr;
 
+  // The current active node, may be null.
+  InternalNode* active_node_ = nullptr;
+
   bool editing_ = false;
 
   // The editor; lazily created and never destroyed (well, until TreeView is

ui/views/view.cc

@@ -1972,6 +1972,12 @@ void View::OnFocus() {
   // TODO(beng): Investigate whether it's possible for us to move this to
   //             Focus().
   // Notify assistive technologies of the focus change.
+  AXVirtualView* focused_virtual_child =
+      view_accessibility_ ? view_accessibility_->FocusedVirtualChild()
+                          : nullptr;
+  if (focused_virtual_child)
+    focused_virtual_child->NotifyAccessibilityEvent(ax::mojom::Event::kFocus);
+  else
     NotifyAccessibilityEvent(ax::mojom::Event::kFocus, true);
 }