Preserve author-specified reference order in AtkRelationSet

BrowserAccessibility::GetTargetNodesForRelation was converting the
correctly-ordered vector of AxIds into a set, causing the target order
to be based on when the accessible object was created. This order does
not necessarily correspond to the position in the accessibility tree,
let alone to the author-provided order.

This situation wasn't problematic because of the few ARIA properties
whose value is an ID reference list, none were likely being used for
assistive technology navigation. Because aria-details is changing to
support multiple targets for which ATs will be expected to provide
navigation, we need to start preserving the author-specified reference
order.

There are only a few properties for which there are multiple targets,
and the list of referenced IDs for those properties is expected to be
quite small. Thus converting the vector to a set doesn't buy us much.
Therefore, return the target nodes as a vector, checking for duplicate
AxIds.

Bug: 1047988
Change-Id: I8352a073b71cc1863cbd25326d0cd48a4abd3d2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2034690Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Joanmarie Diggs <jdiggs@igalia.com>
Cr-Commit-Position: refs/heads/master@{#738216}

content/browser/accessibility/browser_accessibility.cc

@@ -1312,16 +1312,28 @@ ui::AXPlatformNode* BrowserAccessibility::GetTargetNodeForRelation(
   return GetFromNodeID(target_id);
 }
 
-std::set<ui::AXPlatformNode*> BrowserAccessibility::GetTargetNodesForRelation(
+std::vector<ui::AXPlatformNode*>
+BrowserAccessibility::GetTargetNodesForRelation(
     ax::mojom::IntListAttribute attr) {
   DCHECK(ui::IsNodeIdIntListAttribute(attr));
 
   std::vector<int32_t> target_ids;
   if (!GetIntListAttribute(attr, &target_ids))
-    return std::set<ui::AXPlatformNode*>();
+    return std::vector<ui::AXPlatformNode*>();
 
-  std::set<int32_t> target_id_set(target_ids.begin(), target_ids.end());
-  return GetNodesForNodeIdSet(target_id_set);
+  // If we use std::set to eliminate duplicates, the resulting set will be
+  // sorted by the id and we will lose the original order provided by the
+  // author which may be of interest to ATs. The number of ids should be small.
+
+  std::vector<ui::AXPlatformNode*> nodes;
+  for (int32_t target_id : target_ids) {
+    if (ui::AXPlatformNode* node = GetFromNodeID(target_id)) {
+      if (std::find(nodes.begin(), nodes.end(), node) == nodes.end())
+        nodes.push_back(node);
+    }
+  }
+
+  return nodes;
 }
 
 std::set<ui::AXPlatformNode*> BrowserAccessibility::GetReverseRelations(

content/browser/accessibility/browser_accessibility.h

@@ -536,7 +536,7 @@ class CONTENT_EXPORT BrowserAccessibility : public ui::AXPlatformNodeDelegate {
   bool HasVisibleCaretOrSelection() const override;
   ui::AXPlatformNode* GetTargetNodeForRelation(
       ax::mojom::IntAttribute attr) override;
-  std::set<ui::AXPlatformNode*> GetTargetNodesForRelation(
+  std::vector<ui::AXPlatformNode*> GetTargetNodesForRelation(
       ax::mojom::IntListAttribute attr) override;
   std::set<ui::AXPlatformNode*> GetReverseRelations(
       ax::mojom::IntAttribute attr) override;

ui/accessibility/platform/ax_platform_node_auralinux.cc

@@ -2972,13 +2972,12 @@ AtkRelationSet* AXPlatformNodeAuraLinux::GetAtkRelations() {
 
   // Now we do the same for each possible relation defined by an
   // IntListAttribute. In this case we need to handle each target in the list.
-  for (unsigned i = 0; i < G_N_ELEMENTS(kIntListRelations); i++) {
-    const AtkIntListRelation& relation = kIntListRelations[i];
-
-    std::set<AXPlatformNode*> targets =
+  for (const auto& relation : kIntListRelations) {
+    std::vector<AXPlatformNode*> targets =
         GetDelegate()->GetTargetNodesForRelation(relation.attribute);
-    for (AXPlatformNode* target : targets)
+    for (AXPlatformNode* target : targets) {
       AddRelationToSet(relation_set, relation.relation, target);
+    }
 
     if (!relation.reverse_relation.has_value())
       continue;

ui/accessibility/platform/ax_platform_node_auralinux_unittest.cc

@@ -2155,6 +2155,89 @@ TEST_F(AXPlatformNodeAuraLinuxTest, TestAllReverseAtkRelations) {
                          ATK_RELATION_LABELLED_BY, ATK_RELATION_LABEL_FOR);
 }
 
+TEST_F(AXPlatformNodeAuraLinuxTest, TestAtkRelationsTargetIndex) {
+  AXNodeData root;
+  root.id = 1;
+  root.role = ax::mojom::Role::kRootWebArea;
+
+  AXNodeData label1;
+  label1.id = 2;
+  label1.role = ax::mojom::Role::kStaticText;
+  root.child_ids.push_back(2);
+
+  AXNodeData label2;
+  label2.id = 3;
+  label2.role = ax::mojom::Role::kList;
+  root.child_ids.push_back(3);
+
+  AXNodeData label3;
+  label3.id = 4;
+  label3.role = ax::mojom::Role::kTable;
+  root.child_ids.push_back(4);
+
+  AXNodeData button1;
+  button1.id = 5;
+  button1.role = ax::mojom::Role::kButton;
+  button1.AddIntListAttribute(ax::mojom::IntListAttribute::kLabelledbyIds,
+                              {2, 3, 4});
+  root.child_ids.push_back(5);
+
+  AXNodeData button2;
+  button2.id = 6;
+  button2.role = ax::mojom::Role::kButton;
+  button2.AddIntListAttribute(ax::mojom::IntListAttribute::kLabelledbyIds,
+                              {4, 3, 2});
+  root.child_ids.push_back(6);
+
+  AXNodeData button3;
+  button3.id = 7;
+  button3.role = ax::mojom::Role::kButton;
+  button3.AddIntListAttribute(ax::mojom::IntListAttribute::kLabelledbyIds,
+                              {4, 4, 2, 2, 3});
+  root.child_ids.push_back(7);
+
+  Init(root, label1, label2, label3, button1, button2, button3);
+
+  auto test_index = [&](AtkObject* source, AtkObject* target,
+                        AtkRelationType relation_type, int index) {
+    AtkRelationSet* relation_set = atk_object_ref_relation_set(source);
+    ASSERT_TRUE(atk_relation_set_contains(relation_set, relation_type));
+
+    AtkRelation* relation =
+        atk_relation_set_get_relation_by_type(relation_set, relation_type);
+    GPtrArray* targets = atk_relation_get_target(relation);
+    ASSERT_TRUE(ATK_IS_OBJECT(g_ptr_array_index(targets, index)));
+    ASSERT_TRUE(ATK_OBJECT(g_ptr_array_index(targets, index)) == target);
+
+    g_object_unref(G_OBJECT(relation_set));
+  };
+
+  AtkObject* root_atk_object(GetRootAtkObject());
+  EXPECT_TRUE(ATK_IS_OBJECT(root_atk_object));
+  g_object_ref(root_atk_object);
+
+  AtkObject* atk_label1(AtkObjectFromNode(GetRootNode()->children()[0]));
+  AtkObject* atk_label2(AtkObjectFromNode(GetRootNode()->children()[1]));
+  AtkObject* atk_label3(AtkObjectFromNode(GetRootNode()->children()[2]));
+  AtkObject* atk_button1(AtkObjectFromNode(GetRootNode()->children()[3]));
+  AtkObject* atk_button2(AtkObjectFromNode(GetRootNode()->children()[4]));
+  AtkObject* atk_button3(AtkObjectFromNode(GetRootNode()->children()[5]));
+
+  test_index(atk_button1, atk_label1, ATK_RELATION_LABELLED_BY, 0);
+  test_index(atk_button1, atk_label2, ATK_RELATION_LABELLED_BY, 1);
+  test_index(atk_button1, atk_label3, ATK_RELATION_LABELLED_BY, 2);
+
+  test_index(atk_button2, atk_label3, ATK_RELATION_LABELLED_BY, 0);
+  test_index(atk_button2, atk_label2, ATK_RELATION_LABELLED_BY, 1);
+  test_index(atk_button2, atk_label1, ATK_RELATION_LABELLED_BY, 2);
+
+  test_index(atk_button3, atk_label3, ATK_RELATION_LABELLED_BY, 0);
+  test_index(atk_button3, atk_label1, ATK_RELATION_LABELLED_BY, 1);
+  test_index(atk_button3, atk_label2, ATK_RELATION_LABELLED_BY, 2);
+
+  g_object_unref(root_atk_object);
+}
+
 TEST_F(AXPlatformNodeAuraLinuxTest, TestAtkTextTextFieldGetNSelectionsZero) {
   Init(BuildTextField());
   AtkObject* root_atk_object(GetRootAtkObject());

ui/accessibility/platform/ax_platform_node_delegate.h

@@ -252,9 +252,9 @@ class AX_EXPORT AXPlatformNodeDelegate {
       ax::mojom::IntAttribute attr) = 0;
 
   // Given a node ID attribute (one where IsNodeIdIntListAttribute is true),
-  // return a set of all target nodes for which this delegate's node has that
+  // return a vector of all target nodes for which this delegate's node has that
   // relationship attribute.
-  virtual std::set<AXPlatformNode*> GetTargetNodesForRelation(
+  virtual std::vector<AXPlatformNode*> GetTargetNodesForRelation(
       ax::mojom::IntListAttribute attr) = 0;
 
   // Given a node ID attribute (one where IsNodeIdIntAttribute is true), return

ui/accessibility/platform/ax_platform_node_delegate_base.cc

@@ -500,15 +500,27 @@ std::set<AXPlatformNode*> AXPlatformNodeDelegateBase::GetNodesForNodeIds(
   return nodes;
 }
 
-std::set<AXPlatformNode*> AXPlatformNodeDelegateBase::GetTargetNodesForRelation(
+std::vector<AXPlatformNode*>
+AXPlatformNodeDelegateBase::GetTargetNodesForRelation(
     ax::mojom::IntListAttribute attr) {
   DCHECK(IsNodeIdIntListAttribute(attr));
   std::vector<int32_t> target_ids;
   if (!GetData().GetIntListAttribute(attr, &target_ids))
-    return std::set<AXPlatformNode*>();
+    return std::vector<AXPlatformNode*>();
 
-  std::set<int32_t> target_id_set(target_ids.begin(), target_ids.end());
-  return GetNodesForNodeIds(target_id_set);
+  // If we use std::set to eliminate duplicates, the resulting set will be
+  // sorted by the id and we will lose the original order which may be of
+  // interest to ATs. The number of ids should be small.
+
+  std::vector<ui::AXPlatformNode*> nodes;
+  for (int32_t target_id : target_ids) {
+    if (ui::AXPlatformNode* node = GetFromNodeID(target_id)) {
+      if (std::find(nodes.begin(), nodes.end(), node) == nodes.end())
+        nodes.push_back(node);
+    }
+  }
+
+  return nodes;
 }
 
 std::set<AXPlatformNode*> AXPlatformNodeDelegateBase::GetReverseRelations(

ui/accessibility/platform/ax_platform_node_delegate_base.h

@@ -177,9 +177,9 @@ class AX_EXPORT AXPlatformNodeDelegateBase : public AXPlatformNodeDelegate {
       ax::mojom::IntAttribute attr) override;
 
   // Given a node ID attribute (one where IsNodeIdIntListAttribute is true),
-  // return a set of all target nodes for which this delegate's node has that
+  // return a vector of all target nodes for which this delegate's node has that
   // relationship attribute.
-  std::set<AXPlatformNode*> GetTargetNodesForRelation(
+  std::vector<AXPlatformNode*> GetTargetNodesForRelation(
       ax::mojom::IntListAttribute attr) override;
 
   // Given a node ID attribute (one where IsNodeIdIntAttribute is true), return