AX: Accessibility relations become stale for objects that have a rend…

…erer attached after their creation

https://bugs.webkit.org/show_bug.cgi?id=261643
rdar://problem/115604027

Reviewed by Andres Gonzalez.

This patch fixes a bug initiated by this sequence:

  1. We create an AX object for an element that will eventually have a
     renderer, but does not have it yet. This object has some
     relationship to another element (e.g. via aria-activedescendant)
  2. Something triggers AXObjectCache::m_relations to be built.
  3. The renderer is created and attached to the element. AXObjectCache::onRendererCreated(Element&)
     is called, removing the old AX object. m_relations still has the now-stale entry to this old AX object.

With this patch, we detect this scenario and dirty relations when necessary.

This bug blocked fixing accessibility/mac/grid-selected-cells.html in ITM. But a couple more changes
were necessary to fully fix it:

  * Move AccessibilityObject::selectedCells to AXCoreObject and remove
    AXPropertyName::SelectedCells entirely. This obviates the need to update
    the property when relations change, as it is dependent on AXRelationType::ActiveDescendant.
  * Fix a bug where cells that dynamically change their aria-selected
    attribute were not updating AXPropertyName::IsSelected.

With this patch, accessibility/mac/grid-selected-cells.html passes in ITM.

* LayoutTests/accessibility-isolated-tree/TestExpectations:
* LayoutTests/accessibility/mac/grid-selected-cells-expected.txt:
* LayoutTests/accessibility/mac/grid-selected-cells.html:
* Source/WebCore/accessibility/AXLogger.cpp:
(WebCore::operator<<):
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::focusedObjectForNode):
(WebCore::AXObjectCache::onRendererCreated):
(WebCore::AXObjectCache::handleActiveDescendantChange):
(WebCore::AXObjectCache::performDeferredCacheUpdate):
(WebCore::AXObjectCache::updateIsolatedTree):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::selectedCells): Deleted.
(WebCore::AccessibilityObject::activeDescendant const): Deleted.
* Source/WebCore/accessibility/AccessibilityObject.h:
* Source/WebCore/accessibility/AccessibilityObjectInterface.h:
(WebCore::AXCoreObject::activeDescendant const):
(WebCore::AXCoreObject::selectedCells):
(WebCore::AXCoreObject::canHaveSelectedCells const): Deleted.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::initializeProperties):
(WebCore::AXIsolatedObject::activeDescendant const): Deleted.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:

Canonical link: https://commits.webkit.org/268122@main

LayoutTests/accessibility-isolated-tree/TestExpectations

@@ -33,13 +33,6 @@ accessibility/mac/attributed-string/attributed-string-has-completion-annotation.
 accessibility/mac/text-input-marked-range.html [ Failure ]
 accessibility/mac/text-input-marked-text-marker-range.html [ Failure ]
 
-# Fails because we build relations too soon before elements have renderers because calls to
-# AccessibilityNodeObject::ownerParentObject() require building relations very early in tree building.
-# One possible solution would be in AXObjectCache::addRelation(Element* origin, Element* target, AXRelationType relationType),
-# if either origin or target elements are !Element::renderer() && Element::rendererIsNeeded(const RenderStyle&), then we have
-# to queue them up to be processed after a timer in a new m_deferredRelationsUpdate list.
-accessibility/mac/grid-selected-cells.html [ Failure ]
-
 # Frames are off by 1px vs. non-ITM mode.
 accessibility/table-cells.html [ Failure ]
 accessibility/mac/document-links.html [ Failure ]

LayoutTests/accessibility/mac/grid-selected-cells-expected.txt

@@ -1,14 +1,11 @@
 This tests that the ARIA grids will return and post selected cells.
 
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
 PASS: grid.isAttributeSupported('AXSelectedCells') === true
 PASS: grid.selectedCells()[0].isEqual(selectedCell) === true
-Received notification: AXSelectedCellsChanged
-PASS: grid.selectedCells()[0].isEqual(accessibilityController.accessibleElementById('cell1')) === true
-PASS: grid.selectedCells()[1].isEqual(accessibilityController.accessibleElementById('cell2')) === true
-Received notification: AXSelectedCellsChanged
+Received notification AXSelectedCellsChanged for #grid
+Received notification AXSelectedCellsChanged for #cell1
+PASS: selectedCells[0].isEqual(accessibilityController.accessibleElementById('cell1')) === true
+PASS: selectedCells[1].isEqual(accessibilityController.accessibleElementById('cell2')) === true
 
 PASS successfullyParsed is true
 

