e.g., for <button>foo<span></span>bar</button>, we will generate accname "foo bar", but it should be "foobar".
<rdar://problem/125935544>
Created attachment 470760 [details] Patch
Created attachment 470770 [details] Patch
(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
Created attachment 470774 [details] Patch
(In reply to Andres Gonzalez from comment #4) > (In reply to Tyler Wilcock from comment #3) All comments addressed, thanks!
Created attachment 470775 [details] Patch
(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;
> - 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.
Created attachment 470842 [details] Patch
Found 2 new test failures: fast/workers/dedicated-worker-lifecycle.html, inspector/cpu-profiler/tracking.html
Found 1 new test failure: inspector/cpu-profiler/tracking.html
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].