Bug 267477 – AX: Add AccessibilityThreadTextApisEnabled flag and hook it up to several APIs

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

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/120928163>

[Tyler Wilcock]

Created attachment 469387 [details]
Patch

[Joshua Hoffman]

Comment on attachment 469387 [details]
Patch

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

This all looks really great!

> LayoutTests/accessibility/ax-thread-text-apis/text-marker-with-user-select-none-expected.txt:3
> +FAIL: textElement.textMarkerRangeLength(textMarkerRange) !== 43, was 39

Should we be committing these with the expected PASS value, rather than FAIL? I see you have a FIXME below, but I wonder if it's better to get a failure when running the test, rather than a false positive.

[Tyler Wilcock]

(In reply to Joshua Hoffman from comment #3)
> Comment on attachment 469387 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=469387&action=review
> 
> This all looks really great!
> 
> > LayoutTests/accessibility/ax-thread-text-apis/text-marker-with-user-select-none-expected.txt:3
> > +FAIL: textElement.textMarkerRangeLength(textMarkerRange) !== 43, was 39
> 
> Should we be committing these with the expected PASS value, rather than
> FAIL? I see you have a FIXME below, but I wonder if it's better to get a
> failure when running the test, rather than a false positive.
That's a good question. I chose to commit FAIL to try to keep our post-commit bot results clean...but thinking about it more, we're the only ones watching that, and it seems pretty logical to just ignore failures in the ax-thread-text-apis/ folder. Curious to hear what others think, but I think committing the expected PASS (and letting the test fail) instead could make sense.

[chris fleizach]

Comment on attachment 469387 [details]
Patch

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

>>> LayoutTests/accessibility/ax-thread-text-apis/text-marker-with-user-select-none-expected.txt:3
>>> +FAIL: textElement.textMarkerRangeLength(textMarkerRange) !== 43, was 39
>> 
>> Should we be committing these with the expected PASS value, rather than FAIL? I see you have a FIXME below, but I wonder if it's better to get a failure when running the test, rather than a false positive.
> 
> That's a good question. I chose to commit FAIL to try to keep our post-commit bot results clean...but thinking about it more, we're the only ones watching that, and it seems pretty logical to just ignore failures in the ax-thread-text-apis/ folder. Curious to hear what others think, but I think committing the expected PASS (and letting the test fail) instead could make sense.

Is the strategy to fix this before enabling feature? if so, probably ok to commit expected results (PASS) and then when it gets enabled and not skipped, we ensure it passes. otherwise easy to forget a task like this

[Tyler Wilcock]

(In reply to chris fleizach from comment #5)
> Is the strategy to fix this before enabling feature? if so, probably ok to
> commit expected results (PASS) and then when it gets enabled and not
> skipped, we ensure it passes. otherwise easy to forget a task like this
Yes, I'd say all must pass before we can make this feature the default behavior. Sounds good, I'll change this.

[Tyler Wilcock]

Created attachment 469420 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 024ea342700c..a9190f316b87 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -207,6 +207,9 @@ void AccessibilityReplacedText::postTextStateChangeNotification(AXObjectCache* c
 bool AXObjectCache::gAccessibilityEnabled = false;
 bool AXObjectCache::gAccessibilityEnhancedUserInterfaceEnabled = false;
 bool AXObjectCache::gForceDeferredSpellChecking = false;
+#if ENABLE(AX_THREAD_TEXT_APIS)
+bool AXObjectCache::gAccessibilityThreadTextApisEnabled = false;
+#endif

 void AXObjectCache::enableAccessibility()
 {
@@ -261,6 +264,11 @@ AXObjectCache::AXObjectCache(Document& document)
 #endif
     ASSERT(isMainThread());

+#if ENABLE(AX_THREAD_TEXT_APIS)
+    if (auto* frame = document.frame(); frame && frame->isMainFrame())

AG: why do we need to get and check for the main frame? AXObjectCache should be created only for top level docs which should be associated with the mainm frame, right? Maybe the exception being SVG docs.

+        gAccessibilityThreadTextApisEnabled = document.settings().accessibilityThreadTextApisEnabled();
+#endif
+
     // If loading completed before the cache was created, loading progress will have been reset to zero.
     // Consider loading progress to be 100% in this case.
     double loadingProgress = document.page() ? document.page()->progress().estimatedProgress() : 1;

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #8)
> (In reply to Tyler Wilcock from comment #7)
> > Created attachment 469420 [details]
> > Patch
> 
> diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp
> b/Source/WebCore/accessibility/AXObjectCache.cpp
> index 024ea342700c..a9190f316b87 100644
> --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> +++ b/Source/WebCore/accessibility/AXObjectCache.cpp
> @@ -207,6 +207,9 @@ void
> AccessibilityReplacedText::postTextStateChangeNotification(AXObjectCache* c
>  bool AXObjectCache::gAccessibilityEnabled = false;
>  bool AXObjectCache::gAccessibilityEnhancedUserInterfaceEnabled = false;
>  bool AXObjectCache::gForceDeferredSpellChecking = false;
> +#if ENABLE(AX_THREAD_TEXT_APIS)
> +bool AXObjectCache::gAccessibilityThreadTextApisEnabled = false;
> +#endif
> 
>  void AXObjectCache::enableAccessibility()
>  {
> @@ -261,6 +264,11 @@ AXObjectCache::AXObjectCache(Document& document)
>  #endif
>      ASSERT(isMainThread());
> 
> +#if ENABLE(AX_THREAD_TEXT_APIS)
> +    if (auto* frame = document.frame(); frame && frame->isMainFrame())
> 
> AG: why do we need to get and check for the main frame? AXObjectCache should
> be created only for top level docs which should be associated with the mainm
> frame, right? Maybe the exception being SVG docs.
Yes, you've nailed the exact reason why! In tests with SVGs, they were overwriting this static global to the wrong value (it appears that the runtime feature flag values are not passed to SVG documents). And furthermore, this value is read from both the main-thread and secondary thread, so we only want to write to it once here in the main-frame AXObjectCache creation as the secondary thread can't possibly be trying to access it at that point.

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index 5f34e9c59d01..e557c2618f0b 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -272,6 +272,9 @@ public:
     // Note: these may be called from a non-main thread concurrently as other readers.
     static bool accessibilityEnabled() { return gAccessibilityEnabled; }
     static bool accessibilityEnhancedUserInterfaceEnabled() { return gAccessibilityEnhancedUserInterfaceEnabled; }
+#if ENABLE(AX_THREAD_TEXT_APIS)
+    static bool accessibilityThreadTextApisEnabled() { return gAccessibilityThreadTextApisEnabled; }

AG: dont need the accessibility prefix as part of the name of this method, but realize you are following the existing ones.

+#endif

     const Element* rootAXEditableElement(const Node*);
     bool nodeIsTextControl(const Node*);
@@ -677,6 +680,10 @@ private:
     static constexpr bool gForceDeferredSpellChecking { false };
 #endif

+#if ENABLE(AX_THREAD_TEXT_APIS)
+    static bool gAccessibilityThreadTextApisEnabled;

AG: dito.

+#endif
+
     HashSet<AXID> m_idsInUse;

     Timer m_notificationPostTimer;

[Tyler Wilcock]

Created attachment 469428 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
index dc98d073a31e..553c0ebb70af 100644
--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -3234,8 +3234,15 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     if ([attribute isEqualToString:AXTextMarkerRangeForNSRangeAttribute])
         return backingObject->textMarkerRangeForNSRange(range).platformData().bridgingAutorelease();

-    if ([attribute isEqualToString:AXLineTextMarkerRangeForTextMarkerAttribute])
+    if ([attribute isEqualToString:AXLineTextMarkerRangeForTextMarkerAttribute]) {
+#if ENABLE(AX_THREAD_TEXT_APIS)
+        if (AXObjectCache::accessibilityThreadTextApisEnabled() && !isMainThread()) {
+            AXTextMarker inputMarker { textMarker };
+            return inputMarker.lineRange(LineRangeType::Current).platformData().bridgingAutorelease();
+        }
+#endif
         return (id)[self lineTextMarkerRangeForTextMarker:textMarker forUnit:TextUnit::Line];
+    }

     if ([attribute isEqualToString:AXMisspellingTextMarkerRangeAttribute]) {
         return (id)Accessibility::retrieveAutoreleasedValueFromMainThread<AXTextMarkerRangeRef>([&dictionary, protectedSelf = retainPtr(self)] () -> RetainPtr<AXTextMarkerRangeRef> {
@@ -3319,6 +3326,12 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     }

     if ([attribute isEqualToString:AXStringForTextMarkerRangeAttribute]) {
+#if ENABLE(AX_THREAD_TEXT_APIS)
+        if (AXObjectCache::accessibilityThreadTextApisEnabled() && !isMainThread()) {

AG: instead of repeating this all over the place, maybe you can have isThreadTextApisEnabled() that returns this bool, similar to isIsolatedTreEnabled().

+            AXTextMarkerRange range = { textMarkerRange };
+            return range.toString();
+        }
+#endif

...

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #12)
>      if ([attribute isEqualToString:AXStringForTextMarkerRangeAttribute]) {
> +#if ENABLE(AX_THREAD_TEXT_APIS)
> +        if (AXObjectCache::accessibilityThreadTextApisEnabled() &&
> !isMainThread()) {
> 
> AG: instead of repeating this all over the place, maybe you can have
> isThreadTextApisEnabled() that returns this bool, similar to
> isIsolatedTreEnabled().
Great point, will fix this!

[Tyler Wilcock]

Created attachment 469429 [details]
Patch

[EWS]

Committed 273130@main (3a032c58f74b): <https://commits.webkit.org/273130@main>

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