Do not clear children of inserted nodes

This code in AXNodeObject::InsertChild() to clear the inserted child's
children, was added in WebKit, to support a use case that no longer
exists. Specifically, an aria-hidden=false descendant of
aria-hidden=true is no longer exposed. The spec may change again to
allow that, but it would better to find an implementation that does
not require reworking entire subtrees whenever a node changes.

Performance improvement for dynamic changes in large subtrees.
Also required for CL:2192752 to land.

AX-Relnotes: n/a
Bug: 1107988
Change-Id: I43f1fc880e7740cb937f273db76c6429e43e8d29
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2204154Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802786}

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

@@ -3370,15 +3370,13 @@ void AXNodeObject::InsertChild(AXObject* child, unsigned index) {
   if (!child || !CanHaveChildren())
     return;
 
-  // If the parent is asking for this child's children, then either it's the
-  // first time (and clearing is a no-op), or its visibility has changed. In
-  // the latter case, this child may have a stale child cached.  This can
-  // prevent aria-hidden changes from working correctly. Hence, whenever a
-  // parent is getting children, ensure data is not stale.
-  child->ClearChildren();
-
   if (!child->AccessibilityIsIncludedInTree()) {
-    // Re-computes child's children.
+    // Child is ignored and not in the tree.
+    // Recompute the child's children now as we skip over the ignored object.
+    child->SetNeedsToUpdateChildren();
+    // Get the ignored child's children and add to children of ancestor
+    // included in tree. This will recurse if necessary, skipping levels of
+    // unignored descendants as it goes.
     const auto& children = child->ChildrenIncludingIgnored();
     wtf_size_t length = children.size();
     for (wtf_size_t i = 0; i < length; ++i)

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

@@ -868,6 +868,9 @@ class MODULES_EXPORT AXObject : public GarbageCollected<AXObject> {
   // accessibility tree.
   const AXObjectVector& ChildrenIncludingIgnored() const;
   const AXObjectVector& ChildrenIncludingIgnored();
+  const AXObjectVector& CachedChildrenIncludingIgnored() const {
+    return children_;
+  }
 
   // Returns the node's unignored descendants that are one level deeper than
   // this node, after removing all accessibility ignored nodes from the tree.

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

@@ -840,6 +840,9 @@ void AXObjectCacheImpl::DeferTreeUpdateInternal(base::OnceClosure callback,
   if (!IsActive(GetDocument()) || tree_updates_paused_)
     return;
 
+  if (obj->IsDetached())
+    return;
+
   Document& tree_update_document = *obj->GetDocument();
 
   // Ensure the tree update document is in a good state.
@@ -1010,17 +1013,18 @@ void AXObjectCacheImpl::TextChanged(const LayoutObject* layout_object) {
     return;
   }
 
-  if (layout_object->GetNode() || Get(layout_object)) {
-    DeferTreeUpdate(&AXObjectCacheImpl::TextChangedWithCleanLayout,
-                    layout_object->GetNode(), Get(layout_object));
+  if (Get(layout_object)) {
+    DeferTreeUpdate(&AXObjectCacheImpl::TextChangedWithCleanLayout, nullptr,
+                    Get(layout_object));
   }
 }
 
 void AXObjectCacheImpl::TextChangedWithCleanLayout(
     Node* optional_node_for_relation_update,
     AXObject* obj) {
-  if (!obj && !optional_node_for_relation_update)
+  if (obj ? obj->IsDetached() : !optional_node_for_relation_update)
     return;
+
 #if DCHECK_IS_ON()
   Document* document = obj ? obj->GetDocument()
                            : &optional_node_for_relation_update->GetDocument();
@@ -1118,6 +1122,7 @@ void AXObjectCacheImpl::ChildrenChanged(const LayoutObject* layout_object) {
   if (!layout_object)
     return;
 
+  // Update using nearest node (walking ancestors if necessary).
   Node* node = GetClosestNodeForLayoutObject(layout_object);
 
   if (node) {
@@ -1126,12 +1131,49 @@ void AXObjectCacheImpl::ChildrenChanged(const LayoutObject* layout_object) {
       return;
 
     DeferTreeUpdate(&AXObjectCacheImpl::ChildrenChangedWithCleanLayout, node);
-    return;
-  }
 
-  if (AXObject* obj = Get(layout_object)) {
-    DeferTreeUpdate(&AXObjectCacheImpl::ChildrenChangedWithCleanLayout, nullptr,
-                    obj);
+    if (layout_object->GetNode() == node)
+      return;  // Node matched the layout object passed in, no further updates.
+
+    // Node was for an ancestor of an anonymous layout object passed in.
+    // layout object was anonymous. Fall through to continue updating
+    // descendants of the matching AXObject for the layout object.
+  }
+
+  // Update using layout object.
+  // Only using the layout object when no node could be found to update.
+  AXObject* ax_layout_obj = Get(layout_object);
+  if (!ax_layout_obj)
+    return;
+
+  if (ax_layout_obj->AccessibilityIsIncludedInTree()) {
+    // Participates in tree: update children if they haven't already been.
+    DeferTreeUpdate(&AXObjectCacheImpl::ChildrenChangedWithCleanLayout,
+                    ax_layout_obj->GetNode(), ax_layout_obj);
+  }
+
+  // Invalidate child ax objects below an anonymous layout object.
+  // The passed-in layout object was anonymous, e.g. anonymous block flow
+  // inserted by blink as an inline's parent when it had a block sibling.
+  // If children change on an anonymous layout object, this can
+  // mean that child AXObjects actually had their children change.
+  // Therefore, invalidate any of those children as well, using the nearest
+  // parent that participates in the tree.
+  // In this example, if ChildrenChanged() is called on the anonymous block,
+  // then we also process ChildrenChanged() on the <div> and <a>:
+  // <div>
+  //  |    \
+  // <p>  Anonymous block
+  //         \
+  //         <a>
+  //           \
+  //           text
+  AXObject* ax_parent = ax_layout_obj->ParentObjectIncludedInTree();
+  if (ax_parent) {
+    for (const auto& ax_child : ax_parent->CachedChildrenIncludingIgnored()) {
+      DeferTreeUpdate(&AXObjectCacheImpl::ChildrenChangedWithCleanLayout,
+                      ax_child->GetNode(), ax_child);
+    }
   }
 }
 
@@ -1163,7 +1205,7 @@ void AXObjectCacheImpl::ChildrenChangedWithCleanLayout(Node* node) {
 
 void AXObjectCacheImpl::ChildrenChangedWithCleanLayout(Node* optional_node,
                                                        AXObject* obj) {
-  if (!obj && !optional_node)
+  if (obj ? obj->IsDetached() : !optional_node)
     return;
 
 #if DCHECK_IS_ON()
@@ -1172,7 +1214,7 @@ void AXObjectCacheImpl::ChildrenChangedWithCleanLayout(Node* optional_node,
       << "Unclean document at lifecycle " << document->Lifecycle().ToString();
 #endif  // DCHECK_IS_ON()
 
-  if (obj)
+  if (obj && !obj->IsDetached())
     obj->ChildrenChanged();
 
   if (optional_node) {
@@ -1606,9 +1648,6 @@ void AXObjectCacheImpl::HandleRoleChangeWithCleanLayout(Node* node) {
     // If role changes on a table, invalidate the entire table subtree as many
     // objects may suddenly need to change, because presentation is inherited
     // from the table to rows and cells.
-    // TODO(aleventhal) A size change on a select means the children may need to
-    // switch between AXMenuListOption and AXListBoxOption.
-    // For some reason we don't get attribute changes for @size, though.
     LayoutObject* layout_object = node->GetLayoutObject();
     if (layout_object && layout_object->IsTable())
       InvalidateTableSubtree(obj);
@@ -1641,10 +1680,9 @@ void AXObjectCacheImpl::HandleRoleChangeIfNotEditableWithCleanLayout(
   // However, doing that would require
   // waiting for layout to complete, as ComputeAccessibilityRole() looks at
   // layout objects.
-  if (AXObject* obj = Get(node)) {
-    if (!obj->IsTextControl())
-      HandleRoleChangeWithCleanLayout(node);
-  }
+  AXObject* obj = Get(node);
+  if (!obj || !obj->IsTextControl())
+    HandleRoleChangeWithCleanLayout(node);
 }
 
 void AXObjectCacheImpl::HandleAttributeChanged(const QualifiedName& attr_name,

third_party/blink/web_tests/accessibility/change-to-whitespace.html

+<!DOCTYPE HTML>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+
+<div id="container">
+  <ol id="list1">
+    $<li>A</li>
+    <li>B</li>$
+  </ol>
+  $<ul id="list2" style="list-style-type:none">
+    <li>A</li>
+    <li>B</li>
+  </ul>$
+</div>
+
+<script>
+function axElementById(id) {
+    return accessibilityController.accessibleElementById(id);
+}
+
+function convertDollarToWhitespace(node) {
+  let child = node.firstChild;
+  while (child) {
+    if (child.nodeType == Node.ELEMENT_NODE)
+      convertDollarToWhitespace(child);
+    else if (child.nodeType == Node.TEXT_NODE && child.data.indexOf('$') >= 0)
+      child.data = '  \n ';
+    child = child.nextSibling;
+  }
+}
+
+test(function(t) {
+    const container = axElementById('container');
+    assert_equals(container.childrenCount, 4);
+    const axList1 = axElementById("list1");
+    assert_equals(axList1.childrenCount, 4);
+    const axList2 = axElementById("list2");
+    assert_equals(axList2.childrenCount, 2);
+
+    convertDollarToWhitespace(document.getElementById('container'));
+
+    assert_equals(container.childrenCount, 2);
+    assert_equals(axList1.childrenCount, 2);
+    assert_equals(axList2.childrenCount, 2);
+}, "Changing text to whitespace-only should remove it from ax object tree");
+
+</script>