Bug 270616 – AX: VoiceOver doesn't read characters when user presses left / right arrows in Monaco code editor

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

[Dominic Mazzoni]

Created attachment 470221 [details]
Simple repro

rdar://123984168

The Monaco code editor, used as the engine for VS Code, implements a custom text editor. Overall they've gone to great lengths to make it accessible, but the editing feedback when arrowing through character-by-character is broken with the combination of Safari and VoiceOver.

You can try their demo playground here:
https://microsoft.github.io/monaco-editor/playground.html

The underlying issue is that when you press an arrow key, their JavaScript code is intercepting it and moving the cursor programmatically. This results in WebKit generating an editing intent with a text selection direction of AXTextSelectionDirectionDiscontiguous with no granularity, when normally if you pressed right-arrow it would get a direction of AXTextSelectionDirectionNext with a character granularity.

Chrome doesn't have this bug because it tries to infer the text intent in simple cases like when the cursor moves by a single character. I think we should do the same in WebKit. If we're concerned about doing it in general we can limit it to scenarios that we think are more likely to be this sort of custom accessible editor.

I attached a very simple repro that demonstrates the issue without needing to debug all of Monaco. It's just a textarea with JavaScript that intercepts all key presses and manually sets the selection.

[Radar WebKit Bug Importer]

<rdar://problem/124185098>

[Dominic Mazzoni]

Created attachment 470235 [details]
Patch

[Dominic Mazzoni]

Created attachment 470249 [details]
Patch

[Tyler Wilcock]

Comment on attachment 470249 [details]
Patch

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:506
> +AXTextStateChangeIntent AXObjectCache::inferDirectionFromIntent(AccessibilityObject* object, const AXTextStateChangeIntent& originalIntent, const VisibleSelection& selection)

Seems like we dereference `AccessibilityObject* object` without null-checking in this function. Is it possible to make this parameter a reference instead?

> LayoutTests/accessibility/mac/custom-text-editor.html:24
> +var AXTextStateChangeTypeSelectionMove = 2;
> +var AXTextStateChangeTypeSelectionExtend = AXTextStateChangeTypeSelectionMove + 1;
> +
> +var AXTextSelectionDirectionBeginning = 1;
> +var AXTextSelectionDirectionEnd = AXTextSelectionDirectionBeginning + 1;
> +var AXTextSelectionDirectionPrevious = AXTextSelectionDirectionEnd + 1;
> +var AXTextSelectionDirectionNext = AXTextSelectionDirectionPrevious + 1;
> +var AXTextSelectionDirectionDiscontiguous = AXTextSelectionDirectionNext + 1;
> +
> +var AXTextSelectionGranularityCharacter = 1;

Can these be const?

> LayoutTests/accessibility/mac/custom-text-editor.html:34
> +        let dir = userInfo["AXTextSelectionDirection"];

Can this be const?

> LayoutTests/accessibility/mac/custom-text-editor.html:43
> +        let granularity = userInfo["AXTextSelectionGranularity"];

Can this be const?

> LayoutTests/accessibility/mac/custom-text-editor.html:53
> +        if (resolveNotificationPromise) {
> +            resolveNotificationPromise();
> +        }

WebKit style typically avoids curly-braces for single-line control statements, there's a few above this one that can probably do without braces too.

https://webkit.org/code-style-guidelines/#braces-function

> LayoutTests/accessibility/mac/custom-text-editor.html:75
> +    shouldBe("addedNotification", "true");

I think the `output` style of tests (vs. using `shouldBe`) would use this instead: output += expect("addedNotification", "true");

[Dominic Mazzoni]

Created attachment 470251 [details]
Patch

[Tyler Wilcock]

Comment on attachment 470251 [details]
Patch

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:511
> +    AXTextStateChangeIntent intent = originalIntent;
> +
> +    if (!object.isTextControl() && !object.editableAncestor())
> +        return originalIntent;

AXTextStateChangeIntent intent = originalIntent;

This copy is unnecessary if condition !object.isTextControl() && !object.editableAncestor() is true. Does it work to move it below the if-statement?

> LayoutTests/accessibility/mac/custom-text-editor.html:40
> +        if (dir == AXTextSelectionDirectionNext) {
> +            str += " next";
> +        } else if (dir == AXTextSelectionDirectionPrevious) {
> +            str += " previous";
> +        } else if (dir == AXTextSelectionDirectionDiscontiguous) {
> +            str += " discontiguous";
> +        }

I think these braces are unnecessary too, but not a huge deal.

[Dominic Mazzoni]

Created attachment 470255 [details]
Patch

[Dominic Mazzoni]

