Do not remove objects from relation cache when refreshing them

When replacing an AXObject with a different type of object, and keeping
the same id, we don't want to remove it from the relation cache, is its
relations are still the same.

In addition, make sure that AXIDs that are removed are removed from
all relevant maps, so that the condition cannot occur where an AXID
is removed from an owner children map, but is still tracked by
the owner, thus its owned children do not think they are owned.

The best failing test for this is in CL:2585685 which turns on
accessibility for all browser tests, via
browser_tests --gtest_filter="HistoryListTest.*".

Bug: 1153576,1156936
Change-Id: I0bb4d38f78b343bd34d2e42e1cf695fcfd99460e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2584610
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836772}

content/browser/accessibility/dump_accessibility_tree_browsertest.cc

@@ -1553,6 +1553,16 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityCustomElement) {
   RunHtmlTest(FILE_PATH_LITERAL("custom-element.html"));
 }
 
+IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
+                       AccessibilityCustomElementWithAriaOwnsOutside) {
+  RunHtmlTest(FILE_PATH_LITERAL("custom-element-with-aria-owns-outside.html"));
+}
+
+IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
+                       AccessibilityCustomElementWithAriaOwnsInside) {
+  RunHtmlTest(FILE_PATH_LITERAL("custom-element-with-aria-owns-inside.html"));
+}
+
 IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityEm) {
   RunHtmlTest(FILE_PATH_LITERAL("em.html"));
 }

content/test/data/accessibility/html/custom-element-with-aria-owns-inside-expected-blink.txt

+rootWebArea
+++genericContainer ignored
+++++genericContainer ignored
+++++++group
+++++++++docEndnotes
+++++++++++presentational ignored
+++++++++++++docEndnote
+++++++++++++++staticText name='Endnote 1'
+++++++++++++++++inlineTextBox name='Endnote 1'
+++++++++++++docEndnote
+++++++++++++++staticText name='Endnote 2'
+++++++++++++++++inlineTextBox name='Endnote 2'
+++++++++button ignored invisible
+++++++++button ignored invisible

content/test/data/accessibility/html/custom-element-with-aria-owns-inside.html

+<template id="test-contents">
+  <div role="doc-endnotes">
+    <div role="presentation">
+      <span role="doc-endnote" id="e1">Endnote 1</span>
+      <div role="doc-endnote" id="e2">Endnote 2</div>
+    </div>
+  </div>
+</template>
+<test-element role="group" aria-owns="1 2">
+  <div role="group">
+    <div role="presentation">
+      <span role="button" id="1">1</span>
+      <div role="button" id="2">2</div>
+    </div>
+   </div>
+</test-element>
+<script>
+  class TestElement extends HTMLElement {
+    constructor() {
+      super();
+
+      const testContents = document.getElementById('test-contents');
+      this.attachShadow({mode: 'open'}).innerHTML = testContents.innerHTML;
+    }
+  }
+  customElements.define('test-element', TestElement);
+</script>

content/test/data/accessibility/html/custom-element-with-aria-owns-outside-expected-blink.txt

+rootWebArea
+++genericContainer ignored
+++++genericContainer ignored
+++++++splitter horizontal name='Outside custom element'
+++++++paragraph
+++++++++staticText name='Hello, world'
+++++++++++inlineTextBox name='Hello, world'
+++++++genericContainer ignored invisible
+++++++splitter horizontal name='Inside custom element'
+++++++group
+++++++++button name='something'
+++++++++++staticText name='something'
+++++++++++++inlineTextBox name='something'
+++++++++button name='something'
+++++++++++staticText name='something'
+++++++++++++inlineTextBox name='something'
+++++++staticText name=' '
+++++++++inlineTextBox name=' '

content/test/data/accessibility/html/custom-element-with-aria-owns-outside.html

