Bug 169924 – AX: WebKit should not expose redundant AXGroups with missing role when the label is the same as the contents

• Component: Accessibility
• Status: RESOLVED FIXED

[James Craig]

AX: WebKit should not expose redundant AXGroups with missing role when the label is the same as the contents... Unless there are click/key event handlers or some other heuristic indicator of meaning.

VoiceOver users on staff found the following markup pattern problematic:

<div aria-label="1">1</div>
<div aria-label="2">2</div>
<div aria-label="3">3</div>

By default, WebKit exposes layout blocks as an AXGroup, and the aria-label as the AXTitle. This means users with bookended groups redundantly hear:

"1 group, 1, end of group"
"2 group, 3, end of group"
"3 group, 3, end of group"

Yes, it's an authoring problem, but it happens enough that I think we should work around it.

Proposal: On generic elements (GroupRole, not ApplicationGroupRole):
- If there is no explicit role, (With bug 169810, ARIA grouping roles will be WebCore ApplicationGroupRole, not WebCore GroupRole.)
- and if there is no click or key-down/up/press handler (nothing to make the AXGroup actionable)
- and if the content string matches the computed @aria-labelledby or @aria-label string...

Expose only the generic view: AXGroup with no AXTitle. VoiceOver will read the contents.

[Radar WebKit Bug Importer]

<rdar://problem/31221183>

[Tyler Wilcock]

Created attachment 441161 [details]
Patch

[Tyler Wilcock]

Created attachment 441171 [details]
Patch

[Tyler Wilcock]

Looks like this introduced a few crashes by trying to get the `stringValue` of nodes within documents that are marked for update.

// 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());

If we can't accurately determine the text because the document needs an update, we can return an empty string instead of asserting. We can't `updateBackingStore` before executing this codepath because we're calling from `accessibilityPlatformIncludesObject` (and other "is ignored" methods upstream) that are all const.

Stacktrace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000013840aef0 WTFCrash + 20 (Assertions.cpp:321)
1   com.apple.JavaScriptCore      	0x000000013840af04 WTFCrashWithSecurityImplication + 12 (Assertions.cpp:342)
2   com.apple.WebCore             	0x00000001177a5ac4 WebCore::AccessibilityRenderObject::textUnderElement(WebCore::AccessibilityTextUnderElementMode) const + 776
3   com.apple.WebCore             	0x00000001177a645c WebCore::AccessibilityRenderObject::stringValue() const + 428
4   com.apple.WebCore             	0x000000011563a4bc WebCore::isIgnorableGroup(WebCore::AccessibilityObject const&) + 416
5   com.apple.WebCore             	0x000000011563a298 WebCore::AccessibilityObject::accessibilityPlatformIncludesObject() const + 472
6   com.apple.WebCore             	0x00000001177a081c WebCore::AccessibilityObject::defaultObjectInclusion() const + 300
7   com.apple.WebCore             	0x00000001177a90bc WebCore::AccessibilityRenderObject::defaultObjectInclusion() const + 172
8   com.apple.WebCore             	0x00000001177a91b0 WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const + 224
9   com.apple.WebCore             	0x00000001177a0944 WebCore::AccessibilityObject::accessibilityIsIgnored() const + 228
10  com.apple.WebCore             	0x00000001177d1fc4 WebCore::AccessibilityObject::parentObjectUnignored() const::$_2::operator()(WebCore::AccessibilityObject const&) const + 40

[Tyler Wilcock]

Created attachment 441217 [details]
Patch

[Andres Gonzalez]

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

--- a/Source/WebCore/ChangeLog
+++ a/Source/WebCore/ChangeLog