(In reply to Tyler Wilcock from comment #6)
> Comment on attachment 470251 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=470251&action=review
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:511
> > +    AXTextStateChangeIntent intent = originalIntent;
> > +
> > +    if (!object.isTextControl() && !object.editableAncestor())
> > +        return originalIntent;
> 
> AXTextStateChangeIntent intent = originalIntent;
> 
> This copy is unnecessary if condition !object.isTextControl() &&
> !object.editableAncestor() is true. Does it work to move it below the
> if-statement?

Yep, sounds good.

> > LayoutTests/accessibility/mac/custom-text-editor.html:40
> > +        if (dir == AXTextSelectionDirectionNext) {
> > +            str += " next";
> > +        } else if (dir == AXTextSelectionDirectionPrevious) {
> > +            str += " previous";
> > +        } else if (dir == AXTextSelectionDirectionDiscontiguous) {
> > +            str += " discontiguous";
> > +        }
> 
> I think these braces are unnecessary too, but not a huge deal.

Sure! It's still so hard for me to get used to when so many other projects have the opposite guidance.

[Andres Gonzalez]

(In reply to Dominic Mazzoni from comment #7)
> Created attachment 470255 [details]
> Patch

From 57aa3d6b491aa1ce31b63fb71423a369783d6b5f Mon Sep 17 00:00:00 2001
From: Dominic Mazzoni <[e-mail]>
Date: Thu, 7 Mar 2024 13:33:00 -0800
Subject: [PATCH] AX: VoiceOver doesn't read characters when user presses left
 / right arrows in Monaco code editor
 https://bugs.webkit.org/show_bug.cgi?id=270616 rdar://123984168

Reviewed by NOBODY (OOPS!).

When the character or selection extent moves by just one visible position, infer
that it was a character granularity move, rather than a discontiguous selection.

* LayoutTests/accessibility/mac/custom-text-editor-expected.txt: Added.
* LayoutTests/accessibility/mac/custom-text-editor.html: Added.
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:
(WebCore::AXObjectCache::inferDirectionFromIntent):
(WebCore::AXObjectCache::postTextStateChangePlatformNotification):
---
 Source/WebCore/accessibility/AXObjectCache.h  |  7 ++
 .../accessibility/mac/AXObjectCacheMac.mm     | 42 ++++++++-
 .../mac/custom-text-editor-expected.txt       | 24 +++++
 .../accessibility/mac/custom-text-editor.html | 91 +++++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)
 create mode 100644 LayoutTests/accessibility/mac/custom-text-editor-expected.txt
 create mode 100644 LayoutTests/accessibility/mac/custom-text-editor.html

diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index 28386f2ca43f..342ec61dc3d8 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -778,6 +778,13 @@ private:
 #if USE(ATSPI)
     ListHashSet<RefPtr<AXCoreObject>> m_deferredParentChangedList;
 #endif
+
+#if PLATFORM(COCOA)
+    AXID m_lastTextFieldAXID;
+    VisibleSelection m_lastSelection;
+
+    AXTextStateChangeIntent inferDirectionFromIntent(AccessibilityObject&, const AXTextStateChangeIntent&, const VisibleSelection&);

AG: As a rule with exceptions, WebKit prefers to declare member functions first, and member variables last in the class declaration.
AG: this method is declare in COCOA but defined only in MAC. Do we need this in iOS as well?

+#endif
 };

 template<typename U>
diff --git a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
index 9cf57e4d0233..f0e437b59492 100644
--- a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
+++ b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
@@ -503,7 +503,45 @@ static void createIsolatedObjectIfNeeded(AccessibilityObject& object, std::optio
 }
 #endif

