It seems like it should be a very safe optimization to always return a line number of 0 for single-line text fields.
<rdar://problem/119264904>
Created attachment 468915 [details] Patch
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
(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();
Created attachment 468924 [details] Patch
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.
(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;
(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.
Created attachment 468930 [details] Patch
Created attachment 468931 [details] Patch
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.
Created attachment 468946 [details] Patch
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].