Bug 265702 – AX: De-duplicate node changes

• Component: Accessibility
• Status: RESOLVED FIXED

[Joshua Hoffman]

We do a lot of unnecessary node change work, for example, updating a property on a node when that node itself was already updated. This should be de-duplicated.

[Radar WebKit Bug Importer]

<rdar://problem/119052949>

[Joshua Hoffman]

Created attachment 468837 [details]
Patch

[Joshua Hoffman]

Created attachment 468853 [details]
Patch

[Joshua Hoffman]

Created attachment 468900 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #4)
> Created attachment 468900 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 0a75f73aecb2..775b709056ad 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -245,6 +245,7 @@ AXObjectCache::AXObjectCache(Document& document)
     , m_buildIsolatedTreeTimer(*this, &AXObjectCache::buildIsolatedTree)
     , m_geometryManager(AXGeometryManager::create(*this))
     , m_selectedTextRangeTimer(*this, &AXObjectCache::selectedTextRangeTimerFired, platformSelectedTextRangeDebounceInterval())
+    , m_updateTreeSnapshotTimer(*this, &AXObjectCache::updateTreeSnapshotTimerFired)
 #endif
 {
     AXTRACE(makeString("AXObjectCache::AXObjectCache 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));
@@ -281,6 +282,7 @@ AXObjectCache::~AXObjectCache()

 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
     m_selectedTextRangeTimer.stop();
+    m_updateTreeSnapshotTimer.stop();

     if (m_pageID) {
         if (auto tree = AXIsolatedTree::treeForPageID(*m_pageID))
@@ -4127,7 +4129,7 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         auto updatedFields = updatedObjects.get(axObject->objectID());
         if (!updatedFields.node) {
             updatedObjects.set(axObject->objectID(), UpdatedFields { updatedFields.children, true });
-            tree->updateNode(*axObject);
+            tree->queueNodeUpdate(*axObject, { { }, true, false });
         }
     };

@@ -4138,126 +4140,126 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili

         switch (notification.second) {
         case AXAccessKeyChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::AccessKey);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::AccessKey });
             break;
         case AXAutofillTypeChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::ValueAutofillButtonType);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::ValueAutofillButtonType });
             break;
         case AXCellSlotsChanged:
             ASSERT(notification.first->isTable());
-            tree->updateNodeProperty(*notification.first, AXPropertyName::CellSlots);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::CellSlots });
             break;
         case AXCheckedStateChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::IsChecked);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsChecked });
             break;
         case AXCurrentStateChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::CurrentState);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::CurrentState });
             break;
         case AXColumnCountChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::AXColumnCount);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::AXColumnCount });
             break;
         case AXColumnIndexChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::AXColumnIndex);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::AXColumnIndex });
             break;
         case AXColumnSpanChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::ColumnIndexRange);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::ColumnIndexRange });
             break;
         case AXContentEditableAttributeChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::IsNonNativeTextControl);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsNonNativeTextControl });
             break;
         case AXDisabledStateChanged:
-            tree->updatePropertiesForSelfAndDescendants(*notification.first, { AXPropertyName::CanSetFocusAttribute, AXPropertyName::IsEnabled });
+            tree->updatePropertiesForSelfAndDescendants(*notification.first, { { AXPropertyName::CanSetFocusAttribute, AXPropertyName::IsEnabled } });
             break;
         case AXExpandedChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::IsExpanded);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsExpanded });
             break;
         case AXExtendedDescriptionChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::ExtendedDescription);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::ExtendedDescription });
             break;
         case AXFocusableStateChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::CanSetFocusAttribute);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::CanSetFocusAttribute });
             break;
         case AXLabelCreated:
             tree->labelCreated(*notification.first);
             break;
         case AXMaximumValueChanged:
-            tree->updateNodeProperties(*notification.first, { AXPropertyName::MaxValueForRange, AXPropertyName::ValueForRange });
+            tree->queueNodeUpdate(*notification.first, { { AXPropertyName::MaxValueForRange, AXPropertyName::ValueForRange } });
             break;
         case AXMenuListItemSelected: {
             RefPtr ancestor = Accessibility::findAncestor<AccessibilityObject>(*notification.first, false, [] (const auto& object) {
                 return object.isMenu() || object.isMenuBar();
             });
             if (ancestor) {
-                tree->updateNodeProperty(*ancestor, AXPropertyName::SelectedChildren);
-                tree->updateNodeProperty(*notification.first, AXPropertyName::IsSelected);
+                tree->queueNodeUpdate(*ancestor, { AXPropertyName::SelectedChildren });
+                tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsSelected });
             }
             break;
         }
         case AXMinimumValueChanged:
-            tree->updateNodeProperties(*notification.first, { AXPropertyName::MinValueForRange, AXPropertyName::ValueForRange });
+            tree->queueNodeUpdate(*notification.first, { { AXPropertyName::MinValueForRange, AXPropertyName::ValueForRange } });
             break;
         case AXNameChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::NameAttribute);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::NameAttribute });
             break;
         case AXOrientationChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::Orientation);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::Orientation });
             break;
         case AXPositionInSetChanged:
-            tree->updateNodeProperties(*notification.first, { AXPropertyName::PosInSet, AXPropertyName::SupportsPosInSet });
+            tree->queueNodeUpdate(*notification.first, { { AXPropertyName::PosInSet, AXPropertyName::SupportsPosInSet } });
             break;
         case AXPopoverTargetChanged:
-            tree->updateNodeProperties(*notification.first, { AXPropertyName::SupportsExpanded, AXPropertyName::IsExpanded });
+            tree->queueNodeUpdate(*notification.first, { { AXPropertyName::SupportsExpanded, AXPropertyName::IsExpanded } });
             break;
         case AXSelectedTextChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::SelectedTextRange);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::SelectedTextRange });
             break;
         case AXSortDirectionChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::SortDirection);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::SortDirection });
             break;
         case AXIdAttributeChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::IdentifierAttribute);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::IdentifierAttribute });
             break;
         case AXReadOnlyStatusChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::CanSetValueAttribute);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::CanSetValueAttribute });
             break;
         case AXRequiredStatusChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::IsRequired);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsRequired });
             break;
         case AXRoleDescriptionChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::RoleDescription);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::RoleDescription });
             break;
         case AXRowIndexChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::AXRowIndex);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::AXRowIndex });
             break;
         case AXRowSpanChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::RowIndexRange);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::RowIndexRange });
             break;
         case AXCellScopeChanged:
-            tree->updateNodeProperties(*notification.first, { AXPropertyName::CellScope, AXPropertyName::IsColumnHeader, AXPropertyName::IsRowHeader });
+            tree->queueNodeUpdate(*notification.first, { { AXPropertyName::CellScope, AXPropertyName::IsColumnHeader, AXPropertyName::IsRowHeader } });
             break;
         //  FIXME: Contrary to the name "AXSelectedCellsChanged", this notification can be posted on a cell
         //  who has changed selected state, not just on table or grid who has changed its selected cells.
         case AXSelectedCellsChanged:
         case AXSelectedStateChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::IsSelected);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsSelected });
             break;
         case AXSetSizeChanged:
-            tree->updateNodeProperties(*notification.first, { AXPropertyName::SetSize, AXPropertyName::SupportsSetSize });
+            tree->queueNodeUpdate(*notification.first, { { AXPropertyName::SetSize, AXPropertyName::SupportsSetSize } });
             break;
         case AXTableHeadersChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::ColumnHeaders);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::ColumnHeaders });
             break;
         case AXTextCompositionChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::TextInputMarkedTextMarkerRange);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::TextInputMarkedTextMarkerRange });
             break;
         case AXURLChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::URL);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::URL });
             break;
         case AXKeyShortcutsChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::KeyShortcuts);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::KeyShortcuts });
             break;
         case AXVisibilityChanged:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::IsVisible);
+            tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsVisible });
             break;
         case AXActiveDescendantChanged:
         case AXRoleChanged:
@@ -4298,7 +4300,7 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
             auto updatedFields = updatedObjects.get(notification.first->objectID());
             if (!updatedFields.children) {
                 updatedObjects.set(notification.first->objectID(), UpdatedFields { true, updatedFields.node });
-                tree->updateChildren(*notification.first);
+                tree->queueNodeUpdate(*notification.first, { { }, false, true });
             }
             break;
         }
@@ -4317,7 +4319,13 @@ void AXObjectCache::updateIsolatedTree(AccessibilityObject* axObject, AXProperty
 void AXObjectCache::updateIsolatedTree(AccessibilityObject& axObject, AXPropertyName property) const
 {
     if (RefPtr tree = AXIsolatedTree::treeForPageID(m_pageID))
-        tree->updateNodeProperty(axObject, property);
+        tree->queueNodeUpdate(axObject, { property });
+}
+
+void AXObjectCache::startSnapshotTimer()
+{
+    if (!m_updateTreeSnapshotTimer.isActive())
+        m_updateTreeSnapshotTimer.startOneShot(100_ms);

AG: We should declare the elapse time as a const in the consts section of this file.

 }

 void AXObjectCache::onPaint(const RenderObject& renderer, IntRect&& paintRect) const
@@ -4905,6 +4913,16 @@ void AXObjectCache::selectedTextRangeTimerFired()

     m_lastDebouncedTextRangeObject = { };
 }
+
+void AXObjectCache::updateTreeSnapshotTimerFired()
+{
+    if (!accessibilityEnabled())
+        return;
+
+    m_updateTreeSnapshotTimer.stop();
+    if (auto tree = getOrCreateIsolatedTree())

AG: use isolatedTreeForPageID. getOrCreateIsolatedTree should be used only in the paths that we purposely want to create the iso tree, namely, a client request for root or focus.

+        tree->processQueuedNodeUpdates();
+}
 #endif

 void AXObjectCache::onWidgetVisibilityChanged(RenderWidget* widget)

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #4)
> Created attachment 468900 [details]
> Patch

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 890a0ec4857c..44a394534ca2 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -532,7 +532,7 @@ void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject&
         return;

     Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&properties, this] (auto& descendant) {
-        updateNodeProperties(descendant, properties);
+        queueNodeUpdate(descendant, { properties });
     });
 }

@@ -733,7 +733,7 @@ void AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj
     bool updateTableAncestorColumns = is<AccessibilityTableRow>(axObject);
     for (auto* ancestor = axObject.parentObject(); ancestor; ancestor = ancestor->parentObject()) {
         if (ancestor->isTree()) {
-            updateNodeProperty(*ancestor, AXPropertyName::ARIATreeRows);
+            queueNodeUpdate(*ancestor, { AXPropertyName::ARIATreeRows });
             if (!updateTableAncestorColumns)
                 break;
         }
@@ -990,7 +990,7 @@ void AXIsolatedTree::labelCreated(AccessibilityObject& axObject)
 {
     ASSERT(axObject.isLabel());
     if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr)
-        updateNode(*correspondingControl);
+        queueNodeUpdate(*correspondingControl, { { }, true, false });
 }

 void AXIsolatedTree::updateLoadingProgress(double newProgressValue)
@@ -1238,6 +1238,67 @@ AXTreePtr findAXTree(Function<bool(AXTreePtr)>&& match)
 #endif
 }

+void AXIsolatedTree::queueNodeUpdate(AXCoreObject& object, NodeUpdateOptions options)
+{
+    ASSERT(isMainThread());
+
+    // If we are updating a property, but our node is in m_needsUpdateNode, don't queue anything.
+    if (options.properties.size()) {
+        if (m_needsUpdateNode.contains(object.objectID()))
+            return;

AG: what about if the caller passed { someProperties, true, false }, i.e., requested a node update and also passed some properties.

+
+        Vector<AXPropertyName> properties;
+        if (m_needsPropertyUpdates.contains(object.objectID()))
+            properties = m_needsPropertyUpdates.get(object.objectID());
+
+        for (const auto& property : options.properties)
+            properties.appendIfNotContains(property);
+
+        m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties));
+    }
+
+    if (options.shouldUpdateChildren && !m_needsUpdateChildren.contains(object.objectID()))
+        m_needsUpdateChildren.add(object.objectID());
+
+    if (options.shouldUpdateNode && !m_needsUpdateNode.contains(object.objectID()))
+        m_needsUpdateNode.add(object.objectID());
+
+    if (auto* cache = axObjectCache())
+        cache->startSnapshotTimer();
+}
+
+void AXIsolatedTree::processQueuedNodeUpdates()
+{
+    ASSERT(isMainThread());
+
+    auto* cache = axObjectCache();
+    if (!cache)
+        return;
+
+    for (AXID nodeID : m_needsUpdateChildren) {
+        if (auto* axObject = cache->objectForID(nodeID))
+            updateChildren(*axObject, ResolveNodeChanges::No);
+    }
+    m_needsUpdateChildren.clear();
+
+    for (AXID nodeID : m_needsUpdateNode) {
+        if (!m_unresolvedPendingAppends.contains(nodeID))
+            m_unresolvedPendingAppends.set(nodeID, AttachWrapper::OnMainThread);
+    }
+    m_needsUpdateNode.clear();
+
+    for (const auto& propertyUpdate : m_needsPropertyUpdates) {
+        if (m_unresolvedPendingAppends.contains(propertyUpdate.key))
+            break;
+
+        if (auto* axObject = cache->objectForID(propertyUpdate.key))
+            updateNodeProperties(*axObject, propertyUpdate.value);
+    }
+    m_needsPropertyUpdates.clear();
+
+    queueRemovalsAndUnresolvedChanges({ });
+}
+
 } // namespace WebCore

 #endif // ENABLE(ACCESSIBILITY_ISOLATED_TREE)
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index c9609b8c8b1d..1bb2fd444ddc 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -262,6 +262,26 @@ struct AXPropertyChange {
     AXPropertyMap properties; // Changed properties.
 };