-void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject* object, const AXTextStateChangeIntent& intent, const VisibleSelection& selection)
+AXTextStateChangeIntent AXObjectCache::inferDirectionFromIntent(AccessibilityObject& object, const AXTextStateChangeIntent& originalIntent, const VisibleSelection& selection)
+{
+    if (!object.isTextControl() && !object.editableAncestor())
+        return originalIntent;
+
+    AXTextStateChangeIntent intent = originalIntent;

AG: auto intent;

+    if (intent.selection.direction == AXTextSelectionDirectionDiscontiguous && object.objectID() == m_lastTextFieldAXID && m_lastSelection != selection) {

AG: I think this whole block would be more readable as an early return, i.e., if (!condition) { update memeber vars and return early } continue with the processing. The early return block could be before the copy above.

+        if (m_lastSelection.isCaret() && selection.isCaret()) {
+            // Cursor movement
+            if (selection.visibleStart() == m_lastSelection.visibleStart().next(CannotCrossEditingBoundary)) {
+                intent.type = AXTextStateChangeTypeSelectionMove;
+                intent.selection.direction = AXTextSelectionDirectionNext;
+                intent.selection.granularity = AXTextSelectionGranularityCharacter;
+            } else if (selection.visibleStart() == m_lastSelection.visibleStart().previous(CannotCrossEditingBoundary)) {
+                intent.type = AXTextStateChangeTypeSelectionMove;
+                intent.selection.direction = AXTextSelectionDirectionPrevious;
+                intent.selection.granularity = AXTextSelectionGranularityCharacter;
+            }
+        } else if (selection.visibleBase() == m_lastSelection.visibleBase()) {
+            // Selection
+            if (selection.visibleExtent() == m_lastSelection.visibleExtent().next(CannotCrossEditingBoundary)) {
+                intent.type = AXTextStateChangeTypeSelectionExtend;
+                intent.selection.direction = AXTextSelectionDirectionNext;
+                intent.selection.granularity = AXTextSelectionGranularityCharacter;
+            } else if (selection.visibleExtent() == m_lastSelection.visibleExtent().previous(CannotCrossEditingBoundary)) {
+                intent.type = AXTextStateChangeTypeSelectionExtend;
+                intent.selection.direction = AXTextSelectionDirectionPrevious;
+                intent.selection.granularity = AXTextSelectionGranularityCharacter;
+            }
+        }
+    }
+
+    m_lastTextFieldAXID = object.objectID();
+    m_lastSelection = selection;
+
+    return intent;
+}
+
+void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject* object, const AXTextStateChangeIntent& originalIntent, const VisibleSelection& selection)
 {
     if (!object)
         object = rootWebArea();
@@ -516,6 +554,8 @@ void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject*
     processQueuedIsolatedNodeUpdates();
 #endif

+    AXTextStateChangeIntent intent = inferDirectionFromIntent(*object, originalIntent, selection);

AG: auto intent

+
     auto userInfo = adoptNS([[NSMutableDictionary alloc] initWithCapacity:5]);
     if (m_isSynchronizingSelection)
         [userInfo setObject:@YES forKey:NSAccessibilityTextStateSyncKey];

[Andres Gonzalez]

(In reply to Dominic Mazzoni from comment #7)
> Created attachment 470255 [details]
> Patch

diff --git a/LayoutTests/accessibility/mac/custom-text-editor.html b/LayoutTests/accessibility/mac/custom-text-editor.html
new file mode 100644
index 000000000000..0dd36c32cedb
--- /dev/null
+++ b/LayoutTests/accessibility/mac/custom-text-editor.html
@@ -0,0 +1,91 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<meta charset="utf-8">
+<script src="../../resources/accessibility-helper.js"></script>
+<script src="../../resources/js-test.js"></script>
+</head>
+<body>
+
+<textarea autofocus id="text" role="textbox">Text😀area with custom cursor handling</textarea>
+
+<script>
+var output = "Texts that we infer intents when JS is used to move the cursor in a text editor; this practice is common in online code editors.\n\n";

AG: Typo: Texts -> Tests

+
+const AXTextStateChangeTypeSelectionMove = 2;
+const AXTextStateChangeTypeSelectionExtend = AXTextStateChangeTypeSelectionMove + 1;
+
+const AXTextSelectionDirectionBeginning = 1;
+const AXTextSelectionDirectionEnd = AXTextSelectionDirectionBeginning + 1;
+const AXTextSelectionDirectionPrevious = AXTextSelectionDirectionEnd + 1;
+const AXTextSelectionDirectionNext = AXTextSelectionDirectionPrevious + 1;
+const AXTextSelectionDirectionDiscontiguous = AXTextSelectionDirectionNext + 1;
+
+const AXTextSelectionGranularityCharacter = 1;
+
+function notificationCallback(notification, userInfo) {
+    let str = "";
+    if (notification == "AXSelectedTextChanged") {

AG: early return.

[Dominic Mazzoni]

Created attachment 470292 [details]
Patch

[Dominic Mazzoni]

Created attachment 470299 [details]
Patch

[Dominic Mazzoni]

(In reply to Andres Gonzalez from comment #9)
> AG: I think this whole block would be more readable as an early return,
> i.e., if (!condition) { update memeber vars and return early } continue with
> the processing. The early return block could be before the copy above.

The only thing I don't love is duplicating the code to update the member vars. But I do like less indentation. Done.

All other feedback should be address now. Thanks.

[EWS]

Committed 275998@main (18d9a43b9d86): <https://commits.webkit.org/275998@main>

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