AX: NULL attributed string returned for text markers that contain unc…

…onnected nodes https://bugs.webkit.org/show_bug.cgi?id=270499 rdar://122451549 Reviewed by Tyler Wilcock. If a node is unconnected and we queue up a node change for it, nothing will happen, since shouldCreateNodeChange will return false. This means that if we request a text marker range that points to a stale, unconnected node, we may get a null attributed string since the range being requested is invalid. This change adds a new condition to shouldCreateNodeChange for unconnected nodes, and adds a new test to verify this behavior. * LayoutTests/accessibility/mac/selected-text-range-unconnected-object.html: Added. * LayoutTests/platform/mac/accessibility/mac/selected-text-range-unconnected-object-expected.txt: Added. * Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp: (WebCore::AXIsolatedTree::shouldCreateNodeChange): (WebCore::AXIsolatedTree::addUnconnectedNode): (WebCore::AXIsolatedTree::removeSubtreeFromNodeMap): (WebCore::shouldCreateNodeChange): Deleted. * Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h: Canonical link: https://commits.webkit.org/275694@main
WebKit · Mar 5, 2024 · a8f281f · a8f281f
1 parent c0414c4
commit a8f281f
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 2 deletions.
diff --git a/LayoutTests/accessibility/mac/selected-text-range-unconnected-object.html b/LayoutTests/accessibility/mac/selected-text-range-unconnected-object.html
@@ -0,0 +1,61 @@
+<html>
+<script src="../../resources/accessibility-helper.js"></script>
+<script src="../../resources/js-test.js"></script>
+<body>
+
+<input type="text" id="input" value="Hello" style="font-family: sans-serif">
+
+<script>
+var output = "This tests that the selected text marker range is valid when it references an unconnected node that changes.\n\n"
+
+function textMarkerRangeString(textMarkerRange, root) {
+    const startMarker = root.startTextMarkerForTextMarkerRange(textMarkerRange);
+    const endMarker = root.endTextMarkerForTextMarkerRange(textMarkerRange);
+
+    const startElement = root.accessibilityElementForTextMarker(startMarker);
+    const endElement = root.accessibilityElementForTextMarker(endMarker);
+
+    return `Text Marker: [Length: ${root.textMarkerRangeLength(textMarkerRange)}]\n\tSTART: ${startElement ? startElement.role : "NULL"}\n\tEND: ${endElement ? endElement.role : "NULL"}\n\n`;
+}
+
+if (window.accessibilityController && window.textInputController) {
+    window.jsTestIsAsync = true;
+    let input = document.getElementById("input");
+    input.focus();
+
+    let root = accessibilityController.rootElement.childAtIndex(0);
+
+    var selectedTextMarkerRange;
+    let textInput = accessibilityController.focusedElement;
+    setTimeout(async function() {
+        input.select();
+        await waitFor(() => {
+            selectedTextMarkerRange = textInput.selectedTextMarkerRange();
+            return root.textMarkerRangeLength(selectedTextMarkerRange) == 5;
+
+        });
+        output += textMarkerRangeString(selectedTextMarkerRange, root);
+        output += `Text Marker Range Attributed Text: ${textInput.attributedStringForTextMarkerRange(selectedTextMarkerRange)}\n\n`;
+
+        input.focus();
+        output += "Inserting 'world'\n";
+
+        // Move insertion point to end of input to append text without creating a new static text object.
+        input.setSelectionRange(5, 5);
+        textInput.insertText(' world');
+
+        input.select();
+        await waitFor(() => {
+            selectedTextMarkerRange = textInput.selectedTextMarkerRange();
+            return root.textMarkerRangeLength(selectedTextMarkerRange) == 11;
+        });
+        output += textMarkerRangeString(selectedTextMarkerRange, root);
+        output += `Text Marker Range Attributed Text: ${textInput.attributedStringForTextMarkerRange(selectedTextMarkerRange)}\n\n`;
+
+        debug(output);
+        finishJSTest();
+    }, 0);
+}
+</script>
+</body>
+</html>
diff --git a/...tTests/platform/mac/accessibility/mac/selected-text-range-unconnected-object-expected.txt b/...tTests/platform/mac/accessibility/mac/selected-text-range-unconnected-object-expected.txt
@@ -0,0 +1,28 @@
+This tests that the selected text marker range is valid when it references an unconnected node that changes.
+
+Text Marker: [Length: 5]
+	START: AXRole: AXStaticText
+	END: AXRole: AXStaticText
+
+Text Marker Range Attributed Text: AXFont - {
+    AXFontFamily = Helvetica;
+    AXFontName = Helvetica;
+    AXFontSize = 11;
+}, Hello
+
+Inserting 'world'
+Text Marker: [Length: 11]
+	START: AXRole: AXStaticText
+	END: AXRole: AXStaticText
+
+Text Marker Range Attributed Text: AXFont - {
+    AXFontFamily = Helvetica;
+    AXFontName = Helvetica;
+    AXFontSize = 11;
+}, Hello world
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -260,10 +260,10 @@ void AXIsolatedTree::generateSubtree(AccessibilityObject& axObject)
     queueRemovalsAndUnresolvedChanges({ });
 }
 
