Keep the same tree root when the root is simply updated

R=dmazzoni@chromium.org

Bug: 785100, 761882
Change-Id: Iab2181a6208b116eb2043446eb8edb4061a59d76
Tested: Manually with Jaws and NVDA on crbug.com and freedomscientific.com, unit test
Reviewed-on: https://chromium-review.googlesource.com/820107
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523332}

ui/accessibility/ax_tree.cc

@@ -306,6 +306,9 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
   if (update.has_tree_data)
     UpdateData(update.tree_data);
 
+  // We distinguish between updating the root, e.g. changing its children or
+  // some of its attributes, or replacing the root completely.
+  bool root_updated = false;
   if (update.node_id_to_clear != 0) {
     AXNode* node = GetFromId(update.node_id_to_clear);
     if (!node) {
@@ -313,13 +316,25 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
                                   update.node_id_to_clear);
       return false;
     }
+
+    // Only destroy the root if the root was replaced and not if it's simply
+    // updated. To figure out if  the root was simply updated, we compare the ID
+    // of the new root with the existing root ID.
     if (node == root_) {
-      // Clear root_ before calling DestroySubtree so that root_ doesn't
-      // ever point to an invalid node.
-      AXNode* old_root = root_;
-      root_ = nullptr;
-      DestroySubtree(old_root, &update_state);
-    } else {
+      if (update.root_id != old_root_id) {
+        // Clear root_ before calling DestroySubtree so that root_ doesn't ever
+        // point to an invalid node.
+        AXNode* old_root = root_;
+        root_ = nullptr;
+        DestroySubtree(old_root, &update_state);
+      } else {
+        root_updated = true;
+      }
+    }
+
+    // If the root has simply been updated, we treat it like an update to any
+    // other node.
+    if (root_ && (node != root_ || root_updated)) {
       for (int i = 0; i < node->child_count(); ++i)
         DestroySubtree(node->ChildAtIndex(i), &update_state);
       std::vector<AXNode*> children;
@@ -364,17 +379,23 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
       if (is_new_node) {
         if (is_reparented_node) {
           // A reparented subtree is any new node whose parent either doesn't
-          // exist, or is not new.
+          // exist, or whose parent is not new.
+          // Note that we also need to check for the special case when we update
+          // the root without replacing it.
           bool is_subtree = !node->parent() ||
-                            new_nodes.find(node->parent()) == new_nodes.end();
+                            new_nodes.find(node->parent()) == new_nodes.end() ||
+                            (node->parent() == root_ && root_updated);
           change = is_subtree ? AXTreeDelegate::SUBTREE_REPARENTED
                               : AXTreeDelegate::NODE_REPARENTED;
         } else {
           // A new subtree is any new node whose parent is either not new, or
           // whose parent happens to be new only because it has been reparented.
+          // Note that we also need to check for the special case when we update
+          // the root without replacing it.
           bool is_subtree = !node->parent() ||
                             new_nodes.find(node->parent()) == new_nodes.end() ||
-                            update_state.HasRemovedNode(node->parent());
+                            update_state.HasRemovedNode(node->parent()) ||
+                            (node->parent() == root_ && root_updated);
           change = is_subtree ? AXTreeDelegate::SUBTREE_CREATED
                               : AXTreeDelegate::NODE_CREATED;
         }

ui/accessibility/ax_tree_unittest.cc

@@ -423,6 +423,118 @@ TEST(AXTreeTest, InvalidReparentingFails) {
   ASSERT_EQ("Node 3 reparented from 2 to 1", tree.error());
 }
 
+TEST(AXTreeTest, NoReparentingOfRootIfNoNewRoot) {
+  AXNodeData root;
+  root.id = 1;
+  AXNodeData child1;
+  child1.id = 2;
+  AXNodeData child2;
+  child2.id = 3;
+
+  root.child_ids = {child1.id};
+  child1.child_ids = {child2.id};
+
+  AXTreeUpdate initial_state;
+  initial_state.root_id = root.id;
+  initial_state.nodes = {root, child1, child2};
+
+  AXTree tree(initial_state);
+
+  // Update the root but don't change it by reparenting |child2| to be a child
+  // of the root.
+  root.child_ids = {child1.id, child2.id};
+  child1.child_ids = {};
+
+  AXTreeUpdate update;
+  update.root_id = root.id;
+  update.node_id_to_clear = root.id;
+  update.nodes = {root, child1, child2};
+
+  FakeAXTreeDelegate fake_delegate;
+  tree.SetDelegate(&fake_delegate);
+  ASSERT_TRUE(tree.Unserialize(update));
+
+  EXPECT_EQ(0U, fake_delegate.deleted_ids().size());
+  EXPECT_EQ(0U, fake_delegate.subtree_deleted_ids().size());
+  EXPECT_EQ(0U, fake_delegate.created_ids().size());
+
+  EXPECT_EQ(0U, fake_delegate.node_creation_finished_ids().size());
+  EXPECT_EQ(0U, fake_delegate.subtree_creation_finished_ids().size());
+  EXPECT_EQ(0U, fake_delegate.node_reparented_finished_ids().size());
+
+  ASSERT_EQ(2U, fake_delegate.subtree_reparented_finished_ids().size());
+  EXPECT_EQ(child1.id, fake_delegate.subtree_reparented_finished_ids()[0]);
+  EXPECT_EQ(child2.id, fake_delegate.subtree_reparented_finished_ids()[1]);
+
+  ASSERT_EQ(1U, fake_delegate.change_finished_ids().size());
+  EXPECT_EQ(root.id, fake_delegate.change_finished_ids()[0]);
+
+  EXPECT_FALSE(fake_delegate.root_changed());
+  EXPECT_FALSE(fake_delegate.tree_data_changed());
+
+  tree.SetDelegate(nullptr);
+}
+
+TEST(AXTreeTest, ReparentRootIfRootChanged) {
+  AXNodeData root;
+  root.id = 1;
+  AXNodeData child1;
+  child1.id = 2;
+  AXNodeData child2;
+  child2.id = 3;
+
+  root.child_ids = {child1.id};
+  child1.child_ids = {child2.id};
+
+  AXTreeUpdate initial_state;
+  initial_state.root_id = root.id;
+  initial_state.nodes = {root, child1, child2};
+
+  AXTree tree(initial_state);
+
+  // Create a new root and reparent |child2| to be a child of the new root.
+  AXNodeData root2;
+  root2.id = 4;
+  root2.child_ids = {child1.id, child2.id};
+  child1.child_ids = {};
+
+  AXTreeUpdate update;
+  update.root_id = root2.id;
+  update.node_id_to_clear = root.id;
+  update.nodes = {root2, child1, child2};
+
+  FakeAXTreeDelegate fake_delegate;
+  tree.SetDelegate(&fake_delegate);
+  ASSERT_TRUE(tree.Unserialize(update));
+
+  ASSERT_EQ(1U, fake_delegate.deleted_ids().size());
+  EXPECT_EQ(root.id, fake_delegate.deleted_ids()[0]);
+
+  ASSERT_EQ(1U, fake_delegate.subtree_deleted_ids().size());
+  EXPECT_EQ(root.id, fake_delegate.subtree_deleted_ids()[0]);
+
+  ASSERT_EQ(1U, fake_delegate.created_ids().size());
+  EXPECT_EQ(root2.id, fake_delegate.created_ids()[0]);
+
+  EXPECT_EQ(0U, fake_delegate.node_creation_finished_ids().size());
+
+  ASSERT_EQ(1U, fake_delegate.subtree_creation_finished_ids().size());
+  EXPECT_EQ(root2.id, fake_delegate.subtree_creation_finished_ids()[0]);
+
+  ASSERT_EQ(2U, fake_delegate.node_reparented_finished_ids().size());
+  EXPECT_EQ(child1.id, fake_delegate.node_reparented_finished_ids()[0]);
+  EXPECT_EQ(child2.id, fake_delegate.node_reparented_finished_ids()[1]);
+
+  EXPECT_EQ(0U, fake_delegate.subtree_reparented_finished_ids().size());
+
+  EXPECT_EQ(0U, fake_delegate.change_finished_ids().size());
+
+  EXPECT_TRUE(fake_delegate.root_changed());
+  EXPECT_FALSE(fake_delegate.tree_data_changed());
+
+  tree.SetDelegate(nullptr);
+}
+
 TEST(AXTreeTest, TreeDelegateIsCalled) {
   AXTreeUpdate initial_state;
   initial_state.root_id = 1;
@@ -433,8 +545,8 @@ TEST(AXTreeTest, TreeDelegateIsCalled) {
 
   AXTree tree(initial_state);
   AXTreeUpdate update;
-  update.node_id_to_clear = 1;
   update.root_id = 3;
+  update.node_id_to_clear = 1;
   update.nodes.resize(2);
   update.nodes[0].id = 3;
   update.nodes[0].child_ids.push_back(4);
@@ -443,7 +555,7 @@ TEST(AXTreeTest, TreeDelegateIsCalled) {
   FakeAXTreeDelegate fake_delegate;
   tree.SetDelegate(&fake_delegate);
 
-  EXPECT_TRUE(tree.Unserialize(update));
+  ASSERT_TRUE(tree.Unserialize(update));
 
   ASSERT_EQ(2U, fake_delegate.deleted_ids().size());
   EXPECT_EQ(1, fake_delegate.deleted_ids()[0]);
@@ -462,7 +574,7 @@ TEST(AXTreeTest, TreeDelegateIsCalled) {
   ASSERT_EQ(1U, fake_delegate.node_creation_finished_ids().size());
   EXPECT_EQ(4, fake_delegate.node_creation_finished_ids()[0]);
 
-  ASSERT_EQ(true, fake_delegate.root_changed());
+  ASSERT_TRUE(fake_delegate.root_changed());
 
   tree.SetDelegate(nullptr);
 }
@@ -576,7 +688,7 @@ TEST(AXTreeTest, TreeDelegateIsNotCalledForReparenting) {
   EXPECT_EQ(0U, fake_delegate.node_creation_finished_ids().size());
   EXPECT_EQ(0U, fake_delegate.node_reparented_finished_ids().size());
 
-  ASSERT_EQ(true, fake_delegate.root_changed());
+  ASSERT_TRUE(fake_delegate.root_changed());
 
   tree.SetDelegate(nullptr);
 }