+struct NodeUpdateOptions {
+    Vector<AXPropertyName> properties;
+    bool shouldUpdateNode { false };
+    bool shouldUpdateChildren { false };
+
+    NodeUpdateOptions(Vector<AXPropertyName> propertyNames, bool shouldUpdateNode, bool shouldUpdateChildren)
+        : properties(propertyNames)
+        , shouldUpdateNode(shouldUpdateNode)
+        , shouldUpdateChildren(shouldUpdateChildren)
+    { }

AG: pass the Vector by const& or &&.
+
+    NodeUpdateOptions(Vector<AXPropertyName> propertyNames)
+        : properties(propertyNames)
+    { }

AG: same here. Does the compileer require to explicitly define these "default" constructors?

AG: does the order of the properties matter? or we could use a set instead of a vector?

+
+    NodeUpdateOptions(AXPropertyName propertyName)
+        : properties({ propertyName })
+    { }
+};
+
 DECLARE_ALLOCATOR_WITH_HEAP_IDENTIFIER(AXIsolatedTree);
 class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AXIsolatedTree>
     , public AXTreeStore<AXIsolatedTree> {
@@ -336,6 +356,9 @@ public:
     AXTextMarkerRange selectedTextMarkerRange();
     void setSelectedTextMarkerRange(AXTextMarkerRange&&);

+    void queueNodeUpdate(AXCoreObject&, NodeUpdateOptions);

AG: pass NodeUpdateOptions by const& or &&.

+    void processQueuedNodeUpdates();
+
 private:
     AXIsolatedTree(AXObjectCache&);
     static void storeTree(AXObjectCache&, const Ref<AXIsolatedTree>&);
@@ -435,6 +458,11 @@ private:

     Lock m_changeLogLock;
     AXTextMarkerRange m_selectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock);
+
+    // Types of node updates (queued)
+    ListHashSet<AXID> m_needsUpdateChildren;
+    ListHashSet<AXID> m_needsUpdateNode;
+    HashMap<AXID, Vector<AXPropertyName>> m_needsPropertyUpdates;
 };

 inline AXObjectCache* AXIsolatedTree::axObjectCache() const
diff --git a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
index 78d52e57e9ae..b511c7f00d35 100644
--- a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
+++ b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
@@ -325,6 +325,12 @@ void AXObjectCache::postPlatformNotification(AXCoreObject* object, AXNotificatio
         return;
     }

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    // Process all pending node changes before sending notification.
+    if (auto tree = getOrCreateIsolatedTree())

AG: getOrCreateIsolatedTree -> treeByPageID

+        tree->processQueuedNodeUpdates();
+#endif
+
     bool skipSystemNotification = false;
     // Some notifications are unique to Safari and do not have NSAccessibility equivalents.
     NSString *macNotification;
@@ -463,6 +469,11 @@ void AXObjectCache::postPlatformAnnouncementNotification(const String& message)
 {
     ASSERT(isMainThread());

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    if (auto tree = getOrCreateIsolatedTree())

AG: same here.

+        tree->processQueuedNodeUpdates();
+#endif
+
     NSDictionary *userInfo = @{ NSAccessibilityPriorityKey: @(NSAccessibilityPriorityHigh),
         NSAccessibilityAnnouncementKey: message,
     };
@@ -504,6 +515,11 @@ void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject*
         return;
     RefPtr protectedObject = object;

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    if (auto tree = getOrCreateIsolatedTree())

AG: same here.

+        tree->processQueuedNodeUpdates();
+#endif
+
     auto userInfo = adoptNS([[NSMutableDictionary alloc] initWithCapacity:5]);
     if (m_isSynchronizingSelection)
         [userInfo setObject:@YES forKey:NSAccessibilityTextStateSyncKey];
@@ -624,6 +640,11 @@ void AXObjectCache::postTextReplacementPlatformNotification(AccessibilityObject*
         return;
     RefPtr protectedObject = object;

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    if (auto tree = getOrCreateIsolatedTree())

AG: same here.

+        tree->processQueuedNodeUpdates();
+#endif
+
     auto changes = adoptNS([[NSMutableArray alloc] initWithCapacity:2]);
     if (NSDictionary *change = textReplacementChangeDictionary(*object, deletionType, deletedText, position))
         [changes addObject:change];
@@ -643,6 +664,11 @@ void AXObjectCache::postTextReplacementPlatformNotificationForTextControl(Access
         return;
     RefPtr protectedObject = object;

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    if (auto tree = getOrCreateIsolatedTree())

AG: same here.

+        tree->processQueuedNodeUpdates();
+#endif
+
     auto changes = adoptNS([[NSMutableArray alloc] initWithCapacity:2]);
     if (NSDictionary *change = textReplacementChangeDictionary(*object, AXTextEditTypeDelete, deletedText, textControl))
         [changes addObject:change];

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #6)
> (In reply to Joshua Hoffman from comment #4)
> > Created attachment 468900 [details]
> > Patch
> 
> diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> index 890a0ec4857c..44a394534ca2 100644
> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> @@ -532,7 +532,7 @@ void
> AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject&
>          return;
> 
>      Accessibility::enumerateDescendants<AXCoreObject>(axObject, true,
> [&properties, this] (auto& descendant) {
> -        updateNodeProperties(descendant, properties);
> +        queueNodeUpdate(descendant, { properties });
>      });
>  }
> 
> @@ -733,7 +733,7 @@ void
> AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj
>      bool updateTableAncestorColumns = is<AccessibilityTableRow>(axObject);
>      for (auto* ancestor = axObject.parentObject(); ancestor; ancestor =
> ancestor->parentObject()) {
>          if (ancestor->isTree()) {
> -            updateNodeProperty(*ancestor, AXPropertyName::ARIATreeRows);
> +            queueNodeUpdate(*ancestor, { AXPropertyName::ARIATreeRows });
>              if (!updateTableAncestorColumns)
>                  break;
>          }
> @@ -990,7 +990,7 @@ void AXIsolatedTree::labelCreated(AccessibilityObject&
> axObject)
>  {
>      ASSERT(axObject.isLabel());
>      if (RefPtr correspondingControl = axObject.isLabel() ?
> axObject.correspondingControlForLabelElement() : nullptr)
> -        updateNode(*correspondingControl);
> +        queueNodeUpdate(*correspondingControl, { { }, true, false });
>  }
> 
>  void AXIsolatedTree::updateLoadingProgress(double newProgressValue)
> @@ -1238,6 +1238,67 @@ AXTreePtr findAXTree(Function<bool(AXTreePtr)>&&
> match)
>  #endif
>  }
> 
> +void AXIsolatedTree::queueNodeUpdate(AXCoreObject& object,
> NodeUpdateOptions options)
> +{
> +    ASSERT(isMainThread());
> +
> +    // If we are updating a property, but our node is in m_needsUpdateNode,
> don't queue anything.
> +    if (options.properties.size()) {
> +        if (m_needsUpdateNode.contains(object.objectID()))
> +            return;
> 
> AG: what about if the caller passed { someProperties, true, false }, i.e.,
> requested a node update and also passed some properties.

The callers shouldn't do this (since a node update completely invalidates the need to update properties), but this is handled/de-duped correctly with this code as-is. 

> +
> +    NodeUpdateOptions(Vector<AXPropertyName> propertyNames)
> +        : properties(propertyNames)
> +    { }
> 
> AG: same here. Does the compileer require to explicitly define these
> "default" constructors?

When trying to get this to build properly I had to define all three constructors. I notice we define the default in the CharacterOffset struct as well. Have you encountered this before?

> AG: does the order of the properties matter? or we could use a set instead
> of a vector?

We may be able to get away with a set here—I will give that a shot.

[Joshua Hoffman]

Created attachment 468908 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #8)
> Created attachment 468908 [details]
> Patch

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 890a0ec4857c..ed8da3770da0 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -531,8 +531,12 @@ void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject&
     if (isUpdatingSubtree())
         return;

-    Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&properties, this] (auto& descendant) {
-        updateNodeProperties(descendant, properties);
+    AXPropertyNameSet propertySet;
+    for (const auto& property : properties)
+        propertySet.add(property);
+
+    Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&propertySet, this] (auto& descendant) {
+        queueNodeUpdate(descendant, { propertySet });
     });
 }

@@ -733,7 +737,7 @@ void AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj
     bool updateTableAncestorColumns = is<AccessibilityTableRow>(axObject);
     for (auto* ancestor = axObject.parentObject(); ancestor; ancestor = ancestor->parentObject()) {
         if (ancestor->isTree()) {
-            updateNodeProperty(*ancestor, AXPropertyName::ARIATreeRows);
+            queueNodeUpdate(*ancestor, { AXPropertyName::ARIATreeRows });
             if (!updateTableAncestorColumns)
                 break;
         }
@@ -990,7 +994,7 @@ void AXIsolatedTree::labelCreated(AccessibilityObject& axObject)
 {
     ASSERT(axObject.isLabel());
     if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr)
-        updateNode(*correspondingControl);
+        queueNodeUpdate(*correspondingControl, { { }, true, false });
 }

 void AXIsolatedTree::updateLoadingProgress(double newProgressValue)
@@ -1238,6 +1242,67 @@ AXTreePtr findAXTree(Function<bool(AXTreePtr)>&& match)
 #endif
 }

+void AXIsolatedTree::queueNodeUpdate(AXCoreObject& object, const NodeUpdateOptions& options)
+{
+    ASSERT(isMainThread());
+
+    if (!options.shouldUpdateNode && options.properties.size()) {
+        // If we are updating a property, but our node is already in m_needsUpdateNode, don't queue anything.
+        if (m_needsUpdateNode.contains(object.objectID()))
+            return;
+
+        Vector<AXPropertyName> properties;
+        if (m_needsPropertyUpdates.contains(object.objectID()))
+            properties = m_needsPropertyUpdates.get(object.objectID());
+
+        for (const auto& property : options.properties)
+            properties.appendIfNotContains(property);
+
+        m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties));
+    }
+
+    if (options.shouldUpdateChildren && !m_needsUpdateChildren.contains(object.objectID()))
+        m_needsUpdateChildren.add(object.objectID());
+
+    if (options.shouldUpdateNode && !m_needsUpdateNode.contains(object.objectID()))
+        m_needsUpdateNode.add(object.objectID());

