Bug 262711 – AX: AXIsolatedTree::updateChildren should not updateNodeAndDependentProperties if children haven't changed

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

The intention of the updateNodeAndDependentProperties at the end of updateChildren was to update any properties that depend on AX children (of which there are many). However, we are doing this unconditionally, even when children don't change, which causes lots of wasted work.

[Radar WebKit Bug Importer]

<rdar://problem/116531844>

[Tyler Wilcock]

Created attachment 468075 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 468075 [details]
> Patch

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 2f62a847ef15..57c6da0ccdce 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -668,7 +668,7 @@ void AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj
             if (ancestor->accessibilityIsIgnored())
                 break;
             // Use `updateChildren` rather than `updateNodeProperty` because `updateChildren` will ensure the columns (which are children) will have associated isolated objects created.
-            updateChildren(*ancestor);
+            updateChildren(*ancestor, ResolveNodeChanges::No);
             break;
         }
     }
@@ -747,6 +747,7 @@ void AXIsolatedTree::updateChildren(AccessibilityObject& axObject, ResolveNodeCh
     const auto& newChildren = axAncestor->children();
     auto newChildrenIDs = axAncestor->childrenIDs(false);

+    bool childrenChanged = oldChildrenIDs.size() != newChildrenIDs.size();
     for (size_t i = 0; i < newChildren.size(); ++i) {
         ASSERT(newChildren[i]->objectID() == newChildrenIDs[i]);
         ASSERT(newChildrenIDs[i].isValid());
@@ -757,29 +758,57 @@ void AXIsolatedTree::updateChildren(AccessibilityObject& axObject, ResolveNodeCh

             // Propagate any subtree updates downwards for this already-existing child.
             if (auto* liveChild = dynamicDowncast<AccessibilityObject>(newChildren[i].get()); liveChild && liveChild->hasDirtySubtree())
-                collectNodeChangesForSubtree(*liveChild);
+                updateChildren(*liveChild, ResolveNodeChanges::No);
         }
         else {
             // This is a new child, add it to the tree.
+            childrenChanged = true;
             AXLOG(makeString("AXID ", axAncestor->objectID().loggingString(), " gaining new subtree, starting at ID ", newChildren[i]->objectID().loggingString(), ":"));
             AXLOG(newChildren[i]);
             collectNodeChangesForSubtree(*newChildren[i]);
         }
     }
     m_nodeMap.set(axAncestor->objectID(), ParentChildrenIDs { oldIDs.parentID, WTFMove(newChildrenIDs) });
+    // Since axAncestor is definitively part of the AX tree by way of getting here, protect it from being
+    // deleted in case it has been re-parented.
+    m_protectedFromDeletionIDs.add(axAncestor->objectID());

     // What is left in oldChildrenIDs are the IDs that are no longer children of axAncestor.
     // Thus, remove them from m_nodeMap and queue them to be removed from the tree.
     for (const AXID& axID : oldChildrenIDs)
         removeSubtreeFromNodeMap(axID, axAncestor);

+    auto unconditionallyUpdate = [] (AccessibilityRole role) {
+        // These are the roles that should be updated even if AX children don't change. This is necessary because
+        // these roles are not allowed to have children according to accessibility semantics, but can have render
+        // tree or DOM children, changes of which affect many properties (e.g. anything downstream of textUnderElement).
+        // Note this is a subset of the roles in AccessibilityObject::canHaveChildren, deliberately only those that
+        // could reasonably have meaningful-to-accessibility DOM / render tree children.
+        switch (role) {
+        case AccessibilityRole::Button:
+        case AccessibilityRole::PopUpButton:
+        case AccessibilityRole::Tab:
+        case AccessibilityRole::ToggleButton:
+        case AccessibilityRole::ListBoxOption:
+        case AccessibilityRole::ProgressIndicator:
+        case AccessibilityRole::Switch:
+        case AccessibilityRole::MenuItemCheckbox:
+        case AccessibilityRole::MenuItemRadio:
+        case AccessibilityRole::Meter:
+            return true;
+        default:
+            return false;
+        }
+    };
+
+    // Also queue updates to the target node itself and any properties that depend on children().
+    if (childrenChanged || unconditionallyUpdate(axAncestor->roleValue()))
+        updateNodeAndDependentProperties(*axAncestor);

AG: We were doing updateNodeAndDependentProperties after the queueing below, does it matter to do it before?
+
     if (resolveNodeChanges == ResolveNodeChanges::Yes)
         queueRemovalsAndUnresolvedChanges(WTFMove(oldChildrenIDs));
     else
         queueRemovals(WTFMove(oldChildrenIDs));
-
-    // Also queue updates to the target node itself and any properties that depend on children().
-    updateNodeAndDependentProperties(*axAncestor);
 }

 void AXIsolatedTree::updateChildrenForObjects(const ListHashSet<RefPtr<AccessibilityObject>>& axObjects)

[Tyler Wilcock]

> +    // Also queue updates to the target node itself and any properties that
> depend on children().
> +    if (childrenChanged || unconditionallyUpdate(axAncestor->roleValue()))
> +        updateNodeAndDependentProperties(*axAncestor);
> 
> AG: We were doing updateNodeAndDependentProperties after the queueing below,
> does it matter to do it before?
> +
>      if (resolveNodeChanges == ResolveNodeChanges::Yes)
>          queueRemovalsAndUnresolvedChanges(WTFMove(oldChildrenIDs));
>      else
>          queueRemovals(WTFMove(oldChildrenIDs));
> -
> -    // Also queue updates to the target node itself and any properties that
> depend on children().
> -    updateNodeAndDependentProperties(*axAncestor);
>  }

It's an improvement to do it before before queuing, since with the order in main today it's possible to do a bit of wasted work in this sequence:

1. updateChildren traverses and queues an m_unresolvedPendingAppend for ID 5 (presumably axAncestor)
2. We resolve the unresolved pending appends via queueRemovalsAndUnresolvedChanges, creating a node change for ID 5
3. updateNodeAndDependentProperties(axAncestor) then runs, immediately creating another node change for it, which is unnecessary

Other things inside updateNodeAndDependentProperties also can create some node changes that can be de-duplicated by moving it earlier, e.g.:

if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr)
        updateNode(*correspondingControl);

[EWS]

Committed 269088@main (d73ecd9497ab): <https://commits.webkit.org/269088@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 468075 [details].