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.
Created attachment 178837 [details] Patch
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
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.
Created attachment 178878 [details] Patch
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
Created attachment 178949 [details] Patch for landing
Comment on attachment 178949 [details] Patch for landing Clearing flags on attachment: 178949 Committed r137416: <http://trac.webkit.org/changeset/137416>
All reviewed patches have been landed. Closing bug.