AG: Don't need to do && !m_needsUpdateNode.contains(object.objectID()) since the add method handles correctly whether the entry exists or not.

+
+    if (auto* cache = axObjectCache())
+        cache->startSnapshotTimer();
+}
+
+void AXIsolatedTree::processQueuedNodeUpdates()
+{
+    ASSERT(isMainThread());
+
+    auto* cache = axObjectCache();
+    if (!cache)
+        return;
+
+    for (AXID nodeID : m_needsUpdateChildren) {
+        if (auto* axObject = cache->objectForID(nodeID))
+            updateChildren(*axObject, ResolveNodeChanges::No);
+    }
+    m_needsUpdateChildren.clear();
+
+    for (AXID nodeID : m_needsUpdateNode) {

AG: we use objectID not nodeID, although in other naming we mix them. My preference is to reserve node naming for DOM Nodes and use object for AX objects.

+        if (!m_unresolvedPendingAppends.contains(nodeID))
+            m_unresolvedPendingAppends.set(nodeID, AttachWrapper::OnMainThread);

AG: Can we attach the wrapper on the main thread? I think that if we are replacing an existing object, we cannot.

+    }
+    m_needsUpdateNode.clear();
+
+    for (const auto& propertyUpdate : m_needsPropertyUpdates) {
+        if (m_unresolvedPendingAppends.contains(propertyUpdate.key))
+            break;
+
+        if (auto* axObject = cache->objectForID(propertyUpdate.key))
+            updateNodeProperties(*axObject, propertyUpdate.value);
+    }
+    m_needsPropertyUpdates.clear();
+
+    queueRemovalsAndUnresolvedChanges({ });
+}
+
 } // namespace WebCore

 #endif // ENABLE(ACCESSIBILITY_ISOLATED_TREE)
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index c9609b8c8b1d..176f5d589096 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -246,6 +246,8 @@ enum class AXPropertyName : uint16_t {
     VisibleRows,
 };

+using AXPropertyNameSet = HashSet<AXPropertyName, IntHash<AXPropertyName>, WTF::StrongEnumHashTraits<AXPropertyName>>;
+
 // If this type is modified, the switchOn statment in AXIsolatedObject::setProperty must be updated as well.
 using AXPropertyValueVariant = std::variant<std::nullptr_t, AXID, String, bool, int, unsigned, double, float, uint64_t, AccessibilityButtonState, Color, URL, LayoutRect, FloatPoint, FloatRect, IntPoint, IntRect, std::pair<unsigned, unsigned>, Vector<AccessibilityText>, Vector<AXID>, Vector<std::pair<AXID, AXID>>, Vector<String>, Path, OptionSet<AXAncestorFlag>, InsideLink, Vector<Vector<AXID>>, CharacterRange, std::pair<AXID, CharacterRange>
 #if PLATFORM(COCOA)
@@ -262,6 +264,26 @@ struct AXPropertyChange {
     AXPropertyMap properties; // Changed properties.
 };

+struct NodeUpdateOptions {
+    AXPropertyNameSet properties;
+    bool shouldUpdateNode { false };
+    bool shouldUpdateChildren { false };
+
+    NodeUpdateOptions(const AXPropertyNameSet& propertyNames, bool shouldUpdateNode, bool shouldUpdateChildren)
+        : properties(propertyNames)
+        , shouldUpdateNode(shouldUpdateNode)
+        , shouldUpdateChildren(shouldUpdateChildren)
+    { }
+
+    NodeUpdateOptions(const AXPropertyNameSet& propertyNames)
+        : properties(propertyNames)
+    { }
+
+    NodeUpdateOptions(AXPropertyName propertyName)
+        : properties({ propertyName })
+    { }
+};
+
 DECLARE_ALLOCATOR_WITH_HEAP_IDENTIFIER(AXIsolatedTree);
 class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AXIsolatedTree>
     , public AXTreeStore<AXIsolatedTree> {
@@ -336,6 +358,9 @@ public:
     AXTextMarkerRange selectedTextMarkerRange();
     void setSelectedTextMarkerRange(AXTextMarkerRange&&);

+    void queueNodeUpdate(AXCoreObject&, const NodeUpdateOptions&);
+    void processQueuedNodeUpdates();
+
 private:
     AXIsolatedTree(AXObjectCache&);
     static void storeTree(AXObjectCache&, const Ref<AXIsolatedTree>&);
@@ -435,6 +460,11 @@ private:

     Lock m_changeLogLock;
     AXTextMarkerRange m_selectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock);
