Bug 263940 – AX: Move selectedTextRange off of the main-thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joshua Hoffman]

We currently have to go to the main thread to serve selectedTextRange when in isolated tree mode. This should be moved to the AX thread.

rdar://117568720

[Radar WebKit Bug Importer]

<rdar://problem/117713019>

[Joshua Hoffman]

rdar://117568720

[Joshua Hoffman]

Created attachment 468419 [details]
Patch

[Tyler Wilcock]

Comment on attachment 468419 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:2004
> +    for (auto* axObject = object; axObject; axObject = axObject->parentObjectUnignored()) {

Is using axObject->parentObjectUnignored() over axObject->parentObject() necessary? I only ask because parentObjectUnignored() is very expensive.

Might not be possible, but is there any way to hook into an existing traversal vs. adding this new one (I notice we do an observableObject traversal above)? Or is there any information we can use about the context of the change to avoid traversing? For example, do we need to do this when AXTextStateChangeType is Unknown or Edit? Is there any other information we can use or pass into this function to make this cheaper? Do we need to keep iterating after we find a text control container?

> Source/WebCore/accessibility/AXObjectCache.cpp:2006
> +            postNotification(axObject, &document(), AXSelectedTextChanged);

Wondering if we want to use postNotification(axObject, nullptr, AXSelectedTextChanged); The second Document* parameter means that we'll fallback to posting the notification on the document if necessary. Not sure if that's desirable behavior here — maybe so? What do you think?

It's interesting we didn't already post this notification. Does this affect VO announcements in text controls when we select text?

[Tyler Wilcock]

> Might not be possible, but is there any way to hook into an existing
> traversal vs. adding this new one (I notice we do an observableObject
> traversal above)?
To be more precise about this comment — is `observableObject` already the text control that we want? Meaning we don't need to traverse? Or can we not rely on that?

What about contenteditable? I know you have a different patch that's not landed yet that changes those so they're less like text controls, wondering if that will have an effect here (or vice versa, whichever lands first)

[Andres Gonzalez]

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

From 68b0c5aa87c7ca604759985cce3eb1bd60f81dbe Mon Sep 17 00:00:00 2001
From: Joshua Hoffman <[e-mail]>
Date: Mon, 30 Oct 2023 17:59:49 -0700
Subject: [PATCH] AX: Move selectedTextRange off of the main-thread
 https://bugs.webkit.org/show_bug.cgi?id=263940 rdar://117568720

Reviewed by NOBODY (OOPS!).

We currently use the main thread to get the selectedTextRange when in isolated tree mode. In the
effort to completely sever ITM, this should be cached to be served on the AX thread.

This patch caches that value on text input objects, and updates the property it every time text

AG: "the property it every"

selection changes.

* LayoutTests/accessibility/content-editable-as-textarea.html:
* LayoutTests/accessibility/mac/selection-value-changes-for-aria-textbox-expected.txt:
* LayoutTests/accessibility/textbox-role-reports-selection.html:
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::postTextStateChangeNotification):
(WebCore::AXObjectCache::updateIsolatedTree):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::initializeProperties):
(WebCore::AXIsolatedObject::selectedTextRange const): Deleted.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::updateNodeProperties):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:
---
 .../WebCore/accessibility/AXObjectCache.cpp   |  8 ++++++
 .../isolatedtree/AXIsolatedObject.cpp         | 15 +++--------
 .../isolatedtree/AXIsolatedObject.h           |  2 +-
 .../isolatedtree/AXIsolatedTree.cpp           |  3 +++
 .../isolatedtree/AXIsolatedTree.h             |  1 +
 .../content-editable-as-textarea.html         | 13 +++++----
 ...alue-changes-for-aria-textbox-expected.txt |  2 ++
 .../textbox-role-reports-selection.html       | 27 ++++++++++++-------
 8 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 90148f101879..3892bdbed77a 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -2001,6 +2001,11 @@ void AXObjectCache::postTextStateChangeNotification(AccessibilityObject* object,
 #if PLATFORM(MAC)
     onSelectedTextChanged(selection);
 #endif
+    for (auto* axObject = object; axObject; axObject = axObject->parentObjectUnignored()) {
+        if (axObject->isTextControl())
+            postNotification(axObject, &document(), AXSelectedTextChanged);
+    }
+

AG: the call to onSelectedTextChanged above this addition was intended to cache the selectedTextMarkerRange. Why do we need to cache the selected text range again? Doesn't it work for text controls? If it doesn't, we should handle both cases in the same place and update the iso tree once.

AG: BTW, there is a thread safety error in onSelectedTextChanged, cannot set and retrieve the cached selected range without acquiring the lock. So that needs to be fixed as well.

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 468419 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468419&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2004
> > +    for (auto* axObject = object; axObject; axObject = axObject->parentObjectUnignored()) {
> 
> Is using axObject->parentObjectUnignored() over axObject->parentObject()
> necessary? I only ask because parentObjectUnignored() is very expensive.

Yes—this should probably work the same. 
 
> Might not be possible, but is there any way to hook into an existing
> traversal vs. adding this new one (I notice we do an observableObject
> traversal above)? Or is there any information we can use about the context
> of the change to avoid traversing? For example, do we need to do this when
> AXTextStateChangeType is Unknown or Edit? Is there any other information we
> can use or pass into this function to make this cheaper? Do we need to keep
> iterating after we find a text control container?

I can definitely look into that! And agree that it is a good idea. 

As for iterating beyond finding a text control, we need to do this in the case of nested contenteditable markup (we have a test called nested-textareas-value-changed-notifications), but we could perhaps be smarter about this. I will investigate!
 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2006
> > +            postNotification(axObject, &document(), AXSelectedTextChanged);
> 
> Wondering if we want to use postNotification(axObject, nullptr,
> AXSelectedTextChanged); The second Document* parameter means that we'll
> fallback to posting the notification on the document if necessary. Not sure
> if that's desirable behavior here — maybe so? What do you think?
> 
> It's interesting we didn't already post this notification. Does this affect
> VO announcements in text controls when we select text?

That notification sounds better—we only need to post this on input types, so the document wouldn't make sense anyways. 

We had been sending a platform notification for selected text changed on both Mac and iOS in the past. Regular AXSelectedTextChanged notifications are sent for non-Cocoa platforms as well. VO does announce selections both w/ and without this patch and it doesn't modify that behavior.

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #6)
> (In reply to Joshua Hoffman from comment #3)
> AG: the call to onSelectedTextChanged above this addition was intended to
> cache the selectedTextMarkerRange. Why do we need to cache the selected text
> range again? Doesn't it work for text controls? If it doesn't, we should
> handle both cases in the same place and update the iso tree once.

On selected text changed caches the selection for a whole page (and is stored on the isolated tree). This cache is for text within an element and is stored on the isolated object. My initial idea was to find the intersection between the selected text on the page and the visible range of the element, but we need to go to the main thread to get an element's visible position—so that approach does not help us sever the main thread.

> AG: BTW, there is a thread safety error in onSelectedTextChanged, cannot set
> and retrieve the cached selected range without acquiring the lock. So that
> needs to be fixed as well.

Thanks for pointing that out, will get a fix up for that.

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #5)
> > Might not be possible, but is there any way to hook into an existing
> > traversal vs. adding this new one (I notice we do an observableObject
> > traversal above)?
> To be more precise about this comment — is `observableObject` already the
> text control that we want? Meaning we don't need to traverse? Or can we not
> rely on that?

I will look into the observable object—I believe this will cause failures in nested textareas (as alluded to in my comment above), but is worth exploring. 

> What about contenteditable? I know you have a different patch that's not
> landed yet that changes those so they're less like text controls, wondering
> if that will have an effect here (or vice versa, whichever lands first)

Contenteditable will return true for isTextControl in that patch, so it shouldn't have an effect.

[Joshua Hoffman]

Created attachment 468451 [details]
Patch

[Joshua Hoffman]

Created attachment 468456 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 130d1fee5131..48eadec7010a 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -244,6 +244,7 @@ AXObjectCache::AXObjectCache(Document& document)
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
     , m_buildIsolatedTreeTimer(*this, &AXObjectCache::buildIsolatedTree)
     , m_geometryManager(AXGeometryManager::create(*this))
+    , m_selectedTextRangeTimer(*this, &AXObjectCache::selectedTextRangeTimerFired, platformSelectedTextRangeDebounceInterval())
 #endif
 {
     AXTRACE(makeString("AXObjectCache::AXObjectCache 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));
@@ -279,6 +280,8 @@ AXObjectCache::~AXObjectCache()
         object->detach(AccessibilityDetachmentType::CacheDestroyed);

 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    m_selectedTextRangeTimer.stop();
+
     if (m_pageID) {
         if (auto tree = AXIsolatedTree::treeForPageID(*m_pageID))
             tree->setPageActivityState({ });
@@ -1997,6 +2000,12 @@ void AXObjectCache::postTextStateChangeNotification(AccessibilityObject* object,
     if (object) {
         const AXTextStateChangeIntent& newIntent = (intent.type == AXTextStateChangeTypeUnknown || (m_isSynchronizingSelection && m_textSelectionIntent.type != AXTextStateChangeTypeUnknown)) ? m_textSelectionIntent : intent;
         postTextStateChangePlatformNotification(object, newIntent, selection);
+
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+        m_lastDebouncedTextRangeObject = object->objectID();
+        if (!m_selectedTextRangeTimer.isActive())
+            m_selectedTextRangeTimer.restart();
+#endif
     }
 #if PLATFORM(MAC)
     onSelectedTextChanged(selection);
@@ -4198,6 +4207,9 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
         case AXPopoverTargetChanged:
             tree->updateNodeProperties(*notification.first, { AXPropertyName::SupportsExpanded, AXPropertyName::IsExpanded });
             break;
+        case AXSelectedTextChanged:
+            tree->updateNodeProperty(*notification.first, AXPropertyName::SelectedTextRange);
+            break;
         case AXSortDirectionChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::SortDirection);
             break;
@@ -4863,6 +4875,25 @@ AXTextChange AXObjectCache::textChangeForEditType(AXTextEditType type)
 }
 #endif

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+void AXObjectCache::selectedTextRangeTimerFired()
+{
+    if (!accessibilityEnabled())

AG: do we need this check here? Is the assumption that accessibility may have been disabled from the time the timer was scheduled?

+        return;
+
+    m_selectedTextRangeTimer.stop();
+
+    if (m_lastDebouncedTextRangeObject.isValid()) {
+        for (auto* axObject = objectForID(m_lastDebouncedTextRangeObject); axObject; axObject = axObject->parentObject()) {
+            if (axObject->isTextControl())
+                postNotification(axObject, nullptr, AXSelectedTextChanged);
+        }

AG: this walk up will continue after finding a text control even though it will do nothing. To fix and to make it clearer use findAncestor.

+    }
+
+    m_lastDebouncedTextRangeObject = { };
+}
+#endif
+

AG: shouldn't we check for the ancestor text control from the very beginning, and only do all of this in that case? As opposed to doing this and then it turns out that the object we are storing is not a descendant of a text control. If we check at the beginning, we can also store the text control object ID from the start.

AG: also, is the debouncing necessary here? What is different concerning this notification from all the others that are not debounced?

 } // namespace WebCore

 #endif // ENABLE(ACCESSIBILITY)

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #12)
> (In reply to Joshua Hoffman from comment #11)
> > Created attachment 468456 [details]
> > Patch
> 
> 
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> +void AXObjectCache::selectedTextRangeTimerFired()
> +{
> +    if (!accessibilityEnabled())
> 
> AG: do we need this check here? Is the assumption that accessibility may
> have been disabled from the time the timer was scheduled?

Yes—I put this here as a fallback. But since the timer duration is so short it is probably not necessary.

> +    m_selectedTextRangeTimer.stop();
> +
> +    if (m_lastDebouncedTextRangeObject.isValid()) {
> +        for (auto* axObject = objectForID(m_lastDebouncedTextRangeObject);
> axObject; axObject = axObject->parentObject()) {
> +            if (axObject->isTextControl())
> +                postNotification(axObject, nullptr, AXSelectedTextChanged);
> +        }
> 
> AG: this walk up will continue after finding a text control even though it
> will do nothing. To fix and to make it clearer use findAncestor.

We need to handle the case of nested content editable areas, so we have to continue after we find one text control in case there is another higher up in the parent chain.

> +
> +    m_lastDebouncedTextRangeObject = { };
> +}
> +#endif
> +
> 
> AG: shouldn't we check for the ancestor text control from the very
> beginning, and only do all of this in that case? As opposed to doing this
> and then it turns out that the object we are storing is not a descendant of
> a text control. If we check at the beginning, we can also store the text
> control object ID from the start.

I like this idea, but the downside is that we have to walkup the tree for every text selection change, instead of just when the timer fires. My answer to your next question might provide more context here.

> AG: also, is the debouncing necessary here? What is different concerning
> this notification from all the others that are not debounced?

If we don't debounce, we would post a text selection change for every single character that is highlighted. For example, in the case where someone is dragging their mouse to select some text, we only want the selection from when they stop dragging, but if we don't debounce, we will send notifications for each character they cross. If the user is highlighting fast, this could create a lot of extra work.

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #13)
> (In reply to Andres Gonzalez from comment #12)
> > (In reply to Joshua Hoffman from comment #11)
> > > Created attachment 468456 [details]
> > > Patch
> > 
> > 
> > +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> > +void AXObjectCache::selectedTextRangeTimerFired()
> > +{
> > +    if (!accessibilityEnabled())
> > 
> > AG: do we need this check here? Is the assumption that accessibility may
> > have been disabled from the time the timer was scheduled?
> 
> Yes—I put this here as a fallback. But since the timer duration is so short
> it is probably not necessary.

AG: if there is a chance that it can happen, it will indeed happen. Just that we don't do this for other timers, do we?

> 
> > +    m_selectedTextRangeTimer.stop();
> > +
> > +    if (m_lastDebouncedTextRangeObject.isValid()) {
> > +        for (auto* axObject = objectForID(m_lastDebouncedTextRangeObject);
> > axObject; axObject = axObject->parentObject()) {
> > +            if (axObject->isTextControl())
> > +                postNotification(axObject, nullptr, AXSelectedTextChanged);
> > +        }
> > 
> > AG: this walk up will continue after finding a text control even though it
> > will do nothing. To fix and to make it clearer use findAncestor.
> 
> We need to handle the case of nested content editable areas, so we have to
> continue after we find one text control in case there is another higher up
> in the parent chain.

AG: do we need to post notifications for all the nested text controls? That doesn't seem right to me. I think we need to post notification only for the closest text control ancestor.

> 
> > +
> > +    m_lastDebouncedTextRangeObject = { };
> > +}
> > +#endif
> > +
> > 
> > AG: shouldn't we check for the ancestor text control from the very
> > beginning, and only do all of this in that case? As opposed to doing this
> > and then it turns out that the object we are storing is not a descendant of
> > a text control. If we check at the beginning, we can also store the text
> > control object ID from the start.
> 
> I like this idea, but the downside is that we have to walkup the tree for
> every text selection change, instead of just when the timer fires. My answer
> to your next question might provide more context here.
> 
> > AG: also, is the debouncing necessary here? What is different concerning
> > this notification from all the others that are not debounced?
> 
> If we don't debounce, we would post a text selection change for every single
> character that is highlighted. For example, in the case where someone is
> dragging their mouse to select some text, we only want the selection from
> when they stop dragging, but if we don't debounce, we will send
> notifications for each character they cross. If the user is highlighting
> fast, this could create a lot of extra work.

Yes, that makes sense. Maybe we can have a general mechanism to debounce all user interactions that can cause unnecessary bursts of notifications as opposed to having individual timers for each. But since I think this is the first one, not counting the focus one that is still pending, we can go ahead with this patch.

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #14)
> (In reply to Joshua Hoffman from comment #13)
> > (In reply to Andres Gonzalez from comment #12)
> > > (In reply to Joshua Hoffman from comment #11)
> > > > Created attachment 468456 [details]
> > > > Patch
> > > 
> > > 
> > > +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> > > +void AXObjectCache::selectedTextRangeTimerFired()
> > > +{
> > > +    if (!accessibilityEnabled())
> > > 
> > > AG: do we need this check here? Is the assumption that accessibility may
> > > have been disabled from the time the timer was scheduled?
> > 
> > Yes—I put this here as a fallback. But since the timer duration is so short
> > it is probably not necessary.
> 
> AG: if there is a chance that it can happen, it will indeed happen. Just
> that we don't do this for other timers, do we?

True. We do this already in AXObjectCache::notificationPostTimerFired, so I was following the pattern there. 

> > > 
> > > AG: this walk up will continue after finding a text control even though it
> > > will do nothing. To fix and to make it clearer use findAncestor.
> > 
> > We need to handle the case of nested content editable areas, so we have to
> > continue after we find one text control in case there is another higher up
> > in the parent chain.
> 
> AG: do we need to post notifications for all the nested text controls? That
> doesn't seem right to me. I think we need to post notification only for the
> closest text control ancestor.

We have a pre-existing test that checks this behavior: nested-textareas-value-changed-notification.html. Do you think we cioould handle this without notifications to each?

> > If we don't debounce, we would post a text selection change for every single
> > character that is highlighted. For example, in the case where someone is
> > dragging their mouse to select some text, we only want the selection from
> > when they stop dragging, but if we don't debounce, we will send
> > notifications for each character they cross. If the user is highlighting
> > fast, this could create a lot of extra work.
> 
> Yes, that makes sense. Maybe we can have a general mechanism to debounce all
> user interactions that can cause unnecessary bursts of notifications as
> opposed to having individual timers for each. But since I think this is the
> first one, not counting the focus one that is still pending, we can go ahead
> with this patch.

Totally, makes sense to me!

[EWS]

Committed 270340@main (ae0dcb554a69): <https://commits.webkit.org/270340@main>

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