Bug 262433

<rdar://problem/116271179>

rdar://116128660

Created attachment 467992 [details]
Patch

Comment on attachment 467992 [details]
Patch

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

> COMMIT_MESSAGE:12
> +From there, a new method, onSelectedTextChanged, handles updating the isolated tree's

do we have a way to cache this data if accessibility was not enabled initially?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1009
> +    fprintf(stderr, "JDH: selectedTextMarkerRange()\n");

remove

Comment on attachment 467992 [details]
Patch

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

>> COMMIT_MESSAGE:12
>> +From there, a new method, onSelectedTextChanged, handles updating the isolated tree's
> 
> do we have a way to cache this data if accessibility was not enabled initially?

Good point—when we create an isolated tree we should just be able to grab the visible selection like we do in the live object? I can add that to this patch.

(In reply to Joshua Hoffman from comment #3)
> Created attachment 467992 [details]
> Patch
When text selection changes, the AXObjectCache is notified through a call to
postTextStateChangeNotification from FrameSelection::notifyAccessibilityForSelectionChange.

AG: is this also true when the selection changes happens in non-text objects like images and buttons?

From there, a new method, onSelectedTextChanged, handles updating the isolated tree's
m_selectedTextMarkerRange with the relevant text markers for the start and end of the
selection. These are stored in a pair of TextMarkerData structs.

An important piece of this is that we need the nodes of the start and end text marker

AG: Typo: start and end marker -> start and end markers

to exist in the isolated tree. So, if one or both of these nodes are ignored, they are added
to the isolated tree as unconnected nodes. This is handled in onSelectedTextChanged.

Finally, when the range is requested via AXIsolatedObject::selectedTextMarkerRange(),
an AXTextMarkerRange is constructed from the cached TextMarkerData pair.

This patch also modifies previous tests to make them asynchronous for this new behavior.

* LayoutTests/accessibility/mac/doctype-node-in-text-marker-crash-expected.txt:
* LayoutTests/accessibility/mac/doctype-node-in-text-marker-crash.html:
* LayoutTests/accessibility/mac/replace-text-with-range-expected.txt:
* LayoutTests/accessibility/mac/replace-text-with-range-value-change-notification.html:
* LayoutTests/accessibility/mac/replace-text-with-range.html:
* LayoutTests/accessibility/mac/text-marker-p-tags-expected.txt:
* LayoutTests/accessibility/mac/text-marker-p-tags.html:
* Source/WebCore/accessibility/AXCoreObject.h:
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::postTextStateChangeNotification):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::selectedTextMarkerRange):
* Source/WebCore/accessibility/AccessibilityObject.h:
(WebCore::AccessibilityObject::selectedVisiblePositionRange const):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::selectedTextMarkerRange):
(WebCore::AXIsolatedObject::selectedVisiblePositionRange const): Deleted.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:
(WebCore::AXIsolatedTree::selectedTextMarkerRange):
(WebCore::AXIsolatedTree::setSelectedTextMarkerRange):
* Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:
(WebCore::AXObjectCache::onSelectedTextChanged):
* Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
(-[WebAccessibilityObjectWrapper selectedTextMarkerRange]):
---
 Source/WebCore/accessibility/AXCoreObject.h   |  4 +-
 .../WebCore/accessibility/AXObjectCache.cpp   |  3 +
 Source/WebCore/accessibility/AXObjectCache.h  |  3 +
 .../accessibility/AccessibilityObject.cpp     | 10 ++
 .../accessibility/AccessibilityObject.h       |  6 +-
 .../isolatedtree/AXIsolatedObject.cpp         | 18 ++--
 .../isolatedtree/AXIsolatedObject.h           |  4 +-
 .../isolatedtree/AXIsolatedTree.h             |  5 +
 .../accessibility/mac/AXObjectCacheMac.mm     | 24 +++++
 .../mac/WebAccessibilityObjectWrapperMac.mm   | 17 ++--
 ...ype-node-in-text-marker-crash-expected.txt |  9 +-
 .../doctype-node-in-text-marker-crash.html    | 95 ++++++++++---------
 .../mac/replace-text-with-range-expected.txt  | 18 ++--
 ...-with-range-value-change-notification.html | 59 +++++++-----
 .../mac/replace-text-with-range.html          | 42 ++++----
 .../mac/text-marker-p-tags-expected.txt       |  6 +-
 .../accessibility/mac/text-marker-p-tags.html | 39 +++++---
 17 files changed, 222 insertions(+), 140 deletions(-)