+
+    // Types of node updates (queued)
+    ListHashSet<AXID> m_needsUpdateChildren;
+    ListHashSet<AXID> m_needsUpdateNode;
+    HashMap<AXID, Vector<AXPropertyName>> m_needsPropertyUpdates;

AG: can't we use an AXPropertyNameSet instead of Vector<AXPropertyName>?

 };

 inline AXObjectCache* AXIsolatedTree::axObjectCache() const

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Joshua Hoffman from comment #8)
> +    if (options.shouldUpdateChildren &&
> !m_needsUpdateChildren.contains(object.objectID()))
> +        m_needsUpdateChildren.add(object.objectID());
> +
> +    if (options.shouldUpdateNode &&
> !m_needsUpdateNode.contains(object.objectID()))
> +        m_needsUpdateNode.add(object.objectID());
> 
> AG: Don't need to do && !m_needsUpdateNode.contains(object.objectID()) since
> the add method handles correctly whether the entry exists or not.

Updated this and the condition for updateChildren.

> +    for (AXID nodeID : m_needsUpdateNode) {
> 
> AG: we use objectID not nodeID, although in other naming we mix them. My
> preference is to reserve node naming for DOM Nodes and use object for AX
> objects.

Updated.

> 
> +        if (!m_unresolvedPendingAppends.contains(nodeID))
> +            m_unresolvedPendingAppends.set(nodeID,
> AttachWrapper::OnMainThread);
> 
> AG: Can we attach the wrapper on the main thread? I think that if we are
> replacing an existing object, we cannot.

Good catch! Yeah I will swap that to OnAXThread. 

> +    // Types of node updates (queued)
> +    ListHashSet<AXID> m_needsUpdateChildren;
> +    ListHashSet<AXID> m_needsUpdateNode;
> +    HashMap<AXID, Vector<AXPropertyName>> m_needsPropertyUpdates;
> 
> AG: can't we use an AXPropertyNameSet instead of Vector<AXPropertyName>?

Yes, definitely. I just need to modify the parameter type of AXIsolatedTree::updateNodeProperties but that will be much cleaner.

[Joshua Hoffman]

Created attachment 468909 [details]
Patch

[Joshua Hoffman]

Created attachment 468913 [details]
Patch

[Tyler Wilcock]

Comment on attachment 468913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468913&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:237
> +static const Seconds accessibilityUpdateTreeSnapshotTimerInterval { 100_ms };

"accessibility" feels unnecessary considering this is a static symbol inside an accessibility class.

> Source/WebCore/accessibility/AXObjectCache.cpp:4330
> +void AXObjectCache::startSnapshotTimer()

I think it's least confusing when these methods are named "start" and then the name of the timer. So "startUpdateTreeSnapshotTimer".

> Source/WebCore/accessibility/AXObjectCache.cpp:4936
> +    if (!accessibilityEnabled())
> +        return;

Is this necessary? What scenario are you trying to catch?

> Source/WebCore/accessibility/AXObjectCache.cpp:4938
> +    m_updateTreeSnapshotTimer.stop();

We probably want to unconditionally stop this timer.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:998
> +        queueNodeUpdate(*correspondingControl, { { }, true, false });

We've lost a lot of information at the callsite here. Before, we knew this object was going to get `updateNode` (fully recreated). Now, it's just curly braces and some un-named bools, which makes things less clear. Maybe we could make a static method in NodeUpdateOptions that constructs this for us with a more readable name? e.g. NodeUpdateOptions::nodeUpdate()? Same for the other updateNode callsites, and updateChildren callsites.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1279
> +    auto* cache = axObjectCache();

This should be a WeakPtr rather than a raw pointer.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1284
> +        if (auto* axObject = cache->objectForID(nodeID))

We should probably protect this object with a RefPtr instead of using a raw pointer. updateChildren does a lot of complicated logic that could cause this object to be destroyed (unless it's somehow protected elsewhere that you know about?)

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1291
> +        if (!m_unresolvedPendingAppends.contains(objectID))
> +            m_unresolvedPendingAppends.set(objectID, AttachWrapper::OnAXThread);

I think using ensure will require one less hash operation:

m_unresolvedPendingAppends.ensure(axObject.objectID(), [] {
    return AttachWrapper::OnAXThread;
});

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1299
> +        if (auto* axObject = cache->objectForID(propertyUpdate.key))

Probably want to protect this with a RefPtr too.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:464
> +    // Types of node updates (queued)

Can we make this comment more descriptive? e.g. "Queued updates used for building a new tree snapshot."

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:332
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> +    // Process all pending node changes before sending notification.
> +    if (auto tree = AXIsolatedTree::treeForPageID(m_pageID))
> +        tree->processQueuedNodeUpdates();
> +#endif

Can this extracted into a separate method since we repeat it a few times?

[Joshua Hoffman]

Comment on attachment 468913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468913&action=review

>> Source/WebCore/accessibility/AXObjectCache.cpp:237
>> +static const Seconds accessibilityUpdateTreeSnapshotTimerInterval { 100_ms };
> 
> "accessibility" feels unnecessary considering this is a static symbol inside an accessibility class.

Gotcha, will remove that.

>> Source/WebCore/accessibility/AXObjectCache.cpp:4330
>> +void AXObjectCache::startSnapshotTimer()
> 
> I think it's least confusing when these methods are named "start" and then the name of the timer. So "startUpdateTreeSnapshotTimer".

Sounds good!

>> Source/WebCore/accessibility/AXObjectCache.cpp:4936
>> +        return;
> 
> Is this necessary? What scenario are you trying to catch?

We had debated this when adding selectedTextRangeTimerFired too, and I think we decided to leave this check in in case a user turns off an AT. But, I think the timer is so short that it is safe to remove.

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:998
>> +        queueNodeUpdate(*correspondingControl, { { }, true, false });
> 
> We've lost a lot of information at the callsite here. Before, we knew this object was going to get `updateNode` (fully recreated). Now, it's just curly braces and some un-named bools, which makes things less clear. Maybe we could make a static method in NodeUpdateOptions that constructs this for us with a more readable name? e.g. NodeUpdateOptions::nodeUpdate()? Same for the other updateNode callsites, and updateChildren callsites.

I like that idea! Definitely makes things more readable.

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1284
>> +        if (auto* axObject = cache->objectForID(nodeID))
> 
> We should probably protect this object with a RefPtr instead of using a raw pointer. updateChildren does a lot of complicated logic that could cause this object to be destroyed (unless it's somehow protected elsewhere that you know about?)

No, I don't think it is protected anywhere else, so we should make this a RefPtr here.

>> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:332
>> +#endif
> 
> Can this extracted into a separate method since we repeat it a few times?

Yep!

[Joshua Hoffman]

Created attachment 468929 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #15)
> Created attachment 468929 [details]
> Patch

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 890a0ec4857c..bb6c9aa21fa0 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -523,7 +523,7 @@ void AXIsolatedTree::updateNode(AccessibilityObject& axObject)
     }
 }

-void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject& axObject, const Vector<AXPropertyName>& properties)
+void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject& axObject, const AXPropertyNameSet& properties)
 {
     AXTRACE("AXIsolatedTree::updatePropertiesForSelfAndDescendants"_s);
     ASSERT(isMainThread());
@@ -531,15 +531,19 @@ void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject&
     if (isUpdatingSubtree())
         return;

-    Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&properties, this] (auto& descendant) {
-        updateNodeProperties(descendant, properties);
+    AXPropertyNameSet propertySet;
+    for (const auto& property : properties)
+        propertySet.add(property);
+
+    Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&propertySet, this] (auto& descendant) {
+        queueNodeUpdate(descendant, { propertySet });
     });
 }

