Bug 272194 – AX: When constructing accessibility text, tokens should not unconditionally be separated by a space

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

e.g., for <button>foo<span></span>bar</button>, we will generate accname "foo bar", but it should be "foobar".

[Radar WebKit Bug Importer]

<rdar://problem/125935544>

[Tyler Wilcock]

Created attachment 470760 [details]
Patch

[Tyler Wilcock]

Created attachment 470770 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
index 6cbd30cc0a86..8d305ff6dda9 100644
--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
@@ -85,6 +85,7 @@
 #include "UserGestureIndicator.h"
 #include "VisibleUnits.h"
 #include <wtf/Scope.h>
+#include <wtf/SetForScope.h>
 #include <wtf/StdLibExtras.h>
 #include <wtf/text/StringBuilder.h>
 #include <wtf/unicode/CharacterNames.h>
@@ -94,7 +95,7 @@ namespace WebCore {
 using namespace HTMLNames;

 static String accessibleNameForNode(Node&, Node* labelledbyNode = nullptr);
-static void appendNameToStringBuilder(StringBuilder&, String&&);
+static void appendNameToStringBuilder(StringBuilder&, String&&, bool prependSpace = true);

 AccessibilityNodeObject::AccessibilityNodeObject(Node* node)
     : AccessibilityObject()
@@ -2249,16 +2250,56 @@ static bool shouldUseAccessibilityObjectInnerText(AccessibilityObject* obj, Text
     return true;
 }

-static void appendNameToStringBuilder(StringBuilder& builder, String&& text)
+static void appendNameToStringBuilder(StringBuilder& builder, String&& text, bool prependSpace)
 {
     if (text.isEmpty())
         return;

-    if (!isHTMLLineBreak(text[0]) && builder.length() && !isHTMLLineBreak(builder[builder.length() - 1]))
+    if (prependSpace && !isHTMLLineBreak(text[0]) && builder.length() && !isHTMLLineBreak(builder[builder.length() - 1]))
         builder.append(' ');
     builder.append(WTFMove(text));
 }

+
+static bool displayTypeNeedsSpace(DisplayType type)
+{
+    return type == DisplayType::Block
+        || type == DisplayType::InlineBlock
+        || type == DisplayType::InlineFlex
+        || type == DisplayType::InlineGrid
+        || type == DisplayType::InlineTable
+        || type == DisplayType::TableCell;
+}
+
+static bool needsSpaceFromDisplay(AXCoreObject& coreObject)
+{
+    // We should always be dealing with non-isolated objects here. Ideally in the future we can strengthen the types
+    // to make this issue impossible.
+    ASSERT(is<AccessibilityObject>(coreObject));
+    auto* axObject = dynamicDowncast<AccessibilityObject>(coreObject);
+
+    CheckedPtr renderer = axObject ? axObject->renderer() : nullptr;
+    if (is<RenderText>(renderer.get())) {
+        // Never add a space for RenderTexts. They are inherently inline, but take their parent's style, which may
+        // be block, erroneously adding a space.
+        return false;
+    }
+
+    const auto* style = renderer ? &renderer->style() : nullptr;
+    if (!style)
+        style = axObject ? axObject->style() : nullptr;

AG: instead of axObject ? more than once, maybe you can do an early return if it is null, which shouldn't be from the comment at the top of the function. Maybe a RELEASE_ASSERT instead of ASSERT?

+
+    return style ? displayTypeNeedsSpace(style->display()) : false;
+}
+
+static bool shouldPrependSpace(AXCoreObject& currentObject, AXCoreObject* previousObject)
+{
+    return needsSpaceFromDisplay(currentObject)
+        || (previousObject && needsSpaceFromDisplay(*previousObject))
+        || currentObject.isControl()
+        || (previousObject && previousObject->isControl());

AG: currentObject -> just object.

+}
+
 String AccessibilityNodeObject::textUnderElement(TextUnderElementMode mode) const
 {
     auto isAriaVisible = [this] () {
@@ -2294,26 +2335,39 @@ String AccessibilityNodeObject::textUnderElement(TextUnderElementMode mode) cons
     }

     StringBuilder builder;
-    auto appendTextUnderElement = [&] (const RefPtr<AXCoreObject>& object) {
-        auto childText = object->textUnderElement(mode);
-        if (childText.length())
-            appendNameToStringBuilder(builder, WTFMove(childText));
+    RefPtr<AXCoreObject> previous;
+    bool previousRequiresSpace = false;
+    auto appendTextUnderElement = [&] (AXCoreObject& object) {
+        // We don't want to trim whitespace in these intermediate calls to textUnderElement, as doing so will wipe out
+        // spaces we need to build the string properly. If anything (depending on the original `mode`), we will trim
+        // whitespace at the very end.
+        SetForScope resetModeTrim(mode.trimWhitespace, TrimWhitespace::No);
+
+        auto childText = object.textUnderElement(mode);
+        if (childText.length()) {
+            appendNameToStringBuilder(builder, WTFMove(childText), previousRequiresSpace || shouldPrependSpace(object, previous.get()));
+            previousRequiresSpace = false;
+        }
     };

-    for (RefPtr child = firstChild(); child; child = child->nextSibling()) {
+    for (RefPtr child = firstChild(); child; previous = child, child = child->nextSibling()) {
         if (mode.ignoredChildNode && child->node() == mode.ignoredChildNode)
             continue;

         if (mode.isHidden()) {
             // If we are within a hidden context, don't add any text for this node. Instead, fan out downwards
             // to search for un-hidden nodes (e.g. visibility:visible nodes within a visibility:hidden ancestor).
-            appendTextUnderElement(child);
+            appendTextUnderElement(*child);
             continue;
         }

         bool shouldDeriveNameFromAuthor = (mode.childrenInclusion == TextUnderElementMode::Children::IncludeNameFromContentsChildren && !child->accessibleNameDerivesFromContent());
         if (shouldDeriveNameFromAuthor) {
-            appendNameToStringBuilder(builder, accessibleNameForNode(*child->node()));
+            auto nameForNode = accessibleNameForNode(*child->node());
+            bool nameIsEmpty = nameForNode.isEmpty();
+            appendNameToStringBuilder(builder, WTFMove(nameForNode));
+            // Separate author-provided text with a space.
+            previousRequiresSpace = previousRequiresSpace || !nameIsEmpty;
             continue;
         }

@@ -2332,6 +2386,8 @@ String AccessibilityNodeObject::textUnderElement(TextUnderElementMode mode) cons
             accessibilityNodeObject->alternativeText(textOrder);
             if (textOrder.size() > 0 && textOrder[0].text.length()) {
                 appendNameToStringBuilder(builder, WTFMove(textOrder[0].text));
+                // Alternative text (e.g. from aria-label, aria-labelledby, alt, etc) requires space separation.
+                previousRequiresSpace = true;
                 continue;
             }
         }
@@ -2347,10 +2403,13 @@ String AccessibilityNodeObject::textUnderElement(TextUnderElementMode mode) cons
                 continue;
         }

-        appendTextUnderElement(child);
+        appendTextUnderElement(*child);
     }

-    return builder.toString().trim(isUnicodeWhitespace).simplifyWhiteSpace(isHTMLSpaceButNotLineBreak);
+    auto result = builder.toString();
+    return mode.trimWhitespace == TrimWhitespace::Yes
+        ? result.trim(isUnicodeWhitespace).simplifyWhiteSpace(isHTMLSpaceButNotLineBreak)
+        : result;
 }

 String AccessibilityNodeObject::title() const

[Tyler Wilcock]

Created attachment 470774 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #3)
All comments addressed, thanks!

[Tyler Wilcock]

Created attachment 470775 [details]
Patch

[Andres Gonzalez]

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

diff --git a/LayoutTests/accessibility/ios-simulator/link-with-images-text-expected.txt b/LayoutTests/accessibility/ios-simulator/link-with-images-text-expected.txt
index 095262b2525f..4fb9de8d22a6 100644
--- a/LayoutTests/accessibility/ios-simulator/link-with-images-text-expected.txt
+++ b/LayoutTests/accessibility/ios-simulator/link-with-images-text-expected.txt
@@ -5,7 +5,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE


 PASS obj.isIgnored is false
-PASS obj.description is 'AXLabel: iPhone From $99'
+PASS obj.description is 'AXLabel: iPhoneFrom $99'

AG: is this expectation correct? The words 'iPhone" and "From" are run together.

 PASS successfullyParsed is true

 TEST COMPLETE
diff --git a/LayoutTests/accessibility/ios-simulator/link-with-images-text.html b/LayoutTests/accessibility/ios-simulator/link-with-images-text.html
index 66e405f81918..a8914f477404 100644
--- a/LayoutTests/accessibility/ios-simulator/link-with-images-text.html
+++ b/LayoutTests/accessibility/ios-simulator/link-with-images-text.html
@@ -27,7 +27,7 @@ if (window.testRunner)

         var obj = accessibilityController.accessibleElementById("link");
         shouldBe("obj.isIgnored", "false");
-        shouldBe("obj.description", "'AXLabel: iPhone From $99'");
+        shouldBe("obj.description", "'AXLabel: iPhoneFrom $99'");

AG: is this expectation correct? The words 'iPhone" and "From" are run together.

     }

     successfullyParsed = true;

[Tyler Wilcock]

> -        shouldBe("obj.description", "'AXLabel: iPhone From $99'");
> +        shouldBe("obj.description", "'AXLabel: iPhoneFrom $99'");
> 
> AG: is this expectation correct? The words 'iPhone" and "From" are run
> together.
TW: Yes, it's correct. This now matches the visual rendering. Based on the HTML in the test, we should not have been separating these strings with a space, but were.

[Tyler Wilcock]

Created attachment 470842 [details]
Patch

[EWS]

Found 2 new test failures: fast/workers/dedicated-worker-lifecycle.html, inspector/cpu-profiler/tracking.html

[EWS]

Found 1 new test failure: inspector/cpu-profiler/tracking.html

[EWS]

Committed 277332@main (15650dca0c80): <https://commits.webkit.org/277332@main>

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