Removed child tree traversal from PlatformGet*Sibling.

We should not have supported getting a child tree sibling.
Child trees are always considered to be an "only child."

This change removes the ability to traverse to a child
tree using PlatformGet*Sibling, as valid usage of that
method requires a sibling to exist.

PlatformGetChild will now return nullptr if the node has a
child tree and the specified child index is not 0

Updated test.

Bug: 987472
Change-Id: I5b69e169531f3448a317f7b8d698cea5857b7ad7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1717912
Commit-Queue: Jacques Newman <janewman@microsoft.com>
Reviewed-by: Kevin Babbitt <kbabbitt@microsoft.com>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682376}

content/browser/accessibility/browser_accessibility.cc

@@ -144,26 +144,11 @@ BrowserAccessibility* BrowserAccessibility::PlatformGetLastChild() const {
 }
 
 BrowserAccessibility* BrowserAccessibility::PlatformGetNextSibling() const {
-  BrowserAccessibility* next_sibling = InternalGetNextSibling();
-  if (next_sibling)
-    return next_sibling;
-
-  ui::AXNode* parent = node_->GetUnignoredParent();
-  if (!parent) {
-    BrowserAccessibility* parent_tree_parent =
-        manager_->GetParentNodeFromParentTree();
-    if (parent_tree_parent)
-      return parent_tree_parent->PlatformGetChild(1);
-  }
-  return nullptr;
+  return InternalGetNextSibling();
 }
 
 BrowserAccessibility* BrowserAccessibility::PlatformGetPreviousSibling() const {
-  BrowserAccessibility* previous_sibling = InternalGetPreviousSibling();
-  if (!previous_sibling)
-    previous_sibling = PlatformGetRootOfChildTree();
-
-  return previous_sibling;
+  return InternalGetPreviousSibling();
 }
 
 BrowserAccessibility::PlatformChildIterator