diff --git a/Source/WebCore/accessibility/AXCoreObject.h b/Source/WebCore/accessibility/AXCoreObject.h
index 0c2df24ba1b2..c51043b0e158 100644
--- a/Source/WebCore/accessibility/AXCoreObject.h
+++ b/Source/WebCore/accessibility/AXCoreObject.h
@@ -1277,12 +1277,14 @@ public:
     virtual VisiblePositionRange styleRangeForPosition(const VisiblePosition&) const = 0;
     virtual VisiblePositionRange visiblePositionRangeForRange(const CharacterRange&) const = 0;
     virtual VisiblePositionRange lineRangeForPosition(const VisiblePosition&) const = 0;
-    virtual VisiblePositionRange selectedVisiblePositionRange() const = 0;

     virtual std::optional<SimpleRange> rangeForCharacterRange(const CharacterRange&) const = 0;
 #if PLATFORM(COCOA)
     virtual AXTextMarkerRange textMarkerRangeForNSRange(const NSRange&) const = 0;
 #endif
+#if PLATFORM(MAC)
+    virtual AXTextMarkerRange selectedTextMarkerRange() = 0;
+#endif

AG: don't we need this in other platforms? It should work the same.

     virtual String stringForRange(const SimpleRange&) const = 0;
     virtual IntRect boundsForRange(const SimpleRange&) const = 0;
diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 1922e7bbd960..ae78e92330b8 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -1983,6 +1983,9 @@ void AXObjectCache::postTextStateChangeNotification(AccessibilityObject* object,
         const AXTextStateChangeIntent& newIntent = (intent.type == AXTextStateChangeTypeUnknown || (m_isSynchronizingSelection && m_textSelectionIntent.type != AXTextStateChangeTypeUnknown)) ? m_textSelectionIntent : intent;
         postTextStateChangePlatformNotification(object, newIntent, selection);
     }
+#if PLATFORM(MAC)
+    onSelectedTextChanged(selection);
+#endif

AG: I think it should be the same for iOS and ATSPI.

 #else
     UNUSED_PARAM(object);
     UNUSED_PARAM(intent);
diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index c26ee022b20c..1abe4a313e63 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -211,6 +211,9 @@ public:
     // an AXNodeObject. This occurs when an Element with no renderer is
     // re-parented into a subtree that does have a renderer.
     void onRendererCreated(Element&);
+#if PLATFORM(MAC)
+    void onSelectedTextChanged(const VisiblePositionRange&);
+#endif

AG: same here.

     void updateLoadingProgress(double);
     void loadingFinished() { updateLoadingProgress(1); }
diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
index 7d0f55b40908..982bfd4e022e 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -1665,6 +1665,16 @@ VisiblePositionRange AccessibilityObject::lineRangeForPosition(const VisiblePosi
     return { startOfLine(visiblePosition), endOfLine(visiblePosition) };
 }

