Bug 104688 – AX: accessibilityIsIgnored should avoid computing textUnderElement

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

accessibilityIsIgnored is very hot, and while it's useful, it's just a heuristic in many cases - it's not a serious bug if a the accessibility tree contains a few extra elements that don't provide much value. In particular, there's no reason for it to call textUnderElement, which can be quite expensive, if we can achieve basically the same result without it.

[Dominic Mazzoni]

Created attachment 178837 [details]
Patch

[chris fleizach]

Comment on attachment 178837 [details]
Patch

i had some comments about this method in the other bug. maybe you can address them in this radar

[Dominic Mazzoni]

Comment on attachment 178837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178837&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1272
> +    if (!getAttribute(aria_helpAttr).isEmpty() || !getAttribute(aria_describedbyAttr).isEmpty() || !getAttribute(altAttr).isEmpty() || !getAttribute(titleAttr).isEmpty())

chris fleizach:
> is this the exhaustive list of attributes that comprised those three checks (helpText, title, axDescription)?

This line just replaces helpText, but yeah, these new lines exhaustively check every attribute that was checked before.

The main differences are:
* We no longer worry about whether that attribute is allowed for that role or not; if an element has "alt" it won't be ignored, even if it isn't an image. This means it's possible we might have a few more elements in the accessibility tree that aren't needed, but they'll be harmless. I think it's worth it to make accessibilityIsIgnored fast (and safe to call while being destroyed).
* We no longer check inner text - see below

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1275
> +    if (isGenericFocusableElement() && roleValue() != SVGRootRole && node->firstChild())

> this line deserves a comment. why is the node->firstChild() important? what if the first child was aria-hidden or something

It's probably more clear to move the SVG root check into isGenericFocusableElement. SVG roots are focusable (why?) but usually aren't interactive, so if they don't have an ARIA role or something like that, I don't think we need to include them in the tree. (And that's what the previous logic did, too - just in a more roundabout way.)

So that seems to work 95% of the time, but there were a few weird cases where the render tree had a focusable element with no children and no role. One case was for an <input type="text">, I think it may be part of the shadow DOM for the input? This case was caught before by checking if textUnderElement was empty.

Again, this works basically the same for existing layout tests, and if it were to differ from current behavior, it would err on the side of not ignoring a node that was ignored before - but it should be harmless in that case.

[Dominic Mazzoni]

Created attachment 178878 [details]
Patch

[chris fleizach]

Comment on attachment 178878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178878&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1275
> +    if (isGenericFocusableElement() && node->firstChild())

a comment here of the form
// Make sure that a focusable element has a child to focus onto

or something appropriate would be good

[Dominic Mazzoni]

Created attachment 178949 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 178949 [details]
Patch for landing

Clearing flags on attachment: 178949

Committed r137416: <http://trac.webkit.org/changeset/137416>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.