AX: AXIsolatedTree::updateChildren sometimes fails to update isolated subtrees when...

https://bugs.webkit.org/show_bug.cgi?id=241735

Reviewed by Andres Gonzalez.

Our current algorithm in AXIsolatedTree::updateChildren is:

  1. If the object we got an AXChildrenChanged notification
  for is in the isolated tree, update its isolated children

  2. Otherwise, ascend the ancestry to the nearest
  in-isolated-tree ancestor, and update the children of that
  object

This is not always adequate when the object passed to updateChildren
is ignored, as in some cases the ancestor has no children changes
but the subtrees of the ignored object do.

This patch fixes this by also checking the live-children of the ignored
object for any that have unitialized children. If so, we call
AXIsolatedTree::updateChildren on those too.

* Source/WebCore/accessibility/AccessibilityObject.h:
(WebCore::AccessibilityObject::childrenInitialized const):
Move from private to public.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::updateChildren):

link: https://commits.webkit.org/251784@main
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@295779 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h
index 8a1ebea64bb58539445658478c15a0930e923eff..831242b482fac82c66279ce3f76d16fc6c1d46b3 100644 (file)
--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h
@@ -497,6 +497,7 @@ public:
     void decrement() override { }
 
     virtual void updateRole() { }
+    bool childrenInitialized() const { return m_childrenInitialized; }
     const AccessibilityChildrenVector& children(bool updateChildrenIfNeeded = true) override;
     virtual void addChildren() { }
     enum class DescendIfIgnored : uint8_t { No, Yes };
@@ -827,7 +828,6 @@ private:
 #endif
     
 protected: // FIXME: Make the data members private.
-    bool childrenInitialized() const { return m_childrenInitialized; }
     AccessibilityChildrenVector m_children;
     mutable bool m_childrenInitialized { false };
     AccessibilityRole m_role { AccessibilityRole::Unknown };

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 27fd89fc98f32854f4daa2ab6f781e08685346a3..b26e162cf32bcbdfcb79f5c19abd3546458a93a0 100644 (file)
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -209,20 +209,20 @@ void AXIsolatedTree::generateSubtree(AXCoreObject& axObject)
     queueRemovalsAndUnresolvedChanges({ });
 }
 
+static bool shouldCreateNodeChange(AXCoreObject& axObject)
+{
+    // We should never create an isolated object from an ignored object or one with an invalid ID.
+    return !axObject.accessibilityIsIgnored() && axObject.objectID().isValid();
+}
+
 std::optional<AXIsolatedTree::NodeChange> AXIsolatedTree::nodeChangeForObject(Ref<AXCoreObject> axObject, AttachWrapper attachWrapper)
 {
     ASSERT(isMainThread());
+    ASSERT(axObject->objectID().isValid());
 
-    // We should never create an isolated object from an ignored object.
-    if (axObject->accessibilityIsIgnored())
+    if (!shouldCreateNodeChange(axObject.get()))
         return std::nullopt;
 
-    if (!axObject->objectID().isValid()) {
-        // Either the axObject has an invalid ID or something else went terribly wrong. Don't bother doing anything else.
-        ASSERT_NOT_REACHED();
-        return std::nullopt;
-    }
-
     auto object = AXIsolatedObject::create(axObject, this);
     NodeChange nodeChange { object, nullptr };
 
@@ -458,7 +458,7 @@ void AXIsolatedTree::updateNodeAndDependentProperties(AXCoreObject& axObject)
         updateNodeProperty(*treeAncestor, AXPropertyName::ARIATreeRows);
 }
 
-void AXIsolatedTree::updateChildren(AXCoreObject& axObject)
+void AXIsolatedTree::updateChildren(AXCoreObject& axObject, ResolveNodeChanges resolveNodeChanges)
 {
     AXTRACE("AXIsolatedTree::updateChildren"_s);
     AXLOG("For AXObject:");
@@ -489,12 +489,34 @@ void AXIsolatedTree::updateChildren(AXCoreObject& axObject)
         return;
     }
 
-#ifndef NDEBUG
     if (axAncestor != &axObject) {
         AXLOG(makeString("Original object with ID ", axObject.objectID().loggingString(), " wasn't in the isolated tree, so instead updating the closest in-isolated-tree ancestor:"));
         AXLOG(axAncestor);
+        for (auto& child : axObject.children()) {
+            auto* liveChild = dynamicDowncast<AccessibilityObject>(child.get());
+            if (!liveChild || liveChild->childrenInitialized())
+                continue;
+
+            if (!m_nodeMap.contains(liveChild->objectID())) {
+                if (!shouldCreateNodeChange(*liveChild))
+                    continue;
+
+                // This child should be added to the isolated tree but hasn't been yet.
+                // Add it to the nodemap so the recursive call to updateChildren below properly builds the subtree for this object.
+                auto* parent = liveChild->parentObjectUnignored();
+                m_nodeMap.set(liveChild->objectID(), ParentChildrenIDs { parent ? parent->objectID() : AXID(), liveChild->childrenIDs() });
+                m_unresolvedPendingAppends.set(liveChild->objectID(), AttachWrapper::OnMainThread);
+            }
+
+            AXLOG(makeString(
+                "Child ID ", liveChild->objectID().loggingString(), " of original object ID ", axObject.objectID().loggingString(), " was found in the isolated tree with uninitialized live children. Updating its isolated children."
+            ));
+            // Don't immediately resolve node changes in these recursive calls to updateChildren. This avoids duplicate node change creation in this scenario:
+            //   1. Some subtree is updated in the below call to updateChildren.
+            //   2. Later in this function, when updating axAncestor, we update some higher subtree that includes the updated subtree from step 1.
+            updateChildren(*liveChild, ResolveNodeChanges::No);
+        }
     }
-#endif
 
     auto oldIDs = m_nodeMap.get(axAncestor->objectID());
     auto& oldChildrenIDs = oldIDs.childrenIDs;
@@ -528,7 +550,11 @@ void AXIsolatedTree::updateChildren(AXCoreObject& axObject)
         if (axID.isValid())
             removeSubtreeFromNodeMap(axID, axAncestor);
     }
-    queueRemovalsAndUnresolvedChanges(oldChildrenIDs);
+
+    if (resolveNodeChanges == ResolveNodeChanges::Yes)
+        queueRemovalsAndUnresolvedChanges(oldChildrenIDs);
+    else
+        queueRemovals(oldChildrenIDs);
 
     // Also queue updates to the target node itself and any properties that depend on children().
     updateNodeAndDependentProperties(*axAncestor);

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index b1a594c98525b78ee3b8dae12ab0213c5d81868c..0317b2ee887e530ccd54c160808a8d2b518b3568 100644 (file)
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -346,7 +346,8 @@ public:
 
     void generateSubtree(AXCoreObject&);
     void updateNode(AXCoreObject&);
-    void updateChildren(AXCoreObject&);
+    enum class ResolveNodeChanges : bool { No, Yes };
+    void updateChildren(AXCoreObject&, ResolveNodeChanges = ResolveNodeChanges::Yes);
     void updateNodeProperty(AXCoreObject&, AXPropertyName);
     void updateNodeAndDependentProperties(AXCoreObject&);