+#if PLATFORM(MAC)
+AXTextMarkerRange AccessibilityObject::selectedTextMarkerRange()
+{
+    auto visibleRange = selectedVisiblePositionRange();
+    if (visibleRange.isNull())
+        return nil;

AG: return { }; i.e., the AXTextMarkerRange object created with the default constructor.

+    return AXTextMarkerRange(visibleRange);

AG: you should be able to write just:

    return { visibleRange };

No need for the if statement.

+}
+#endif
+
 bool AccessibilityObject::replacedNodeNeedsCharacter(Node* replacedNode)
 {
     // we should always be given a rendered node and a replaced node, but be safe
diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h
index d29dbcbb23d8..56cb4a7ff952 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h
@@ -552,13 +552,15 @@ public:
     VisiblePositionRange styleRangeForPosition(const VisiblePosition&) const override;
     VisiblePositionRange visiblePositionRangeForRange(const CharacterRange&) const override;
     VisiblePositionRange lineRangeForPosition(const VisiblePosition&) const override;
-    VisiblePositionRange selectedVisiblePositionRange() const override { return { }; }
+    virtual VisiblePositionRange selectedVisiblePositionRange() const { return { }; }

     std::optional<SimpleRange> rangeForCharacterRange(const CharacterRange&) const override;
 #if PLATFORM(COCOA)
     AXTextMarkerRange textMarkerRangeForNSRange(const NSRange&) const override;
 #endif
-
+#if PLATFORM(MAC)
+    AXTextMarkerRange selectedTextMarkerRange() override;
+#endif

AG: should be the same in other platforms.

     static String stringForVisiblePositionRange(const VisiblePositionRange&);
     String stringForRange(const SimpleRange&) const override;
     virtual IntRect boundsForVisiblePositionRange(const VisiblePositionRange&) const { return IntRect(); }
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 90d86087e862..57cc5b9e981a 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -1003,6 +1003,15 @@ std::optional<SimpleRange> AXIsolatedObject::rangeForCharacterRange(const Charac
     return axObject ? axObject->rangeForCharacterRange(axRange) : std::nullopt;
 }

+#if PLATFORM(MAC)

AG: same for other platforms.

+AXTextMarkerRange AXIsolatedObject::selectedTextMarkerRange()
+{
+    fprintf(stderr, "JDH: selectedTextMarkerRange()\n");

AG: debugging leftover.

+    auto textMarkerPair = tree()->selectedTextMarkerRange();
+    return { textMarkerPair.first, textMarkerPair.second };
+}
+#endif
+
 String AXIsolatedObject::stringForRange(const SimpleRange& range) const
 {
     ASSERT(isMainThread());
@@ -1367,15 +1376,6 @@ VisibleSelection AXIsolatedObject::selection() const
     return object ? object->selection() : VisibleSelection();
 }

-VisiblePositionRange AXIsolatedObject::selectedVisiblePositionRange() const
-{
-    ASSERT(isMainThread());
-
-    if (auto* axObject = associatedAXObject())
-        return axObject->selectedVisiblePositionRange();
-    return { };
-}
-
 void AXIsolatedObject::setSelectedVisiblePositionRange(const VisiblePositionRange& visiblePositionRange) const
 {
     ASSERT(isMainThread());
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index 47d3e7c6d7c4..4666a253a78f 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -353,7 +353,6 @@ private:
     unsigned doAXLineForIndex(unsigned) override;

     VisibleSelection selection() const override;
-    VisiblePositionRange selectedVisiblePositionRange() const override;
     void setSelectedVisiblePositionRange(const VisiblePositionRange&) const override;

     std::optional<SimpleRange> simpleRange() const override;
@@ -376,6 +375,9 @@ private:
     std::optional<SimpleRange> rangeForCharacterRange(const CharacterRange&) const override;
 #if PLATFORM(COCOA)
     AXTextMarkerRange textMarkerRangeForNSRange(const NSRange&) const override;
+#endif
+#if PLATFORM(MAC)

AG: same for other platforms.

+    AXTextMarkerRange selectedTextMarkerRange() override;
 #endif
     String stringForRange(const SimpleRange&) const override;
     IntRect boundsForRange(const SimpleRange&) const override;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index 3d91e290da4e..40be74a9d159 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -28,6 +28,7 @@
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)

 #include "AXCoreObject.h"
+#include "AXTextMarker.h"
 #include "AXTreeStore.h"
 #include "PageIdentifier.h"
 #include "RenderStyleConstants.h"
@@ -327,6 +328,9 @@ public:
     // Use only if the s_storeLock is already held like in findAXTree.
     WEBCORE_EXPORT OptionSet<ActivityState> lockedPageActivityState() const;

+    std::pair<TextMarkerData, TextMarkerData> selectedTextMarkerRange() { return m_selectedTextMarkerRange; };

AG: Don't need a pair, return an AXTextMarkerRange.

+    void setSelectedTextMarkerRange(std::pair<TextMarkerData, TextMarkerData> data) { m_selectedTextMarkerRange = data; }

AG: don't need a pair, pass an AXTextMarkerRange.

+
 private:
     AXIsolatedTree(AXObjectCache&);
     static void storeTree(AXObjectCache&, const Ref<AXIsolatedTree>&);
@@ -409,6 +413,7 @@ private:
     std::atomic<bool> m_relationsNeedUpdate { true };

     Lock m_changeLogLock;
+    std::pair<TextMarkerData, TextMarkerData> m_selectedTextMarkerRange { };

AG: AXTextMarkerRange m_selectedTextMarkerRange;
 };

 inline AXObjectCache* AXIsolatedTree::axObjectCache() const