-void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<AXPropertyName>& properties)
+void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const AXPropertyNameSet& properties)
 {
     AXTRACE("AXIsolatedTree::updateNodeProperties"_s);
-    AXLOG(makeString("Updating properties ", properties, " for objectID ", axObject.objectID().loggingString()));
+    AXLOG(makeString("Updating properties for objectID ", axObject.objectID().loggingString(), ": "));

AG: what is the ": " for?
AG: if you want to continue logging the PropertyNameSet, you probably need to write an operator << for it. But not necessary in this patch. You can get rid of this AXLOG all together or log the objectID in the AXTRACE.

     ASSERT(isMainThread());

     if (isUpdatingSubtree())
@@ -547,6 +551,7 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A

     AXPropertyMap propertyMap;
     for (const auto& property : properties) {
+        AXLOG(makeString("Property: ", property));

AG: ah I see that you are logging the property down here. It would be good to write the operator << and the AXLogger::log for it, but again not needed in this patch.

         switch (property) {
         case AXPropertyName::AccessKey:
             propertyMap.set(AXPropertyName::AccessKey, axObject.accessKey().isolatedCopy());
@@ -733,7 +738,7 @@ void AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj
     bool updateTableAncestorColumns = is<AccessibilityTableRow>(axObject);
     for (auto* ancestor = axObject.parentObject(); ancestor; ancestor = ancestor->parentObject()) {
         if (ancestor->isTree()) {
-            updateNodeProperty(*ancestor, AXPropertyName::ARIATreeRows);
+            queueNodeUpdate(*ancestor, { AXPropertyName::ARIATreeRows });
             if (!updateTableAncestorColumns)
                 break;
         }
@@ -990,7 +995,7 @@ void AXIsolatedTree::labelCreated(AccessibilityObject& axObject)
 {
     ASSERT(axObject.isLabel());
     if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr)
-        updateNode(*correspondingControl);
+        queueNodeUpdate(*correspondingControl, NodeUpdateOptions::nodeUpdate());
 }

 void AXIsolatedTree::updateLoadingProgress(double newProgressValue)
@@ -1238,6 +1243,70 @@ AXTreePtr findAXTree(Function<bool(AXTreePtr)>&& match)
 #endif
 }

+void AXIsolatedTree::queueNodeUpdate(AXCoreObject& object, const NodeUpdateOptions& options)
+{
+    ASSERT(isMainThread());
+
+    if (!options.shouldUpdateNode && options.properties.size()) {
+        // If we are updating a property, but our node is already in m_needsUpdateNode, don't queue anything.
+        if (m_needsUpdateNode.contains(object.objectID()))
+            return;
+
+        AXPropertyNameSet properties;
+        if (m_needsPropertyUpdates.contains(object.objectID()))
+            properties = m_needsPropertyUpdates.get(object.objectID());
+
+        for (const auto& property : options.properties)
+            properties.add(property);
+
+        m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties));
+    }

AG: I think you can do just something like:

        if (m_needsPropertyUpdates.contains(object.objectID()))
            m_needsPropertyUpdates.get(object.objectID()).value.formUnion(options.properties);
        else
            m_needsPropertyUpdates.set(object.objectID(), options.properties);


+
+    if (options.shouldUpdateChildren)
+        m_needsUpdateChildren.add(object.objectID());
+
+    if (options.shouldUpdateNode)
+        m_needsUpdateNode.add(object.objectID());
+
+    if (auto* cache = axObjectCache())
+        cache->startUpdateTreeSnapshotTimer();
+}
+
+void AXIsolatedTree::processQueuedNodeUpdates()
+{
+    ASSERT(isMainThread());
+
+    WeakPtr cache = axObjectCache();
+    if (!cache)
+        return;
+
+    for (AXID nodeID : m_needsUpdateChildren) {
+        if (RefPtr axObject = cache->objectForID(nodeID))

AG: shouldn't we null check cache before using it? It may go away at any time I think.

+            updateChildren(*axObject, ResolveNodeChanges::No);
+    }
+    m_needsUpdateChildren.clear();
+
+    for (AXID objectID : m_needsUpdateNode) {
+        if (!m_unresolvedPendingAppends.contains(objectID)) {
+            m_unresolvedPendingAppends.ensure(objectID, [] {
+                return AttachWrapper::OnAXThread;
+            });
+        }
+    }
+    m_needsUpdateNode.clear();
+
+    for (const auto& propertyUpdate : m_needsPropertyUpdates) {
+        if (m_unresolvedPendingAppends.contains(propertyUpdate.key))
+            break;

AG: shouldn't be continue to the next object?

+
+        if (RefPtr axObject = cache->objectForID(propertyUpdate.key))

AG: shouldn't we null check cache?

+            updateNodeProperties(*axObject, propertyUpdate.value);
+    }
+    m_needsPropertyUpdates.clear();
+
+    queueRemovalsAndUnresolvedChanges({ });
+}
+
 } // namespace WebCore

 #endif // ENABLE(ACCESSIBILITY_ISOLATED_TREE)
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index c9609b8c8b1d..88d36893ae0b 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -246,6 +246,8 @@ enum class AXPropertyName : uint16_t {
     VisibleRows,
 };

+using AXPropertyNameSet = HashSet<AXPropertyName, IntHash<AXPropertyName>, WTF::StrongEnumHashTraits<AXPropertyName>>;
+
 // If this type is modified, the switchOn statment in AXIsolatedObject::setProperty must be updated as well.
 using AXPropertyValueVariant = std::variant<std::nullptr_t, AXID, String, bool, int, unsigned, double, float, uint64_t, AccessibilityButtonState, Color, URL, LayoutRect, FloatPoint, FloatRect, IntPoint, IntRect, std::pair<unsigned, unsigned>, Vector<AccessibilityText>, Vector<AXID>, Vector<std::pair<AXID, AXID>>, Vector<String>, Path, OptionSet<AXAncestorFlag>, InsideLink, Vector<Vector<AXID>>, CharacterRange, std::pair<AXID, CharacterRange>
 #if PLATFORM(COCOA)
@@ -262,6 +264,36 @@ struct AXPropertyChange {
     AXPropertyMap properties; // Changed properties.
 };

