AX: WebKit should not expose redundant AXGroups with missing role whe…

…n the label is the same as the contents https://bugs.webkit.org/show_bug.cgi?id=169924 Patch by Tyler Wilcock <tyler_w@apple.com> on 2021-10-17 Reviewed by Chris Fleizach. Source/WebCore: Don't expose groups with redundant accessibility text on the Mac. Here's an example of one such group: <div aria-label="1">1</div> This group is not useful to accessibility clients. Instead, we expose the text contents directly. Test: accessibility/ignore-redundant-accessibility-text-groups.html * accessibility/AccessibilityRenderObject.cpp: (WebCore::AccessibilityRenderObject::textUnderElement const): Instead of asserting when we try to get the text of an element in a document that needs a style update, return an empty string. * accessibility/mac/AccessibilityObjectMac.mm: (WebCore::shouldIgnoreGroup): Added. (WebCore::AccessibilityObject::accessibilityPlatformIncludesObject const): LayoutTests: Add a test ensuring WebKit appropriately does and doesn't expose groups with redundant accessibility text. * accessibility/mac/ignore-redundant-accessibility-text-groups-expected.txt: Added. * accessibility/mac/ignore-redundant-accessibility-text-groups.html: Added. Canonical link: https://commits.webkit.org/243130@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@284335 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Oct 17, 2021 · 977f75a79bc71147cd9a434241072b720096320d · 977f75a
1 parent b411e0c
commit 977f75a79bc71147cd9a434241072b720096320d
Show file tree

Hide file tree

Showing 6 changed files with 198 additions and 4 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2021-10-17  Tyler Wilcock  <tyler_w@apple.com>
+
+        AX: WebKit should not expose redundant AXGroups with missing role when the label is the same as the contents
+        https://bugs.webkit.org/show_bug.cgi?id=169924
+
+        Reviewed by Chris Fleizach.
+
+        Add a test ensuring WebKit appropriately does and doesn't expose
+        groups with redundant accessibility text.
+
+        * accessibility/mac/ignore-redundant-accessibility-text-groups-expected.txt: Added.
+        * accessibility/mac/ignore-redundant-accessibility-text-groups.html: Added.
+
 2021-10-17  Antti Koivisto  <antti@apple.com>
 
         [LFC][Integration] Support background-clip:text on inline boxes

