Bug 267944 – AX: Implement AXStartTextMarkerAttribute and AXEndTextMarkerAttribute off the main-thread

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/121463431>

[Tyler Wilcock]

Created attachment 469513 [details]
Patch

[chris fleizach]

Comment on attachment 469513 [details]
Patch

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

> Source/WebCore/accessibility/AXTextMarker.h:177
> +    AXTextMarker findLast() const;

lastMarker() const?

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXCoreObject.h b/Source/WebCore/accessibility/AXCoreObject.h
index 024823645747..862b34c2ac4e 100644
--- a/Source/WebCore/accessibility/AXCoreObject.h
+++ b/Source/WebCore/accessibility/AXCoreObject.h
@@ -544,6 +544,7 @@ enum class AccessibilityDetachmentType { CacheDestroyed, ElementDestroyed, Eleme

 enum class AccessibilityConversionSpace { Screen, Page };

+// FIXME: This should be replaced by AXDirection (or vice versa).
 enum class AccessibilitySearchDirection {
     Next = 1,
     Previous,
@@ -577,6 +578,9 @@ enum class AccessibilitySearchKey {
     FontColorChange,
     Frame,
     Graphic,
+#if ENABLE(AX_THREAD_TEXT_APIS)
+    HasTextRuns,
+#endif
     HeadingLevel1,
     HeadingLevel2,
     HeadingLevel3,
@@ -1100,6 +1104,9 @@ public:
     virtual String description() const = 0;

     virtual std::optional<String> textContent() const = 0;
+#if ENABLE(AX_THREAD_TEXT_APIS)
+    virtual bool hasTextRuns() = 0;
+#endif

AG: clients of this class shouldn't ever have to call this, except that findMatchingObject now uses it. Maybe it would be a bit more elegant to make the find function that uses it a friend, and make the method private.

     // Methods for determining accessibility text.
     virtual String stringValue() const = 0;
diff --git a/Source/WebCore/accessibility/AXLogger.cpp b/Source/WebCore/accessibility/AXLogger.cpp
index 01b102c97351..4a82dcb33cdd 100644
--- a/Source/WebCore/accessibility/AXLogger.cpp
+++ b/Source/WebCore/accessibility/AXLogger.cpp
@@ -294,6 +294,11 @@ TextStream& operator<<(TextStream& stream, AccessibilitySearchKey searchKey)
     case AccessibilitySearchKey::Graphic:
         stream << "Graphic";
         break;
+#if ENABLE(AX_THREAD_TEXT_APIS)
+    case AccessibilitySearchKey::HasTextRuns:
+        stream << "HasTextRuns";
+        break;
+#endif
     case AccessibilitySearchKey::HeadingLevel1:
         stream << "HeadingLevel1";
         break;
diff --git a/Source/WebCore/accessibility/AXTextMarker.cpp b/Source/WebCore/accessibility/AXTextMarker.cpp
index e13cf8f47756..eb1ec707b88a 100644
--- a/Source/WebCore/accessibility/AXTextMarker.cpp
+++ b/Source/WebCore/accessibility/AXTextMarker.cpp
@@ -390,43 +390,20 @@ static RefPtr<AXIsolatedObject> findObjectWithRuns(AXIsolatedObject& start, AXDi
             return &start;
     }

-    RefPtr current = &start;
-    while (current) {
-        if (current != &start) {
-            auto* runs = current->textRuns();
-            if (runs && runs->size())
-                return current;
-        }
-
-        // FIXME: aria-owns breaks this traversal, as aria-owns causes the AX tree to be changed, affecting
-        // our iteration below, but it doesn't actually change text position on the page. So we need to ignore aria-owns
-        // tree changes here in order to behave correctly.
-        // We also probably need to do something about text within aria-hidden containers, which affects the AX tree.
-
-        const auto& children = current->children();
-        if (children.size()) {
-            size_t childIndex = direction == AXDirection::Next ? 0 : children.size() - 1;
-            RELEASE_ASSERT(children[childIndex]);
-            current = dynamicDowncast<AXIsolatedObject>(children[childIndex].get());
-            continue;
-        }
-
-        // `current` has no children, meaning it's a leaf node (e.g. it's text, which cannot have children).
-        // Check `current`s siblings.
-        if (auto* sibling = current->sibling(direction)) {
-            current = dynamicDowncast<AXIsolatedObject>(sibling);
-            continue;
-        }
+    // FIXME: aria-owns breaks this function, as aria-owns causes the AX tree to be changed, affecting
+    // our search below, but it doesn't actually change text position on the page. So we need to ignore
+    // aria-owns tree changes here in order to behave correctly. We also probably need to do something
+    // about text within aria-hidden containers, which affects the AX tree.

-        // We have no children, and no next/previous sibling. Try our parent's sibling.
-        if (auto* parent = current->parentObjectUnignored()) {
-            current = dynamicDowncast<AXIsolatedObject>(parent->sibling(direction));
-            continue;
-        }
+    AccessibilitySearchCriteria criteria = { &start, direction == AXDirection::Next ? AccessibilitySearchDirection::Next : AccessibilitySearchDirection::Previous, emptyString(), 1, false, false };

AG: don't need the =.

+    RefPtr tree = std::get<RefPtr<AXIsolatedTree>>(axTreeForID(start.treeID()));
+    RefPtr root = tree ? tree->rootNode() : nullptr;
+    criteria.anchorObject = root ? root.get() : &start;

AG: shouldn't we early return if no root?

+    criteria.searchKeys = { AccessibilitySearchKey::HasTextRuns };

-        break;
-    }
-    return nullptr;
+    AXCoreObject::AccessibilityChildrenVector results;
+    Accessibility::findMatchingObjects(criteria, results);
+    return results.isEmpty() ? nullptr : dynamicDowncast<AXIsolatedObject>(results[0]);
 }

 unsigned AXTextMarker::offsetFromRoot() const
@@ -472,6 +449,31 @@ AXTextMarker AXTextMarker::nextMarkerFromOffset(unsigned offset) const
     return marker;
 }

+AXTextMarker AXTextMarker::findLast() const
+{
+    RELEASE_ASSERT(!isMainThread());
+
+    if (!isValid())
+        return { };
+    if (!isInTextLeaf()) {
+        auto textLeafMarker = toTextLeafMarker();
+        // We couldn't turn this non-text-leaf marker into a marker pointing to actual text, e.g. because
+        // this marker points at an empty container / group at the end of the document. In this case, we
+        // call ourselves the last marker.
+        if (!textLeafMarker.isValid())
+            return *this;
+        return textLeafMarker.findLast();
+    }
+
+    AXTextMarker marker = { };

AG: don't need = { }

+    auto newMarker = *this;
+    while (newMarker.isValid()) {
+        marker = WTFMove(newMarker);
+        newMarker = marker.findMarker(AXDirection::Next);
+    }
+    return marker;
+}
+
 String AXTextMarkerRange::toString() const
 {
     RELEASE_ASSERT(!isMainThread());
@@ -517,7 +519,6 @@ AXTextMarker AXTextMarker::findMarker(AXDirection direction) const
         return { };
     if (!isInTextLeaf())
         return toTextLeafMarker().findMarker(direction);
-    RELEASE_ASSERT(isInTextLeaf());

     size_t runIndex = runs()->indexForOffset(offset());
     RELEASE_ASSERT(runIndex != notFound);
@@ -549,7 +550,6 @@ AXTextMarker AXTextMarker::findMarker(AXDirection direction, AXTextUnit textUnit
         return { };
     if (!isInTextLeaf())
         return toTextLeafMarker().findMarker(direction, textUnit, boundary);
-    RELEASE_ASSERT(isInTextLeaf());

     if (textUnit == AXTextUnit::Line) {
         size_t runIndex = runs()->indexForOffset(offset());
diff --git a/Source/WebCore/accessibility/AXTextMarker.h b/Source/WebCore/accessibility/AXTextMarker.h
index 7ba414df6b47..15c0f62c9434 100644
--- a/Source/WebCore/accessibility/AXTextMarker.h
+++ b/Source/WebCore/accessibility/AXTextMarker.h
@@ -173,6 +173,8 @@ public:
     AXTextMarker nextMarkerFromOffset(unsigned) const;
     // Returns the number of intermediate text markers between this and the root.
     unsigned offsetFromRoot() const;
+    // Starting from this marker, navigate to the last marker in the entire page.
+    AXTextMarker findLast() const;
 #endif // ENABLE(AX_THREAD_TEXT_APIS)

 private:
diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
index b03f6e6ca656..2fc54061c966 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -4467,6 +4467,10 @@ static bool isAccessibilityObjectSearchMatchAtIndex(RefPtr<AXCoreObject> axObjec
         return axObject->isWebArea();
     case AccessibilitySearchKey::Graphic:
         return axObject->isImage();
+#if ENABLE(AX_THREAD_TEXT_APIS)
+    case AccessibilitySearchKey::HasTextRuns:
+        return axObject->hasTextRuns();
+#endif
     case AccessibilitySearchKey::HeadingLevel1:
         return axObject->headingLevel() == 1;
     case AccessibilitySearchKey::HeadingLevel2:
diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h
index 4f8e7c7d7ba8..acb99b1d4fb9 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h
@@ -385,6 +385,7 @@ public:
     unsigned textLength() const final;
 #if ENABLE(AX_THREAD_TEXT_APIS)
     virtual AXTextRuns textRuns() { return { }; }
+    bool hasTextRuns() final { return textRuns().size(); };

AG: these two may be private.

 #endif
 #if PLATFORM(COCOA)
     // Returns an array of strings and AXObject wrappers corresponding to the
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index cd5c771cf472..e8f43e614aa6 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -563,6 +563,8 @@ void AXIsolatedObject::setSelectedChildren(const AccessibilityChildrenVector& se
 AXCoreObject* AXIsolatedObject::sibling(AXDirection direction) const
 {
     RefPtr parent = parentObjectUnignored();
+    if (!parent)
+        return nullptr;
     const auto& siblings = parent->children();
     size_t indexOfThis = siblings.find(this);
     if (indexOfThis == notFound)
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index 2041c41a3901..e317198ca4a0 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -72,6 +72,11 @@ public:

 #if ENABLE(AX_THREAD_TEXT_APIS)
     const AXTextRuns* textRuns() const;
+    bool hasTextRuns() final
+    {
+        const auto* runs = textRuns();
+        return runs && runs->size();
+    };

AG: extra ; after }. These two may be private.

 #endif

 private:
diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
index ad67056b9aba..c3f7c8d740ac 100644
--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -1936,6 +1936,15 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         return (id)[self selectedTextMarkerRange];

     if ([attributeName isEqualToString:AXStartTextMarkerAttribute]) {
+#if ENABLE(AX_THREAD_TEXT_APIS)
+        if (AXObjectCache::useAXThreadTextApis()) {
+            RefPtr tree = std::get<RefPtr<AXIsolatedTree>>(axTreeForID(backingObject->treeID()));
+            if (RefPtr root = tree ? tree->rootNode() : nullptr) {
+                AXTextMarker rootMarker = { root->treeID(), root->objectID(), 0 };
+                return rootMarker.platformData().bridgingAutorelease();

AG: I think you can write in one line:
    return AXTextMarker { root->treeID(), root->objectID(), 0 }.platformData().bridgingAutorelease();

+            }
+        }
+#endif // ENABLE(AX_THREAD_TEXT_APIS)
         return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
             auto* backingObject = protectedSelf.get().axBackingObject;
             if (!backingObject)
@@ -1946,6 +1955,17 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     }

     if ([attributeName isEqualToString:AXEndTextMarkerAttribute]) {
+#if ENABLE(AX_THREAD_TEXT_APIS)
+        if (AXObjectCache::useAXThreadTextApis()) {
+            RefPtr tree = std::get<RefPtr<AXIsolatedTree>>(axTreeForID(backingObject->treeID()));
+            if (RefPtr root = tree ? tree->rootNode() : nullptr) {
+                const auto& children = root->children();
+                // Start the `findLast` traversal from the last child of the root to reduce the amount of traversal done.
+                RefPtr endObject = children.isEmpty() ? root : dynamicDowncast<AXIsolatedObject>(children[children.size() - 1].get());
+                return AXTextMarker { endObject->treeID(), endObject->objectID(), 0 }.findLast().platformData().bridgingAutorelease();
+            }
+        }
+#endif // ENABLE(AX_THREAD_TEXT_APIS)
         return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
             auto* backingObject = protectedSelf.get().axBackingObject;
             if (!backingObject)

[Andres Gonzalez]

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

I see firstMarker() and lastMarker() as belonging to the tree class instead of to the AXTextMarker class. Since you need the tree any way in the wrapper code, I would move them to AXIsolatedTree.

[Tyler Wilcock]

Created attachment 469602 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #2)
>      virtual std::optional<String> textContent() const = 0;
> +#if ENABLE(AX_THREAD_TEXT_APIS)
> +    virtual bool hasTextRuns() = 0;
> +#endif
> 
> AG: clients of this class shouldn't ever have to call this, except that
> findMatchingObject now uses it. Maybe it would be a bit more elegant to make
> the find function that uses it a friend, and make the method private.
TW: The function that needs this is:

static bool isAccessibilityObjectSearchMatchAtIndex

Can static functions be made to have friend access? Tried a few things but couldn't get it working, maybe you know.

> @@ -385,6 +385,7 @@ public:
>      unsigned textLength() const final;
>  #if ENABLE(AX_THREAD_TEXT_APIS)
>      virtual AXTextRuns textRuns() { return { }; }
> +    bool hasTextRuns() final { return textRuns().size(); };
> 
> AG: these two may be private.
TW: The former (textRuns()) cannot be private, it is used by AXIsolatedObject::initializeProperties. The latter cannot be
private unless we find a way to make your friend suggestion work.

>  #if ENABLE(AX_THREAD_TEXT_APIS)
>      const AXTextRuns* textRuns() const;
> +    bool hasTextRuns() final
> +    {
> +        const auto* runs = textRuns();
> +        return runs && runs->size();
> +    };
> 
> AG: extra ; after }. These two may be private.
TW: The former (textRuns()) cannot be private, it is used by AXTextMarker.cpp. The latter cannot be private unless we find
a way to make your friend suggestion work.

All other comments addressed.

[Andres Gonzalez]

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

+    // Fails because:
+    //  1. We don't include spaces between cells
+    //  2. We don't include newline characters between rows
+    //  3. We miss the table caption text entirely (it is rendered and selectable text, so we should be including it)

AG: Is the test currently failing?

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #8)
> (In reply to Tyler Wilcock from comment #6)
> > Created attachment 469602 [details]
> > Patch
> 
> +    // Fails because:
> +    //  1. We don't include spaces between cells
> +    //  2. We don't include newline characters between rows
> +    //  3. We miss the table caption text entirely (it is rendered and
> selectable text, so we should be including it)
> 
> AG: Is the test currently failing?
TW: With this feature off, no it does not fail. That's why I made a copy of this test and moved that copy into LayoutTests/accessibility/ax-thread-text-apis. This allows us to get some test coverage while we continue fixing up bugs (the alternative would be a big bang patch that somehow implements all APIs with zero bugs at once, which is impossible). This test is still valuable despite partially failing because it confirms that start and end text marker APIs work right (we are including all the text from the entire table, excluding the caption, just missing spaces / newlines between elements).

[EWS]

Committed 273962@main (dffba3d2838b): <https://commits.webkit.org/273962@main>

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