Summary: | AX: WebKit should not expose redundant AXGroups with missing role when the label is the same as the contents | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, commit-queue, dmazzoni, ews-watchlist, jdiggs, samuel_white, tyler_w, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | 231848 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
James Craig
2017-03-21 14:22:52 PDT
Created attachment 441161 [details]
Patch
Created attachment 441171 [details]
Patch
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 Created attachment 441217 [details]
Patch
(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. (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. > + 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. (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. Created attachment 441233 [details]
Patch
Marking r? again. Will wait on passing tests before marking cq? 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]. Re-opened since this is blocked by bug 231848 Created attachment 441492 [details]
Patch
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. 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]. |