diff --git a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
index b1cfe1b0f48b..3fa2ac38ffe2 100644
--- a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
+++ b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
@@ -917,6 +917,30 @@ std::optional<SimpleRange> rangeForTextMarkerRange(AXObjectCache* cache, AXTextM
     return cache->rangeForUnorderedCharacterOffsets(startCharacterOffset, endCharacterOffset);
 }

+void AXObjectCache::onSelectedTextChanged(const VisiblePositionRange& selection)
+{
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    if (auto tree = AXIsolatedTree::treeForPageID(m_pageID)) {
+        if (selection.isNull())
+            tree->setSelectedTextMarkerRange(std::pair(TextMarkerData(), TextMarkerData()));
+        else {
+            auto startTextMarker = textMarkerDataForVisiblePosition(selection.start);
+            auto endTextMarker = textMarkerDataForVisiblePosition(selection.end);
+
+            if (startTextMarker && endTextMarker) {
+                auto* startObject = get(startTextMarker->node);
+                auto* endObject = get(endTextMarker->node);
+                createIsolatedObjectIfNeeded(*startObject, m_pageID);
+                createIsolatedObjectIfNeeded(*endObject, m_pageID);
+                tree->setSelectedTextMarkerRange(std::pair(startTextMarker.value(), endTextMarker.value()));
+            }
+        }
+    }
+#else
+    UNUSED_PARAM(selection);
+#endif
+}
+
 }

 #endif // ENABLE(ACCESSIBILITY) && PLATFORM(MAC)

Comment on attachment 467992 [details]
Patch

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

>>> COMMIT_MESSAGE:12
>>> +From there, a new method, onSelectedTextChanged, handles updating the isolated tree's
>> 
>> do we have a way to cache this data if accessibility was not enabled initially?
> 
> Good point—when we create an isolated tree we should just be able to grab the visible selection like we do in the live object? I can add that to this patch.

Investigated this and it turns out we already get a notifyAccessibilityForSelectionChange when AX is enabled (see LayoutTests/accessibility/mac/text-marker-p-tags.html which already verifies this), so this is handled.

(In reply to Andres Gonzalez from comment #6)
> (In reply to Joshua Hoffman from comment #3)
> > Created attachment 467992 [details]
> > Patch
> 
> AG: is this also true when the selection changes happens in non-text objects
> like images and buttons?

Yes—any frame selection change will also update this. I'll make that note more clear in the commit message. 

> +#if PLATFORM(MAC)
> +    virtual AXTextMarkerRange selectedTextMarkerRange() = 0;
> +#endif
> 
> AG: don't we need this in other platforms? It should work the same.
> 

selectedTextMarkerRange is only called inside WebAccessibilityObjectWrapperMac & the Mac AccessibilityUIElement—so it will only be needed on that platform.

Created attachment 468035 [details]
Patch

Comment on attachment 468035 [details]
Patch

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:930
> +            if (startPosition.isNull() || endPosition.isNull())

we may want to setSelectedTextMarkerRange({ }) in these failure cases too

Created attachment 468047 [details]
Patch

Comment on attachment 468035 [details]
Patch

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

>> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:930
>> +            if (startPosition.isNull() || endPosition.isNull())
> 
> we may want to setSelectedTextMarkerRange({ }) in these failure cases too

Updated this code to do this in the latest patch.

Committed 268821@main (a1746d78059d): <https://commits.webkit.org/268821@main>

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