+struct NodeUpdateOptions {
+    AXPropertyNameSet properties;
+    bool shouldUpdateNode { false };
+    bool shouldUpdateChildren { false };
+
+    NodeUpdateOptions(const AXPropertyNameSet& propertyNames, bool shouldUpdateNode, bool shouldUpdateChildren)
+        : properties(propertyNames)
+        , shouldUpdateNode(shouldUpdateNode)
+        , shouldUpdateChildren(shouldUpdateChildren)
+    { }
+
+    NodeUpdateOptions(const AXPropertyNameSet& propertyNames)
+        : properties(propertyNames)
+    { }
+
+    NodeUpdateOptions(AXPropertyName propertyName)
+        : properties({ propertyName })
+    { }
+
+    static NodeUpdateOptions nodeUpdate()
+    {
+        return { { }, true, false };
+    }
+
+    static NodeUpdateOptions childrenUpdate()
+    {
+        return { { }, false, true };
+    }

AG: can these two be constexpr ?

+};
+
 DECLARE_ALLOCATOR_WITH_HEAP_IDENTIFIER(AXIsolatedTree);
 class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AXIsolatedTree>
     , public AXTreeStore<AXIsolatedTree> {
@@ -298,9 +330,9 @@ public:
     void updateChildren(AccessibilityObject&, ResolveNodeChanges = ResolveNodeChanges::Yes);
     void updateChildrenForObjects(const ListHashSet<RefPtr<AccessibilityObject>>&);
     void updateNodeProperty(AXCoreObject& object, AXPropertyName property) { updateNodeProperties(object, { property }); };
-    void updateNodeProperties(AXCoreObject&, const Vector<AXPropertyName>&);
+    void updateNodeProperties(AXCoreObject&, const AXPropertyNameSet&);
     void updateNodeAndDependentProperties(AccessibilityObject&);
-    void updatePropertiesForSelfAndDescendants(AccessibilityObject&, const Vector<AXPropertyName>&);
+    void updatePropertiesForSelfAndDescendants(AccessibilityObject&, const AXPropertyNameSet&);
     void updateFrame(AXID, IntRect&&);
     void overrideNodeProperties(AXID, AXPropertyMap&&);

@@ -336,6 +368,9 @@ public:
     AXTextMarkerRange selectedTextMarkerRange();
     void setSelectedTextMarkerRange(AXTextMarkerRange&&);

+    void queueNodeUpdate(AXCoreObject&, const NodeUpdateOptions&);
+    void processQueuedNodeUpdates();
+
 private:
     AXIsolatedTree(AXObjectCache&);
     static void storeTree(AXObjectCache&, const Ref<AXIsolatedTree>&);
@@ -435,6 +470,11 @@ private:

     Lock m_changeLogLock;
     AXTextMarkerRange m_selectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock);
+
+    // Queued node updates used for building a new tree snapshot.
+    ListHashSet<AXID> m_needsUpdateChildren;
+    ListHashSet<AXID> m_needsUpdateNode;
+    HashMap<AXID, AXPropertyNameSet> m_needsPropertyUpdates;
 };

 inline AXObjectCache* AXIsolatedTree::axObjectCache() const

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #16)
> (In reply to Joshua Hoffman from comment #15)
> > Created attachment 468929 [details]
> > Patch
> 
>      AXPropertyMap propertyMap;
>      for (const auto& property : properties) {
> +        AXLOG(makeString("Property: ", property));
> 
> AG: ah I see that you are logging the property down here. It would be good
> to write the operator << and the AXLogger::log for it, but again not needed
> in this patch.

Sounds good! This is definitely something I'd love to clean up in the future.

> +    if (!options.shouldUpdateNode && options.properties.size()) {
> +        // If we are updating a property, but our node is already in
> m_needsUpdateNode, don't queue anything.
> +        if (m_needsUpdateNode.contains(object.objectID()))
> +            return;
> +
> +        AXPropertyNameSet properties;
> +        if (m_needsPropertyUpdates.contains(object.objectID()))
> +            properties = m_needsPropertyUpdates.get(object.objectID());
> +
> +        for (const auto& property : options.properties)
> +            properties.add(property);
> +
> +        m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties));
> +    }
> 
> AG: I think you can do just something like:

I like that idea, that should work.