diff --git a/LayoutTests/accessibility/mac/ignore-redundant-accessibility-text-groups-expected.txt b/LayoutTests/accessibility/mac/ignore-redundant-accessibility-text-groups-expected.txt
@@ -0,0 +1,30 @@
+This test ensures WebKit ignores groups with redundant accessibility text.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS !ariaLabelGroup is true
+PASS !titleGroup is true
+PASS !ariaLabelDiv is true
+PASS !titleDiv is true
+PASS typeof clickHandlerGroup is 'object'
+PASS typeof clickHandlerDiv is 'object'
+PASS resultElement.role is 'AXRole: AXStaticText'
+PASS resultElement.stringValue is 'AXValue: Blue cheese'
+PASS resultElement.role is 'AXRole: AXStaticText'
+PASS resultElement.stringValue is 'AXValue: Oranges'
+PASS contentContainer.childrenCount is 6
+PASS contentContainer.childAtIndex(0).stringValue is 'AXValue: Blue cheese'
+PASS contentContainer.childAtIndex(0).role is 'AXRole: AXStaticText'
+PASS contentContainer.childAtIndex(1).stringValue is 'AXValue: Oranges'
+PASS contentContainer.childAtIndex(1).role is 'AXRole: AXStaticText'
+PASS contentContainer.childAtIndex(2).role is 'AXRole: AXGroup'
+PASS contentContainer.childAtIndex(3).stringValue is 'AXValue: Jello'
+PASS contentContainer.childAtIndex(3).role is 'AXRole: AXStaticText'
+PASS contentContainer.childAtIndex(4).stringValue is 'AXValue: Broccoli'
+PASS contentContainer.childAtIndex(4).role is 'AXRole: AXStaticText'
+PASS contentContainer.childAtIndex(5).role is 'AXRole: AXGroup'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/mac/ignore-redundant-accessibility-text-groups.html b/LayoutTests/accessibility/mac/ignore-redundant-accessibility-text-groups.html
@@ -0,0 +1,94 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test.js"></script>
+<script src="../../resources/accessibility-helper.js"></script>
+</head>
+<body>
+
+<div id="content" role="group">
+    <!-- First, test role="group" elements. -->
+    <div id="aria-label-group" role="group" aria-label="Blue cheese">
+        Blue cheese
+    </div>
+
+    <div id="title-group" role="group" title="Oranges">
+        Oranges
+    </div>
+
+    <div id="click-handler-group" role="group" aria-label="Group click handler" onclick="emptyClickHandler()">
+        Group click handler
+    </div>
+
+    <!-- Also test role-less generic divs. -->
+    <div id="aria-label-div" aria-label="Jello">
+        Jello
+    </div>
+
+    <div id="title-div" title="Broccoli">
+        Broccoli
+    </div>
+
+    <div id="click-handler-div" aria-label="Div click handler" onclick="emptyClickHandler()">
+        Div click handler
+    </div>
+</div>
+
+<script>
+    description("This test ensures WebKit ignores groups with redundant accessibility text.");
+    const emptyClickHandler = () => {};
+
+    if (window.accessibilityController) {
+
+        var contentContainer = accessibilityController.accessibleElementById("content");
+
+        var ariaLabelGroup = accessibilityController.accessibleElementById("aria-label-group");
+        var clickHandlerGroup = accessibilityController.accessibleElementById("click-handler-group");
+        var titleGroup = accessibilityController.accessibleElementById("title-group");
+
+        var ariaLabelDiv = accessibilityController.accessibleElementById("aria-label-div");
+        var clickHandlerDiv = accessibilityController.accessibleElementById("click-handler-div");
+        var titleDiv = accessibilityController.accessibleElementById("title-div");
+
+        // We shouldn't be able to get an accessible element for these groups because they should be ignored.
+        shouldBeTrue("!ariaLabelGroup");
+        shouldBeTrue("!titleGroup");
+        shouldBeTrue("!ariaLabelDiv");
+        shouldBeTrue("!titleDiv");
+        // But any group with an event handler should always be exposed.
+        shouldBe("typeof clickHandlerGroup", "'object'");
+        shouldBe("typeof clickHandlerDiv", "'object'");
+
+        // Ensure we can search for the text within the ignored groups.
+        var resultElement = contentContainer.uiElementForSearchPredicate(contentContainer, true, "AXAnyTypeSearchKey", "", false);
+        shouldBe("resultElement.role", "'AXRole: AXStaticText'");
+        shouldBe("resultElement.stringValue", "'AXValue: Blue cheese'");
+
+        resultElement = contentContainer.uiElementForSearchPredicate(resultElement, true, "AXAnyTypeSearchKey", "", false);
+        shouldBe("resultElement.role", "'AXRole: AXStaticText'");
+        shouldBe("resultElement.stringValue", "'AXValue: Oranges'");
+
+        // Ensure the only accessible content exposed via `children` is the text elements and event handler groups.
+        shouldBe("contentContainer.childrenCount", "6");
+        shouldBe("contentContainer.childAtIndex(0).stringValue", "'AXValue: Blue cheese'");
+        shouldBe("contentContainer.childAtIndex(0).role", "'AXRole: AXStaticText'");
+
+        shouldBe("contentContainer.childAtIndex(1).stringValue", "'AXValue: Oranges'");
+        shouldBe("contentContainer.childAtIndex(1).role", "'AXRole: AXStaticText'");
+
+        shouldBe("contentContainer.childAtIndex(2).role", "'AXRole: AXGroup'");
+
+        shouldBe("contentContainer.childAtIndex(3).stringValue", "'AXValue: Jello'");
+        shouldBe("contentContainer.childAtIndex(3).role", "'AXRole: AXStaticText'");
+
+        shouldBe("contentContainer.childAtIndex(4).stringValue", "'AXValue: Broccoli'");
+        shouldBe("contentContainer.childAtIndex(4).role", "'AXRole: AXStaticText'");
+
+        shouldBe("contentContainer.childAtIndex(5).role", "'AXRole: AXGroup'");
+
+        document.getElementById("content").style.visibility = "hidden";
+    }
+</script>
+</body>
+</html>
+
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,29 @@
+2021-10-17  Tyler Wilcock  <tyler_w@apple.com>
+
+        AX: WebKit should not expose redundant AXGroups with missing role when the label is the same as the contents
+        https://bugs.webkit.org/show_bug.cgi?id=169924
+
+        Reviewed by Chris Fleizach.
+
+        Don't expose groups with redundant accessibility text on the Mac.
+        Here's an example of one such group:
+
+        <div aria-label="1">1</div>
+
+        This group is not useful to accessibility clients. Instead, we
+        expose the text contents directly.
+
+        Test: accessibility/ignore-redundant-accessibility-text-groups.html
+
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::textUnderElement const):
+        Instead of asserting when we try to get the text of an element in a
+        document that needs a style update, return an empty string.
+
+        * accessibility/mac/AccessibilityObjectMac.mm:
+        (WebCore::shouldIgnoreGroup): Added.
+        (WebCore::AccessibilityObject::accessibilityPlatformIncludesObject const):
+
 2021-10-17  Antti Koivisto  <antti@apple.com>
 
         [LFC][Integration] Support background-clip:text on inline boxes

diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -680,12 +680,14 @@ String AccessibilityRenderObject::textUnderElement(AccessibilityTextUnderElement
             if (Frame* frame = nodeDocument->frame()) {
                 // catch stale WebCoreAXObject (see <rdar://problem/3960196>)
                 if (frame->document() != nodeDocument)
-                    return String();
+                    return { };
 
-                // Renders referenced by accessibility objects could get destroyed, if TextIterator ends up triggering
-                // style update/layout here. See also AXObjectCache::deferTextChangedIfNeeded().
-                ASSERT_WITH_SECURITY_IMPLICATION(!nodeDocument->childNeedsStyleRecalc());
+                // Renderers referenced by accessibility objects could get destroyed if TextIterator ends up triggering
+                // a style update or layout here. See also AXObjectCache::deferTextChangedIfNeeded().
+                if (nodeDocument->childNeedsStyleRecalc())
+                    return { };
                 ASSERT_WITH_SECURITY_IMPLICATION(!nodeDocument->view()->layoutContext().isInRenderTreeLayout());
+
                 return plainText(*textRange, textIteratorBehaviorForTextRange());
             }
         }

diff --git a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm b/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm
@@ -114,6 +114,32 @@
     return true;
 }
 
+static bool shouldIgnoreGroup(const AccessibilityObject& axObject)
+{
+    if (!axObject.isGroup() && axObject.roleValue() != AccessibilityRole::Div)
+        return false;
+
+    // Never ignore a <div> with event listeners attached to it (e.g. onclick).
+    if (axObject.node() && axObject.node()->hasEventListeners())
+        return false;
+
+    auto* first = axObject.firstChild();
+    if (first && first == axObject.lastChild() && first->roleValue() == AccessibilityRole::StaticText) {
+        auto childString = first->stringValue();
+        // stringValue() can be null if the underlying document needs style recalculation.
+        if (!childString.isNull()) {
+            Vector<AccessibilityText> axText;
+            axObject.accessibilityText(axText);
+            // Don't expose <div>s whose only child is text that has the same content as the <div>s accessibility text.
+            // Instead, we should expose the text element directly.
+            auto firstText = axText.size() ? axText[0].text : String();
+            if (firstText == childString)
+                return true;
+        }
+    }
+    return false;
+}
+
 AccessibilityObjectInclusion AccessibilityObject::accessibilityPlatformIncludesObject() const
 {
     if (isMenuListPopup() || isMenuListOption())
@@ -142,6 +168,9 @@
         }
     }
 
+    if (shouldIgnoreGroup(*this))
+        return AccessibilityObjectInclusion::IgnoreObject;
+
     return AccessibilityObjectInclusion::DefaultBehavior;
 }