Bug 265952 – AX: insertionPointLineNumber hits the main thread even for single-line text fields

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

It seems like it should be a very safe optimization to always return a line number of 0 for single-line text fields.

[Radar WebKit Bug Importer]

<rdar://problem/119264904>

[Dominic Mazzoni]

Created attachment 468915 [details]
Patch

[chris fleizach]

Comment on attachment 468915 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:382
> +    if (auto *renderer = object.renderer(); renderer->isRenderTextControl())

auto*

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:87
> +    bool computeIsSingleLineTextField(AccessibilityObject&, bool isNonNativeTextControl);

compute verb seems unnecessary. we could just name

isSingleLineTExtField

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 9bcd149f3594..5a6835b38f32 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -168,7 +168,6 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
     setProperty(AXPropertyName::IsKeyboardFocusable, object.isKeyboardFocusable());
     setProperty(AXPropertyName::BrailleRoleDescription, object.brailleRoleDescription().isolatedCopy());
     setProperty(AXPropertyName::BrailleLabel, object.brailleLabel().isolatedCopy());
-    setProperty(AXPropertyName::IsNonNativeTextControl, object.isNonNativeTextControl());

     RefPtr geometryManager = tree()->geometryManager();
     std::optional frame = geometryManager ? geometryManager->cachedRectForID(object.objectID()) : std::nullopt;
@@ -352,6 +351,10 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
         auto range = object.textInputMarkedTextMarkerRange();
         if (auto characterRange = range.characterRange(); range && characterRange)
             setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, std::pair<AXID, CharacterRange>(range.start().objectID(), *characterRange));
+
+        bool isNonNativeTextControl = object.isNonNativeTextControl();
+        setProperty(AXPropertyName::IsNonNativeTextControl, isNonNativeTextControl);
+        setProperty(AXPropertyName::IsSingleLineTextField, computeIsSingleLineTextField(object, isNonNativeTextControl));
     }

     // These properties are only needed on the AXCoreObject interface due to their use in ATSPI,
@@ -371,6 +374,18 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
     initializePlatformProperties(axObject);
 }

+bool AXIsolatedObject::computeIsSingleLineTextField(AccessibilityObject& object, bool isNonNativeTextControl)
+{
+    if (isNonNativeTextControl)
+        return object.hasAttribute(aria_multilineAttr) && !object.ariaIsMultiline();
+
+    if (auto *renderer = object.renderer(); renderer->isRenderTextControl())

AG: style: auto *renderer should be auto* renderer
AG: need to check renderer for null. This is why I don't like this compound if statement syntax, if you had the expression to the left of the ;, that would be a check for null. But since you have a second expression, the first one is just initialization and the second is the actual check.

+        return !renderer->isRenderTextControlMultiLine();
+
+    // If we're not sure, return false, it means we can't use this as an optimization to avoid computing the line index.
+    return false;
+}

AG: it would be clearer to have this method return an enum, something like enum class : uint_8 { Unknown, SingleLine, MultiLine }.

+
 AccessibilityObject* AXIsolatedObject::associatedAXObject() const
 {
     ASSERT(isMainThread());
@@ -1284,6 +1299,9 @@ bool AXIsolatedObject::isNativeTextControl() const

 int AXIsolatedObject::insertionPointLineNumber() const
 {
+    if (boolAttributeValue(AXPropertyName::IsSingleLineTextField))
+        return 0;
+
     return Accessibility::retrieveValueFromMainThread<int>([this] () -> int {
         if (auto* axObject = associatedAXObject())
             return axObject->insertionPointLineNumber();

[Dominic Mazzoni]

Created attachment 468924 [details]
Patch

[Dominic Mazzoni]

Fixed auto*, renamed to isSingleLineTextField, and got rid of the compound if statement.

> AG: it would be clearer to have this method return an enum, something like
> enum class : uint_8 { Unknown, SingleLine, MultiLine }.

I'm not a fan of computing things that aren't actually used. Is this okay as-is or maybe would there be a way to rename the function?

Interestingly, it looks like there are no Mac accessibility APIs that expose multiline directly. On Windows and Linux text fields have a native multiline accessibility attribute.

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index e5b3a6bf0502..f2d3e093d9a0 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -84,6 +84,8 @@ private:
     void setObjectProperty(AXPropertyName, AXCoreObject*);
     void setObjectVectorProperty(AXPropertyName, const AccessibilityChildrenVector&);

+    bool isSingleLineTextField(AccessibilityObject&, bool isNonNativeTextControl);

AG: static method.

+
     // FIXME: consolidate all AttributeValue retrieval in a single template method.
     bool boolAttributeValue(AXPropertyName) const;
     String stringAttributeValue(AXPropertyName) const;

[Andres Gonzalez]

(In reply to Dominic Mazzoni from comment #6)
> Fixed auto*, renamed to isSingleLineTextField, and got rid of the compound
> if statement.
> 
> > AG: it would be clearer to have this method return an enum, something like
> > enum class : uint_8 { Unknown, SingleLine, MultiLine }.
> 
> I'm not a fan of computing things that aren't actually used. Is this okay
> as-is or maybe would there be a way to rename the function?

AG: maybe canBeMultiline() ? And then check for the negation in the optimization.

> 
> Interestingly, it looks like there are no Mac accessibility APIs that expose
> multiline directly. On Windows and Linux text fields have a native multiline
> accessibility attribute.

Yes, that's quite an omission that has a user experience impact.

[Dominic Mazzoni]

Created attachment 468930 [details]
Patch

[Dominic Mazzoni]

Created attachment 468931 [details]
Patch

[Dominic Mazzoni]

OK, made the method static and I renamed both the method and attribute to CanBeMultilineTextField for consistency, flipping the logic.

I'll look into what VoiceOver could do better for single-line vs multi-line text fields if we had an API for it.

[Dominic Mazzoni]

Created attachment 468946 [details]
Patch

[EWS]

Committed 271769@main (f771811335e6): <https://commits.webkit.org/271769@main>

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