> +
> +void AXIsolatedTree::processQueuedNodeUpdates()
> +{
> +    ASSERT(isMainThread());
> +
> +    WeakPtr cache = axObjectCache();
> +    if (!cache)
> +        return;
> +
> +    for (AXID nodeID : m_needsUpdateChildren) {
> +        if (RefPtr axObject = cache->objectForID(nodeID))
> 
> AG: shouldn't we null check cache before using it? It may go away at any
> time I think.

Is the null check at the top not sufficient in this case? There are other places we null check the cache once and then use it unconditionally.
> 
> +            updateChildren(*axObject, ResolveNodeChanges::No);
> +    }
> +    m_needsUpdateChildren.clear();
> +
> +    for (AXID objectID : m_needsUpdateNode) {
> +        if (!m_unresolvedPendingAppends.contains(objectID)) {
> +            m_unresolvedPendingAppends.ensure(objectID, [] {
> +                return AttachWrapper::OnAXThread;
> +            });
> +        }
> +    }
> +    m_needsUpdateNode.clear();
> +
> +    for (const auto& propertyUpdate : m_needsPropertyUpdates) {
> +        if (m_unresolvedPendingAppends.contains(propertyUpdate.key))
> +            break;
> 
> AG: shouldn't be continue to the next object?

Ah yes—good catch!

> +    static NodeUpdateOptions nodeUpdate()
> +    {
> +        return { { }, true, false };
> +    }
> +
> +    static NodeUpdateOptions childrenUpdate()
> +    {
> +        return { { }, false, true };
> +    }
> 
> AG: can these two be constexpr ?

Yes I'll update this.

[Joshua Hoffman]

(In reply to Joshua Hoffman from comment #17)
> (In reply to Andres Gonzalez from comment #16)
> > (In reply to Joshua Hoffman from comment #15)
> 
> > +    static NodeUpdateOptions nodeUpdate()
> > +    {
> > +        return { { }, true, false };
> > +    }
> > +
> > +    static NodeUpdateOptions childrenUpdate()
> > +    {
> > +        return { { }, false, true };
> > +    }
> > 
> > AG: can these two be constexpr ?
> 
> Yes I'll update this.

Tried this out and these actually cannot be constexpr, since the properties field (AXPropertyNameSet) in this struct is not a literal.

[Joshua Hoffman]

Created attachment 468947 [details]
Patch

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #16)
> (In reply to Joshua Hoffman from comment #15)
> > Created attachment 468929 [details]
> > Patch
> +
> +        AXPropertyNameSet properties;
> +        if (m_needsPropertyUpdates.contains(object.objectID()))
> +            properties = m_needsPropertyUpdates.get(object.objectID());
> +
> +        for (const auto& property : options.properties)
> +            properties.add(property);
> +
> +        m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties));
> +    }
> 
> AG: I think you can do just something like:
> 
>         if (m_needsPropertyUpdates.contains(object.objectID()))
>            
> m_needsPropertyUpdates.get(object.objectID()).value.formUnion(options.
> properties);
>         else
>             m_needsPropertyUpdates.set(object.objectID(),
> options.properties);
> 

I've been thinking about this a bit and I think we can make it even simpler without using any conditions:

auto existingProperties = m_needsPropertyUpdates.get(object.objectID());
m_needsPropertyUpdates.set(object.objectID(), existingProperties.unionWith(options.properties));

ExistingProperties will be empty if it doesn't exist in the map, so we can union with options.properties regardless.

[Joshua Hoffman]

Created attachment 468967 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #20)
> (In reply to Andres Gonzalez from comment #16)
> > (In reply to Joshua Hoffman from comment #15)
> > > Created attachment 468929 [details]
> > > Patch
> > +
> > +        AXPropertyNameSet properties;
> > +        if (m_needsPropertyUpdates.contains(object.objectID()))
> > +            properties = m_needsPropertyUpdates.get(object.objectID());
> > +
> > +        for (const auto& property : options.properties)
> > +            properties.add(property);
> > +
> > +        m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties));
> > +    }
> > 
> > AG: I think you can do just something like:
> > 
> >         if (m_needsPropertyUpdates.contains(object.objectID()))
> >            
> > m_needsPropertyUpdates.get(object.objectID()).value.formUnion(options.
> > properties);
> >         else
> >             m_needsPropertyUpdates.set(object.objectID(),
> > options.properties);
> > 
> 
> I've been thinking about this a bit and I think we can make it even simpler
> without using any conditions:
> 
> auto existingProperties = m_needsPropertyUpdates.get(object.objectID());
> m_needsPropertyUpdates.set(object.objectID(),
> existingProperties.unionWith(options.properties));
> 
> ExistingProperties will be empty if it doesn't exist in the map, so we can
> union with options.properties regardless.

Yes, something like that, although unionWith creates a new set while formUnion operates in place.

[Joshua Hoffman]

Created attachment 468978 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #17)
> (In reply to Andres Gonzalez from comment #16)
> > (In reply to Joshua Hoffman from comment #15)
> > > Created attachment 468929 [details]
> > > Patch
> > 
> >      AXPropertyMap propertyMap;
> >      for (const auto& property : properties) {
> > +        AXLOG(makeString("Property: ", property));
> > 
> > AG: ah I see that you are logging the property down here. It would be good
> > to write the operator << and the AXLogger::log for it, but again not needed
> > in this patch.
> 
> Sounds good! This is definitely something I'd love to clean up in the future.
> 
> > +    if (!options.shouldUpdateNode && options.properties.size()) {
> > +        // If we are updating a property, but our node is already in
> > m_needsUpdateNode, don't queue anything.
> > +        if (m_needsUpdateNode.contains(object.objectID()))
> > +            return;
> > +
> > +        AXPropertyNameSet properties;
> > +        if (m_needsPropertyUpdates.contains(object.objectID()))
> > +            properties = m_needsPropertyUpdates.get(object.objectID());
> > +
> > +        for (const auto& property : options.properties)
> > +            properties.add(property);
> > +
> > +        m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties));
> > +    }
> > 
> > AG: I think you can do just something like:
> 
> I like that idea, that should work.
> 
> > +
> > +void AXIsolatedTree::processQueuedNodeUpdates()
> > +{
> > +    ASSERT(isMainThread());
> > +
> > +    WeakPtr cache = axObjectCache();
> > +    if (!cache)
> > +        return;
> > +
> > +    for (AXID nodeID : m_needsUpdateChildren) {
> > +        if (RefPtr axObject = cache->objectForID(nodeID))
> > 
> > AG: shouldn't we null check cache before using it? It may go away at any
> > time I think.
> 
> Is the null check at the top not sufficient in this case? There are other
> places we null check the cache once and then use it unconditionally.
> > 

AG: maybe we can get away with no checking that the cache is still alive as we iterate here, but it would be safer and not costly to check inside the loop. updateChildren is a complex operation that can trigger indirectly the destruction of the cache. The rest LGTM. Thanks!

> > +            updateChildren(*axObject, ResolveNodeChanges::No);
> > +    }
> > +    m_needsUpdateChildren.clear();
...

[Tyler Wilcock]

Comment on attachment 468978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468978&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1251
> +        // If we are updating a property, but our node is already in m_needsUpdateNode, don't queue anything.

It would be nice if this comment explained "why" rather than "what" (since the code tells us "what"). For example (re-word as desired, just an idea):

// If we're going to recompute all properties for the node, indicated by presence in m_needsUpdateNode, don't bother queueing this individual property update.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1257
> +        auto addResult = m_needsPropertyUpdates.add(object.objectID(), options.properties);
> +
> +        if (!addResult.isNewEntry)

nit: the newline between these statements feels unnecessary.

[Joshua Hoffman]

Created attachment 468983 [details]
Patch

[Joshua Hoffman]

Thanks Tyler and Andres for your comments! They are all addressed in the latest update.

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #26)
> Created attachment 468983 [details]
> Patch

+    WeakPtr cache = axObjectCache();
+    if (!cache)
+        return;
+
+    for (AXID nodeID : m_needsUpdateChildren) {
+        if (!cache)
+            break;

AG: return ? Or we need to exec the rest of the function ?

+
+        if (RefPtr axObject = cache->objectForID(nodeID))
+            updateChildren(*axObject, ResolveNodeChanges::No);
+    }
+    m_needsUpdateChildren.clear();
+
+    for (AXID objectID : m_needsUpdateNode) {
+        if (!m_unresolvedPendingAppends.contains(objectID)) {
+            m_unresolvedPendingAppends.ensure(objectID, [] {
+                return AttachWrapper::OnAXThread;
+            });
+        }
+    }
+    m_needsUpdateNode.clear();
+
+    for (const auto& propertyUpdate : m_needsPropertyUpdates) {
+        if (m_unresolvedPendingAppends.contains(propertyUpdate.key))
+            continue;
+
+        if (!cache)
+            break;

AG: same here.

+
+        if (RefPtr axObject = cache->objectForID(propertyUpdate.key))
+            updateNodeProperties(*axObject, propertyUpdate.value);
+    }
+    m_needsPropertyUpdates.clear();
+
+    queueRemovalsAndUnresolvedChanges({ });
+}

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #28)
> (In reply to Joshua Hoffman from comment #26)
> > Created attachment 468983 [details]
> > Patch
> 
> +    WeakPtr cache = axObjectCache();
> +    if (!cache)
> +        return;
> +
> +    for (AXID nodeID : m_needsUpdateChildren) {
> +        if (!cache)
> +            break;
> 
> AG: return ? Or we need to exec the rest of the function ?

I think we’d still want to resolve appends, which we do at the end of this method.

[Tyler Wilcock]

cq-, let's make sure EWS is still OK

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #30)
> cq-, let's make sure EWS is still OK

I think it won't be landed if something fails, so it makes no difference whether you CQ+ now or later, unless there are more comments of course.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #31)
> (In reply to Tyler Wilcock from comment #30)
> > cq-, let's make sure EWS is still OK
> 
> I think it won't be landed if something fails, so it makes no difference
> whether you CQ+ now or later, unless there are more comments of course.
Commit queue only checks that Mac WK2 builds and passes layout tests, it doesn't wait for anything else as far as I know

[EWS]

Committed 271907@main (d39e84f7bea7): <https://commits.webkit.org/271907@main>

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