+<hr aria-label="Outside custom element">
+<p>Hello, world</p>
+<template id="test-contents">something</template>
+<hr aria-label="Inside custom element">
+<div role="group" aria-owns="t1 t2"></div>
+<test-element role="button" id="t1"></test-element>
+<test-element role="button" id="t2"></test-element>
+<script>
+  class TestElement extends HTMLElement {
+    constructor() {
+      super();
+
+      const testContents = document.getElementById('test-contents');
+      this.attachShadow({mode: 'open'}).innerHTML = testContents.innerHTML;
+    }
+  }
+  customElements.define('test-element', TestElement);
+</script>

third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc

@@ -732,6 +732,8 @@ void AXObjectCacheImpl::Remove(AXID ax_id) {
   RemoveAXID(obj);
 
   // Finally, remove the object.
+  // TODO(accessibility) We don't use the return value, can we use .erase()
+  // and it will still make sure that the object is cleaned up?
   if (!objects_.Take(ax_id))
     return;
 
@@ -1392,7 +1394,15 @@ void AXObjectCacheImpl::ProcessInvalidatedObjects(Document& document) {
     DCHECK(node) << "Refresh() is currently only supported for objects "
                     "with a backing node";
     AXID retained_axid = current->AXObjectID();
-    Remove(current);
+    // Remove from relevant maps, but not from relation cache, as the relations
+    // between AXIDs will still the same.
+    node_object_mapping_.erase(node);
+    if (current->GetLayoutObject())
+      layout_object_mapping_.erase(current->GetLayoutObject());
+    current->Detach();
+    // TODO(accessibility) We don't use the return value, can we use .erase()
+    // and it will still make sure that the object is cleaned up?
+    objects_.Take(retained_axid);
     return CreateAndInit(node, retained_axid);
   };
 

third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc

@@ -201,8 +201,10 @@ void AXRelationCache::GetAriaOwnedChildren(
       aria_owner_to_children_mapping_.at(owner->AXObjectID());
   for (AXID child_id : current_child_axids) {
     AXObject* child = ObjectFromAXID(child_id);
-    if (child)
+    if (child) {
       validated_owned_children_result.push_back(child);
+      DCHECK(IsAriaOwned(child)) << "Owned child not in owned child map";
+    }
   }
 }
 
@@ -368,12 +370,33 @@ void AXRelationCache::UpdateRelatedText(Node* node) {
 }
 
 void AXRelationCache::RemoveAXID(AXID obj_id) {
+  // Need to remove from maps.
+  // There are maps from children to their owners, and owners to their children.
+  // In addition, the removed id may be an owner, or be owned, or both.
+
+  // |obj_id| owned others:
   if (aria_owner_to_children_mapping_.Contains(obj_id)) {
+    // |obj_id| longer owns anything.
     Vector<AXID> child_axids = aria_owner_to_children_mapping_.at(obj_id);
     aria_owned_child_to_owner_mapping_.RemoveAll(child_axids);
+    // Owned children are no longer owned by |obj_id|
     aria_owner_to_children_mapping_.erase(obj_id);
   }
-  aria_owned_child_to_owner_mapping_.erase(obj_id);
+
+  // Another id owned |obj_id|:
+  if (aria_owned_child_to_owner_mapping_.Contains(obj_id)) {
+    // Previous owner no longer relevant to this child.
+    // Also, remove |obj_id| from previous owner's owned child list:
+    AXID owner_id = aria_owned_child_to_owner_mapping_.Take(obj_id);
+    const Vector<AXID>& owners_owned_children =
+        aria_owner_to_children_mapping_.at(owner_id);
+    for (wtf_size_t index = 0; index < owners_owned_children.size(); index++) {
+      if (owners_owned_children[index] == obj_id) {
+        aria_owner_to_children_mapping_.at(owner_id).EraseAt(index);
+        break;
+      }
+    }
+  }
 }
 
 AXObject* AXRelationCache::ObjectFromAXID(AXID axid) const {

third_party/blink/renderer/modules/accessibility/ax_relation_cache.h

@@ -28,6 +28,7 @@ class AXRelationCache {
 
   // Returns true if the given object's position in the tree was due to
   // aria-owns.
+  bool IsAriaOwned(AXID) const;
   bool IsAriaOwned(const AXObject*) const;
 
   // Returns the parent of the given object due to aria-owns.