@@ -221,9 +206,12 @@ BrowserAccessibility* BrowserAccessibility::PlatformGetChild(
     uint32_t child_index) const {
   BrowserAccessibility* result = nullptr;
 
-  if (child_index == 0 &&
-      HasStringAttribute(ax::mojom::StringAttribute::kChildTreeId)) {
-    result = PlatformGetRootOfChildTree();
+  if (HasStringAttribute(ax::mojom::StringAttribute::kChildTreeId)) {
+    // A node should not have both children and a child tree.
+    DCHECK_EQ(node_->children().size(), 0u);
+    // child_trees do not have siblings.
+    if (child_index == 0)
+      result = PlatformGetRootOfChildTree();
   } else {
     result = InternalGetChild(child_index);
   }
@@ -1911,7 +1899,6 @@ bool BrowserAccessibility::SetHypertextSelection(int start_offset,
 BrowserAccessibility* BrowserAccessibility::PlatformGetRootOfChildTree() const {
   if (!HasStringAttribute(ax::mojom::StringAttribute::kChildTreeId))
     return nullptr;
-
   AXTreeID child_tree_id = AXTreeID::FromString(
       GetStringAttribute(ax::mojom::StringAttribute::kChildTreeId));
   BrowserAccessibilityManager* child_manager =

content/browser/accessibility/browser_accessibility_unittest.cc

@@ -210,99 +210,149 @@ TEST_F(BrowserAccessibilityTest, TestGetDescendants) {
 
 TEST_F(BrowserAccessibilityTest, PlatformChildIterator) {
   // (i) => node is ignored
+  // Parent Tree
   // 1
   // |__________
   // |     |   |
   // 2(i)  3   4
-  // |_______________________
-  // |   |      |           |
-  // 5   6      7(i)        8(i)
-  // |   |      |________
-  // |   |      |       |
-  // 9   10(i)   11(i)   12
-  //     |      |____
-  //     |      |   |
-  //     13(i)  14  15
-  ui::AXTreeUpdate tree_update;
-  tree_update.root_id = 1;
-  tree_update.nodes.resize(15);
-  tree_update.nodes[0].id = 1;
-  tree_update.nodes[0].child_ids = {2, 3, 4};
+  // |__________________________________
+  // |              |      |           |
+  // 5              6      7(i)        8(i)
+  // |              |      |________
+  // |              |      |       |
+  // Child Tree     9(i)   10(i)   11
+  //                |      |____
+  //                |      |   |
+  //                12(i)  13  14
+  // Child Tree
+  // 1
+  // |_________
+  // |    |   |
+  // 2    3   4
+  //      |
+  //      5
+  ui::AXTreeID parent_tree_id = ui::AXTreeID::CreateNewAXTreeID();
+  ui::AXTreeID child_tree_id = ui::AXTreeID::CreateNewAXTreeID();
 
-  tree_update.nodes[1].id = 2;
-  tree_update.nodes[1].child_ids = {5, 6, 7, 8};
-  tree_update.nodes[1].AddState(ax::mojom::State::kIgnored);
+  ui::AXTreeUpdate parent_tree_update;
+  parent_tree_update.tree_data.tree_id = parent_tree_id;
+  parent_tree_update.has_tree_data = true;
+  parent_tree_update.root_id = 1;
+  parent_tree_update.nodes.resize(14);
+  parent_tree_update.nodes[0].id = 1;
+  parent_tree_update.nodes[0].child_ids = {2, 3, 4};
 
-  tree_update.nodes[2].id = 3;
-  tree_update.nodes[3].id = 4;
+  parent_tree_update.nodes[1].id = 2;
+  parent_tree_update.nodes[1].child_ids = {5, 6, 7, 8};
+  parent_tree_update.nodes[1].AddState(ax::mojom::State::kIgnored);
 
-  tree_update.nodes[4].id = 5;
-  tree_update.nodes[4].child_ids = {9};
+  parent_tree_update.nodes[2].id = 3;
+  parent_tree_update.nodes[3].id = 4;
 
-  tree_update.nodes[5].id = 6;
-  tree_update.nodes[5].child_ids = {10};
+  parent_tree_update.nodes[4].id = 5;
+  parent_tree_update.nodes[4].AddStringAttribute(
+      ax::mojom::StringAttribute::kChildTreeId, child_tree_id.ToString());
 
-  tree_update.nodes[6].id = 7;
-  tree_update.nodes[6].child_ids = {11, 12};
-  tree_update.nodes[6].AddState(ax::mojom::State::kIgnored);
+  parent_tree_update.nodes[5].id = 6;
+  parent_tree_update.nodes[5].child_ids = {9};
 
-  tree_update.nodes[7].id = 8;
-  tree_update.nodes[7].AddState(ax::mojom::State::kIgnored);
+  parent_tree_update.nodes[6].id = 7;
+  parent_tree_update.nodes[6].child_ids = {10, 11};
+  parent_tree_update.nodes[6].AddState(ax::mojom::State::kIgnored);
 
-  tree_update.nodes[8].id = 9;
+  parent_tree_update.nodes[7].id = 8;
+  parent_tree_update.nodes[7].AddState(ax::mojom::State::kIgnored);
 
-  tree_update.nodes[9].id = 10;
-  tree_update.nodes[9].child_ids = {13};
-  tree_update.nodes[9].AddState(ax::mojom::State::kIgnored);
+  parent_tree_update.nodes[8].id = 9;
+  parent_tree_update.nodes[8].child_ids = {12};
+  parent_tree_update.nodes[8].AddState(ax::mojom::State::kIgnored);
 
-  tree_update.nodes[10].id = 11;
-  tree_update.nodes[10].child_ids = {14, 15};
-  tree_update.nodes[10].AddState(ax::mojom::State::kIgnored);
+  parent_tree_update.nodes[9].id = 10;
+  parent_tree_update.nodes[9].child_ids = {13, 14};
+  parent_tree_update.nodes[9].AddState(ax::mojom::State::kIgnored);
 
-  tree_update.nodes[11].id = 12;
+  parent_tree_update.nodes[10].id = 11;
 
-  tree_update.nodes[12].id = 13;
-  tree_update.nodes[12].AddState(ax::mojom::State::kIgnored);
+  parent_tree_update.nodes[11].id = 12;
+  parent_tree_update.nodes[11].AddState(ax::mojom::State::kIgnored);
 
-  tree_update.nodes[13].id = 14;
+  parent_tree_update.nodes[12].id = 13;
 
-  tree_update.nodes[14].id = 15;
+  parent_tree_update.nodes[13].id = 14;
 
-  std::unique_ptr<BrowserAccessibilityManager> manager(
+  ui::AXTreeUpdate child_tree_update;
+  child_tree_update.tree_data.tree_id = child_tree_id;
+  child_tree_update.tree_data.parent_tree_id = parent_tree_id;
+  child_tree_update.has_tree_data = true;
+  child_tree_update.root_id = 1;
+  child_tree_update.nodes.resize(5);
+  child_tree_update.nodes[0].id = 1;
+  child_tree_update.nodes[0].child_ids = {2, 3, 4};
+
+  child_tree_update.nodes[1].id = 2;
+
+  child_tree_update.nodes[2].id = 3;
+  child_tree_update.nodes[2].child_ids = {5};
+
+  child_tree_update.nodes[3].id = 4;
+
+  child_tree_update.nodes[4].id = 5;
+
+  std::unique_ptr<BrowserAccessibilityManager> parent_manager(
       BrowserAccessibilityManager::Create(
-          tree_update, test_browser_accessibility_delegate_.get(),
+          parent_tree_update, test_browser_accessibility_delegate_.get(),
           new BrowserAccessibilityFactory()));
 
-  BrowserAccessibility* root_obj = manager->GetRoot();
+  std::unique_ptr<BrowserAccessibilityManager> child_manager(
+      BrowserAccessibilityManager::Create(
+          child_tree_update, test_browser_accessibility_delegate_.get(),
+          new BrowserAccessibilityFactory()));
+
+  BrowserAccessibility* root_obj = parent_manager->GetRoot();
   // Test traversal
-  // PlatformChildren(root_obj) = {5, 6, 14, 15, 12, 3, 4}
+  // PlatformChildren(root_obj) = {5, 6, 13, 15, 11, 3, 4}
   BrowserAccessibility::PlatformChildIterator platform_iterator =
       root_obj->PlatformChildrenBegin();
   EXPECT_EQ(5, platform_iterator->GetId());
+  EXPECT_EQ(nullptr, platform_iterator->PlatformGetPreviousSibling());
+  EXPECT_EQ(1u, platform_iterator->PlatformChildCount());
+
+  // Test Child-Tree Traversal
+  BrowserAccessibility* child_tree_root =
+      platform_iterator->PlatformGetFirstChild();
+  EXPECT_EQ(1, child_tree_root->GetId());
+  BrowserAccessibility::PlatformChildIterator child_tree_iterator =
+      child_tree_root->PlatformChildrenBegin();
+
+  EXPECT_EQ(2, child_tree_iterator->GetId());
+  ++child_tree_iterator;
+  EXPECT_EQ(3, child_tree_iterator->GetId());
+  ++child_tree_iterator;
+  EXPECT_EQ(4, child_tree_iterator->GetId());
 
   ++platform_iterator;
   EXPECT_EQ(6, platform_iterator->GetId());
 
   ++platform_iterator;
-  EXPECT_EQ(14, platform_iterator->GetId());
+  EXPECT_EQ(13, platform_iterator->GetId());
 
   ++platform_iterator;
-  EXPECT_EQ(15, platform_iterator->GetId());
+  EXPECT_EQ(14, platform_iterator->GetId());
 
   --platform_iterator;
-  EXPECT_EQ(14, platform_iterator->GetId());
+  EXPECT_EQ(13, platform_iterator->GetId());
 
   --platform_iterator;
   EXPECT_EQ(6, platform_iterator->GetId());
 
   ++platform_iterator;
-  EXPECT_EQ(14, platform_iterator->GetId());
+  EXPECT_EQ(13, platform_iterator->GetId());
 
   ++platform_iterator;
-  EXPECT_EQ(15, platform_iterator->GetId());
+  EXPECT_EQ(14, platform_iterator->GetId());
 
   ++platform_iterator;
-  EXPECT_EQ(12, platform_iterator->GetId());
+  EXPECT_EQ(11, platform_iterator->GetId());
 
   ++platform_iterator;
   EXPECT_EQ(3, platform_iterator->GetId());
@@ -314,22 +364,22 @@ TEST_F(BrowserAccessibilityTest, PlatformChildIterator) {
   EXPECT_EQ(root_obj->PlatformChildrenEnd(), platform_iterator);
 
   // test empty list
-  // PlatformChildren(2) = {}
-  BrowserAccessibility* node2 = manager->GetFromID(3);
+  // PlatformChildren(3) = {}
+  BrowserAccessibility* node2 = parent_manager->GetFromID(3);
   platform_iterator = node2->PlatformChildrenBegin();
   EXPECT_EQ(node2->PlatformChildrenEnd(), platform_iterator);
 
   // empty list from ignored node
-  // PlatformChildren(7) = {}
-  BrowserAccessibility* node7 = manager->GetFromID(8);
-  platform_iterator = node7->PlatformChildrenBegin();
-  EXPECT_EQ(node7->PlatformChildrenEnd(), platform_iterator);
+  // PlatformChildren(8) = {}
+  BrowserAccessibility* node8 = parent_manager->GetFromID(8);
+  platform_iterator = node8->PlatformChildrenBegin();
+  EXPECT_EQ(node8->PlatformChildrenEnd(), platform_iterator);
 
   // non-empty list from ignored node
-  // PlatformChildren(10) = {14, 15}
-  BrowserAccessibility* node10 = manager->GetFromID(11);
+  // PlatformChildren(10) = {13, 15}
+  BrowserAccessibility* node10 = parent_manager->GetFromID(10);
   platform_iterator = node10->PlatformChildrenBegin();
-  EXPECT_EQ(14, platform_iterator->GetId());
+  EXPECT_EQ(13, platform_iterator->GetId());
 
   // Two UnignoredChildIterators from the same parent at the same position
   // should be equivalent, even in end position.