+        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()` instead.

Two insteads.

--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp

+                if (nodeDocument->childNeedsStyleRecalc())
+                    return String();

return { };

--- a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm
+++ a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm

+static bool isIgnorableGroup(const AccessibilityObject& axObject)

isIgnorableGroup -> shouldIgnoreGroup

+    // Never ignore a <div> with event listeners attached to it (e.g. onclick).
+    if (axObject.node() && axObject.node()->hasEventListeners())
+        return false;

Isn't this check done for any object already in the isIgnored method? Wondering why we need to do this specifically for groups.

+    auto* first = axObject.firstChild();
+    if (first && first == axObject.lastChild() && first->roleValue() == AccessibilityRole::StaticText) {

Wouldn't this be more efficient?

+    auto& children = axObject.children();
+    if (children.size() == 1 && children[0]->roleValue() == AccessibilityRole::StaticText) {

or the problem is that this is used in children()?

+        Vector<AccessibilityText> text;
...
+        auto axText = text.size() ? text[0].text : String();

Aren't these names inverted?  i.e., shouldn't the String be text and the vector axText?

What if the vector has more than one element? Perhaps we should return false in that case.

[Andres Gonzalez]

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

+        // 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(1).stringValue", "'AXValue: Oranges'");
+        shouldBe("contentContainer.childAtIndex(2).role", "'AXRole: AXGroup'");
+        shouldBe("contentContainer.childAtIndex(3).stringValue", "'AXValue: Jello'");
+        shouldBe("contentContainer.childAtIndex(4).stringValue", "'AXValue: Broccoli'");
+        shouldBe("contentContainer.childAtIndex(5).role", "'AXRole: AXGroup'");

Perhaps we should check the role of the static text objects as well.

[Tyler Wilcock]

> +                if (nodeDocument->childNeedsStyleRecalc())
> +                    return String();
>
> return { };

There is code two lines above that also does `return String();`. I can change both if that's the style preference here.

> +    // Never ignore a <div> with event listeners attached to it (e.g.
> onclick).
> +    if (axObject.node() && axObject.node()->hasEventListeners())
> +        return false;
> 
> Isn't this check done for any object already in the isIgnored method?
> Wondering why we need to do this specifically for groups.

If it is, I can't find it. It probably should exist everywhere?

Regardless, I think we'd still need this check to avoid returning a false positive for a group with redundant AX text and an event handler.

> +    auto* first = axObject.firstChild();
> +    if (first && first == axObject.lastChild() && first->roleValue() ==
> AccessibilityRole::StaticText) {
> 
> Wouldn't this be more efficient?
> 
> +    auto& children = axObject.children();
> +    if (children.size() == 1 && children[0]->roleValue() ==
> AccessibilityRole::StaticText) {
> 
> or the problem is that this is used in children()?

We can't call children() here because children() is not const (it updates the children if required). Making any "is ignored" function non-const will make many other things have to be non-const as well.

This is generally unfortunate, because it makes ignoring things based on their contents much more difficult. We'll have to comb through the AX objects represented by the raw layout tree ourselves. It works fine in this simple example, but more complex analyses will be harder and fragile.

> +        Vector<AccessibilityText> text;
> ...
> +        auto axText = text.size() ? text[0].text : String();
> 
> Aren't these names inverted?  i.e., shouldn't the String be text and the
> vector axText?
> 
> What if the vector has more than one element? Perhaps we should return false
> in that case.

My understanding of accessibilityText is that it's ordered in terms of the most applicable text based on https://www.w3.org/TR/accname-1.2/#step1. So by picking the first one, we are choosing the one AX clients should be using when describing the element. Web Inspector also picks the first accessibilityText.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #8)
> > +                if (nodeDocument->childNeedsStyleRecalc())
> > +                    return String();
> >
> > return { };
> 
> There is code two lines above that also does `return String();`. I can
> change both if that's the style preference here.

Yes, feel free to change the other two. We don't do mass style changes but if you are adding new code or touching a function, you may just fix it up.

> 
> > +    // Never ignore a <div> with event listeners attached to it (e.g.
> > onclick).
> > +    if (axObject.node() && axObject.node()->hasEventListeners())
> > +        return false;
> > 
> > Isn't this check done for any object already in the isIgnored method?
> > Wondering why we need to do this specifically for groups.
> 
> If it is, I can't find it. It probably should exist everywhere?
> 
> Regardless, I think we'd still need this check to avoid returning a false
> positive for a group with redundant AX text and an event handler.
> 
> > +    auto* first = axObject.firstChild();
> > +    if (first && first == axObject.lastChild() && first->roleValue() ==
> > AccessibilityRole::StaticText) {
> > 
> > Wouldn't this be more efficient?
> > 
> > +    auto& children = axObject.children();
> > +    if (children.size() == 1 && children[0]->roleValue() ==
> > AccessibilityRole::StaticText) {
> > 
> > or the problem is that this is used in children()?
> 
> We can't call children() here because children() is not const (it updates
> the children if required). Making any "is ignored" function non-const will
> make many other things have to be non-const as well.
> 
> This is generally unfortunate, because it makes ignoring things based on
> their contents much more difficult. We'll have to comb through the AX
> objects represented by the raw layout tree ourselves. It works fine in this
> simple example, but more complex analyses will be harder and fragile.

Maybe we need a variant of children that doesn't do an update. And  we need to rework the first and last child two. But that is for a separate patch.

> 
> > +        Vector<AccessibilityText> text;
> > ...
> > +        auto axText = text.size() ? text[0].text : String();
> > 
> > Aren't these names inverted?  i.e., shouldn't the String be text and the
> > vector axText?
> > 
> > What if the vector has more than one element? Perhaps we should return false
> > in that case.
> 
> My understanding of accessibilityText is that it's ordered in terms of the
> most applicable text based on https://www.w3.org/TR/accname-1.2/#step1. So
> by picking the first one, we are choosing the one AX clients should be using
> when describing the element. Web Inspector also picks the first
> accessibilityText.

[Tyler Wilcock]

Created attachment 441233 [details]
Patch

[Tyler Wilcock]

Marking r? again. Will wait on passing tests before marking cq?

[EWS]

Committed r284245 (243054@main): <https://commits.webkit.org/243054@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 231848

[Tyler Wilcock]

Created attachment 441492 [details]
Patch

[Tyler Wilcock]

The previous patch was reverted because it caused a failure in inspector/dom/getAccessibilityPropertiesForNode.html. This wasn’t caught in EWS because the test is marked as flaky timeout, and EWS skips those.

The test failure pointed out a bug. This patch changes AccessibilityNodeObject::textUnderElement to return null String() if the document needs style recalculation.

if (nodeDocument->childNeedsStyleRecalc())
    return { };

We never accounted for this in shouldIgnoreGroup, and we can sometimes get a null stringValue where there actually is a non-empty value in the DOM. This caused the group to be incorrectly ignored.

To protect against this, the new patch only checks for redundancies against non-null static text values. If the static text is actually null, it will be ignored anyways, and the containing group will be marked AXEmptyGroup.

[EWS]

Committed r284335 (243130@main): <https://commits.webkit.org/243130@main>

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