-static bool shouldCreateNodeChange(AXCoreObject& axObject)
+bool AXIsolatedTree::shouldCreateNodeChange(AccessibilityObject& axObject)
 {
     // We should never create an isolated object from a detached or ignored object.
-    return !axObject.isDetached() && !axObject.accessibilityIsIgnored();
+    return !axObject.isDetached() && (!axObject.accessibilityIsIgnored() || m_unconnectedNodes.contains(axObject.objectID()));
 }
 
 std::optional<AXIsolatedTree::NodeChange> AXIsolatedTree::nodeChangeForObject(Ref<AccessibilityObject> axObject, AttachWrapper attachWrapper)
@@ -313,6 +313,11 @@ void AXIsolatedTree::addUnconnectedNode(Ref<AccessibilityObject> axObject)
     AXTRACE("AXIsolatedTree::addUnconnectedNode"_s);
     ASSERT(isMainThread());
 
+    if (m_unconnectedNodes.contains(axObject->objectID())) {
+        AXLOG(makeString("AXIsolatedTree::addUnconnectedNode exiting because an isolated object for ", axObject->objectID().loggingString(), " already exists."));
+        return;
+    }
+
     if (axObject->isDetached() || !axObject->wrapper()) {
         AXLOG(makeString("AXIsolatedTree::addUnconnectedNode bailing because associated live object ID ", axObject->objectID().loggingString(), " had no wrapper or is detached. Object is:"));
         AXLOG(axObject.ptr());
@@ -333,6 +338,7 @@ void AXIsolatedTree::addUnconnectedNode(Ref<AccessibilityObject> axObject)
     NodeChange nodeChange { object, nullptr };
     Locker locker { m_changeLogLock };
     m_pendingAppends.append(WTFMove(nodeChange));
+    m_unconnectedNodes.add(axObject->objectID());
 }
 
 void AXIsolatedTree::queueRemovals(Vector<AXID>&& subtreeRemovals)
@@ -1094,6 +1100,9 @@ void AXIsolatedTree::removeSubtreeFromNodeMap(AXID objectID, AccessibilityObject
     if (!objectID.isValid())
         return;
 
+    if (m_unconnectedNodes.remove(objectID))
+        return;
+
     if (!m_nodeMap.contains(objectID)) {
         AXLOG(makeString("Tried to remove AXID ", objectID.loggingString(), " that is no longer in m_nodeMap."));
         return;

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -337,6 +337,7 @@ class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AX
     template<typename U> Vector<RefPtr<AXCoreObject>> objectsForIDs(const U&);
 
     void generateSubtree(AccessibilityObject&);
+    bool shouldCreateNodeChange(AccessibilityObject&);
     void updateNode(AccessibilityObject&);
     enum class ResolveNodeChanges : bool { No, Yes };
     void updateChildren(AccessibilityObject&, ResolveNodeChanges = ResolveNodeChanges::Yes);
@@ -449,6 +450,10 @@ class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AX
     // its ObjectID to its ParentChildrenIDs struct.
     HashMap<AXID, ParentChildrenIDs> m_nodeMap;
 
+    // Only accessed on the main thread.
+    // Stores all nodes that are added via addUnconnectedNode, which do not get stored in m_nodeMap.
+    HashSet<AXID> m_unconnectedNodes;
+
     // Only accessed on the main thread.
     // The key is the ID of the object that will be resolved into an m_pendingAppends NodeChange.
     // The value is whether the wrapper should be attached on the main thread or the AX thread.