LayoutTests/accessibility/mac/grid-selected-cells.html

@@ -1,8 +1,8 @@
 <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
 <html>
 <head>
-<script src="../../resources/js-test.js"></script>
 <script src="../../resources/accessibility-helper.js"></script>
+<script src="../../resources/js-test.js"></script>
 </head>
 <body>
 
@@ -29,29 +29,22 @@
 
 <script>
 
-description("This tests that the ARIA grids will return and post selected cells.");
+var output = "This tests that the ARIA grids will return and post selected cells.\n\n";
 
 if (window.accessibilityController) {
     window.jsTestIsAsync = true
 
-    var output = "";
-    var grid, selectedCelll;
-    var notificationCount = 0;
+    var grid, selectedCell, selectedCells, notificationCount = 0;
     accessibilityController.addNotificationListener(function(element, notification) {
         if (notification == "AXSelectedCellsChanged") {
-            output += "Received notification: " + notification + "\n";
+            output += `Received notification ${notification} for #${element.domIdentifier}\n`;
             notificationCount++;
-            if (notificationCount == 2) {
-                document.getElementById("content").style.visibility = 'hidden';
-                debug(output);
-                finishJSTest();
-            }
         }
     });
 
     // Focus the grid so it can handle active descendants.
     document.getElementById("grid").focus();
-    setTimeout(function() {
+    setTimeout(async function() {
         grid = accessibilityController.accessibleElementById("grid");
         selectedCell = grid.childAtIndex(2).childAtIndex(0);
         output += expect("grid.isAttributeSupported('AXSelectedCells')", "true");
@@ -62,8 +55,18 @@
 
         // Now change with aria-selected to ensure we also get the right notification.
         document.getElementById("cell1").setAttribute("aria-selected", "true");
-        output += expect("grid.selectedCells()[0].isEqual(accessibilityController.accessibleElementById('cell1'))", "true");
-        output += expect("grid.selectedCells()[1].isEqual(accessibilityController.accessibleElementById('cell2'))", "true");
+        await waitFor(() => {
+            if (notificationCount < 2)
+                return false;
+            selectedCells = grid.selectedCells();
+            return selectedCells.length >= 2;
+        });
+        output += expect("selectedCells[0].isEqual(accessibilityController.accessibleElementById('cell1'))", "true");
+        output += expect("selectedCells[1].isEqual(accessibilityController.accessibleElementById('cell2'))", "true");
+
+        document.getElementById("content").style.visibility = 'hidden';
+        debug(output);
+        finishJSTest();
     }, 0);
 }
 </script>

Source/WebCore/accessibility/AXLogger.cpp

@@ -452,6 +452,71 @@ TextStream& operator<<(TextStream& stream, AccessibilityObjectInclusion inclusio
     return stream;
 }
 
+TextStream& operator<<(TextStream& stream, AXRelationType relationType)
+{
+    switch (relationType) {
+    case AXRelationType::None:
+        stream << "None";
+        break;
+    case AXRelationType::ActiveDescendant:
+        stream << "ActiveDescendant";
+        break;
+    case AXRelationType::ActiveDescendantOf:
+        stream << "ActiveDescendantOf";
+        break;
+    case AXRelationType::ControlledBy:
+        stream << "ControlledBy";
+        break;
+    case AXRelationType::ControllerFor:
+        stream << "ControllerFor";
+        break;
+    case AXRelationType::DescribedBy:
+        stream << "DescribedBy";
+        break;
+    case AXRelationType::DescriptionFor:
+        stream << "DescriptionFor";
+        break;
+    case AXRelationType::Details:
+        stream << "Details";
+        break;
+    case AXRelationType::DetailsFor:
+        stream << "DetailsFor";
+        break;
+    case AXRelationType::ErrorMessage:
+        stream << "ErrorMessage";
+        break;
+    case AXRelationType::ErrorMessageFor:
+        stream << "ErrorMessageFor";
+        break;
+    case AXRelationType::FlowsFrom:
+        stream << "FlowsFrom";
+        break;
+    case AXRelationType::FlowsTo:
+        stream << "FlowsTo";
+        break;
+    case AXRelationType::Headers:
+        stream << "Headers";
+        break;
+    case AXRelationType::HeaderFor:
+        stream << "HeaderFor";
+        break;
+    case AXRelationType::LabelledBy:
+        stream << "LabelledBy";
+        break;
+    case AXRelationType::LabelFor:
+        stream << "LabelFor";
+        break;
+    case AXRelationType::OwnedBy:
+        stream << "OwnedBy";
+        break;
+    case AXRelationType::OwnerFor:
+        stream << "OwnerFor";
+        break;
+    }
+
+    return stream;
+}
+
 TextStream& operator<<(TextStream& stream, AXObjectCache::AXNotification notification)
 {
     switch (notification) {

Source/WebCore/accessibility/AXObjectCache.cpp

@@ -504,7 +504,7 @@ AccessibilityObject* AXObjectCache::focusedObjectForNode(Node* focusedNode)
 
     if (focus->shouldFocusActiveDescendant()) {
         if (auto* descendant = focus->activeDescendant())
-            return descendant;
+            return dynamicDowncast<AccessibilityObject>(descendant);
     }
 
     if (focus->accessibilityIsIgnored())
@@ -1185,7 +1185,7 @@ void AXObjectCache::onRendererCreated(Element& element)
         // update and remove(AXID) updates the isolated tree, that in turn calls
         // parentObjectUnignored() on the object being removed, that may result
         // in a call to textUnderElement, that can not be called during a layout.
-        m_deferredRemovedObjects.add(axID);
+        m_deferredReplacedObjects.add(axID);
         if (!m_performCacheUpdateTimer.isActive())
             m_performCacheUpdateTimer.startOneShot(0_s);
     }
@@ -2222,7 +2222,7 @@ void AXObjectCache::handleActiveDescendantChange(Element& element, const AtomStr
     if (element.document().focusedElement() != &element)
         return;
 
-    RefPtr activeDescendant = object->activeDescendant();
+    RefPtr activeDescendant = dynamicDowncast<AccessibilityObject>(object->activeDescendant());
     if (!activeDescendant) {
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
         if (object->shouldFocusActiveDescendant()
@@ -2272,12 +2272,8 @@ void AXObjectCache::handleActiveDescendantChange(Element& element, const AtomStr
         postPlatformNotification(target.get(), AXNotification::AXActiveDescendantChanged);
 
         // Table cell active descendant changes should trigger selected cell changes.
-        if (target->isTable() && activeDescendant->isExposedTableCell()) {
-#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-            updateIsolatedTree(target.get(), AXNotification::AXSelectedCellsChanged);
-#endif
+        if (target->isTable() && activeDescendant->isExposedTableCell())
             postPlatformNotification(target.get(), AXSelectedCellsChanged);
-        }
     }
 }
 
@@ -3862,10 +3858,21 @@ void AXObjectCache::performDeferredCacheUpdate()
     }
     SetForScope performingDeferredCacheUpdate(m_performingDeferredCacheUpdate, true);
 
-    AXLOGDeferredCollection("RemovedObjects"_s, m_deferredRemovedObjects);
-    for (AXID axID : m_deferredRemovedObjects)
+    bool markedRelationsDirty = false;
+    auto markRelationsDirty = [&] () {
+        if (!markedRelationsDirty) {
+            relationsNeedUpdate(true);
+            markedRelationsDirty = true;
+        }
+    };
+    AXLOGDeferredCollection("ReplacedObjectsList"_s, m_deferredReplacedObjects);
+    for (AXID axID : m_deferredReplacedObjects) {
+        // If the replaced object was part of any relation, we need to make sure the relations are updated.
+        if (m_relations.contains(axID))
+            markRelationsDirty();
         remove(axID);
-    m_deferredRemovedObjects.clear();
+    }
+    m_deferredReplacedObjects.clear();
 
     AXLOGDeferredCollection("RecomputeTableIsExposedList"_s, m_deferredRecomputeTableIsExposedList);
     m_deferredRecomputeTableIsExposedList.forEach([this] (auto& tableElement) {
@@ -3916,15 +3923,12 @@ void AXObjectCache::performDeferredCacheUpdate()
     m_deferredTextFormControlValue.clear();
 
     AXLOGDeferredCollection("AttributeChange"_s, m_deferredAttributeChange);
-    bool idAttributeChanged = false;
     for (const auto& attributeChange : m_deferredAttributeChange) {
         handleAttributeChange(attributeChange.element.get(), attributeChange.attrName, attributeChange.oldValue, attributeChange.newValue);
         if (attributeChange.attrName == idAttr)
-            idAttributeChanged = true;
+            markRelationsDirty();
     }
     m_deferredAttributeChange.clear();
-    if (idAttributeChanged)
-        relationsNeedUpdate(true);
 
     if (m_deferredFocusedNodeChange) {
         AXLOG(makeString(
@@ -4133,9 +4137,9 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXRowIndexChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::AXRowIndex);
             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:
-            tree->updateNodeProperty(*notification.first, AXPropertyName::SelectedCells);
-            break;
         case AXSelectedStateChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::IsSelected);
             break;

Source/WebCore/accessibility/AXObjectCache.h

@@ -654,7 +654,9 @@ class AXObjectCache : public CanMakeWeakPtr<AXObjectCache>, public CanMakeChecke
     Timer m_performCacheUpdateTimer;
 
     AXTextStateChangeIntent m_textSelectionIntent;
-    HashSet<AXID> m_deferredRemovedObjects;
+    // An object can be "replaced" when we create an AX object from the backing element before it has
+    // attached a renderer, but then want to replace it with a new AX object after the renderer has been attached.
+    HashSet<AXID> m_deferredReplacedObjects;
     WeakHashSet<Element, WeakPtrImplWithEventTargetData> m_deferredRecomputeIsIgnoredList;
     WeakHashSet<HTMLTableElement, WeakPtrImplWithEventTargetData> m_deferredRecomputeTableIsExposedList;
     WeakHashSet<AccessibilityTable> m_deferredRecomputeTableCellSlotsList;

Source/WebCore/accessibility/AccessibilityObject.cpp

@@ -3126,25 +3126,6 @@ AXCoreObject* AccessibilityObject::focusedUIElement() const
     return page && axObjectCache ? axObjectCache->focusedObjectForPage(page) : nullptr;
 }
 
-AXCoreObject::AccessibilityChildrenVector AccessibilityObject::selectedCells()
-{
-    if (!isTable())
-        return { };
-
-    AXCoreObject::AccessibilityChildrenVector selectedCells;
-    for (auto& cell : cells()) {
-        if (cell->isSelected())
-            selectedCells.append(cell);
-    }
-
-    if (auto* activeDescendant = this->activeDescendant()) {
-        if (activeDescendant->isExposedTableCell() && !selectedCells.contains(activeDescendant))
-            selectedCells.append(activeDescendant);
-    }
-
-    return selectedCells;
-}
-
 void AccessibilityObject::setSelectedRows(AccessibilityChildrenVector&& selectedRows)
 {
     // Setting selected only makes sense in trees and tables (and tree-tables).
@@ -4304,15 +4285,6 @@ bool AccessibilityObject::shouldFocusActiveDescendant() const
     }
 }
 
-AccessibilityObject* AccessibilityObject::activeDescendant() const
-{
-    auto activeDescendants = relatedObjects(AXRelationType::ActiveDescendant);
-    ASSERT(activeDescendants.size() <= 1);
-    if (!activeDescendants.isEmpty())
-        return dynamicDowncast<AccessibilityObject>(activeDescendants[0].get());
-    return nullptr;
-}
-
 bool AccessibilityObject::isActiveDescendantOfFocusedContainer() const
 {
     auto containers = activeDescendantOfObjects();

Source/WebCore/accessibility/AccessibilityObject.h

@@ -151,7 +151,6 @@ class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr<Accessibi
     AccessibilityChildrenVector columnHeaders() override { return AccessibilityChildrenVector(); }
     AccessibilityChildrenVector rowHeaders() override { return AccessibilityChildrenVector(); }
     AccessibilityChildrenVector visibleRows() override { return AccessibilityChildrenVector(); }
-    AccessibilityChildrenVector selectedCells() override;
     AXCoreObject* headerContainer() override { return nullptr; }
     int axColumnCount() const override { return 0; }
     int axRowCount() const override { return 0; }
@@ -524,7 +523,6 @@ class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr<Accessibi
     void setSelectedChildren(const AccessibilityChildrenVector&) override { }
     AccessibilityChildrenVector visibleChildren() override { return { }; }
     bool shouldFocusActiveDescendant() const;
-    AccessibilityObject* activeDescendant() const final;
 
     WEBCORE_EXPORT static AccessibilityRole ariaRoleToWebCoreRole(const String&);
     virtual bool hasAttribute(const QualifiedName&) const;