AccessibilityRenderObject::computeAccessibilityIsIgnored for RenderTexts traverses the ancestry twice. We can combine this into one traversal.
<rdar://problem/118437052>
Created attachment 468599 [details] Patch
Comment on attachment 468599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468599&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1104 > + if (auto* renderText = dynamicDowncast<RenderText>(m_renderer.get())) { do we need to hold onto this object for scope of function incase it's deallocated? > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1126 > if (!m_renderer) do we use m_renderer anymore after this point? can we get rid of this check
Created attachment 468600 [details] Patch
(In reply to chris fleizach from comment #4) > Comment on attachment 468599 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=468599&action=review > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1104 > > + if (auto* renderText = dynamicDowncast<RenderText>(m_renderer.get())) { > > do we need to hold onto this object for scope of function incase it's > deallocated? > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1126 > > if (!m_renderer) > > do we use m_renderer anymore after this point? can we get rid of this check Addressed both comments with use of a WeakPtr, which also simplifies the code a bit further. It's OK if the RenderText gets deallocated in the middle of our function call, as the WeakPtr will become null and we will ignore the associated AX object (which presumably will be destroyed shortly after).
(In reply to Tyler Wilcock from comment #5) > Created attachment 468600 [details] > Patch diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp index b4de65036782..27448140ec73 100644 --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp @@ -1101,45 +1101,38 @@ bool AccessibilityRenderObject::computeAccessibilityIsIgnored() const if (m_renderer->isBR()) return true; - if (is<RenderText>(*m_renderer)) { - // static text beneath MenuItems and MenuButtons are just reported along with the menu item, so it's ignored on an individual level - AXCoreObject* parent = parentObjectUnignored(); - - // Walking up the parent chain might reset the m_renderer. - if (!m_renderer) - return true; - - if (parent && (parent->isMenuItem() || parent->isMenuButton())) - return true; - - auto& renderText = downcast<RenderText>(*m_renderer); - if (!renderText.hasRenderedText()) + if (WeakPtr renderText = dynamicDowncast<RenderText>(m_renderer.get())) { + if (!renderText->hasRenderedText()) return true; - if (renderText.parent()->isFirstLetter()) + if (renderText->parent()->isFirstLetter()) return true; - // static text beneath TextControls is reported along with the text control text so it's ignored. - for (AccessibilityObject* parent = parentObject(); parent; parent = parent->parentObject()) { - if (parent->roleValue() == AccessibilityRole::TextField) + bool checkForIgnored = true; + for (RefPtr ancestor = parentObject(); ancestor; ancestor = ancestor->parentObject()) { + // Static text beneath TextControls is reported along with the text control text so it's ignored. + if (ancestor->roleValue() == AccessibilityRole::TextField) return true; AG: do we need to account also for other roles that are isTextControl()? + + if (checkForIgnored && !ancestor->accessibilityIsIgnored()) { + checkForIgnored = false; + // Static text beneath MenuItems and MenuButtons are just reported along with the menu item, so it's ignored on an individual level. + if (ancestor->isMenuItem() || ancestor->isMenuButton()) + return true; + } } - - // Walking up the parent chain might reset the m_renderer. - if (!m_renderer) - return true; - + // The alt attribute may be set on a text fragment through CSS, which should be honored. - if (is<RenderTextFragment>(renderText)) { - AccessibilityObjectInclusion altTextInclusion = objectInclusionFromAltText(downcast<RenderTextFragment>(renderText).altText()); + if (auto* renderTextFragment = dynamicDowncast<RenderTextFragment>(renderText.get())) { + auto altTextInclusion = objectInclusionFromAltText(renderTextFragment->altText()); if (altTextInclusion == AccessibilityObjectInclusion::IgnoreObject) return true; if (altTextInclusion == AccessibilityObjectInclusion::IncludeObject) return false; } - // text elements that are just empty whitespace should not be returned - return renderText.text().containsOnly<isASCIIWhitespace>(); + // Text elements that are just empty whitespace should not be part of the AX tree. + return !renderText || renderText->text().containsOnly<isASCIIWhitespace>(); } if (isHeading())
Created attachment 468605 [details] Patch
Created attachment 468606 [details] Patch
> - // static text beneath TextControls is reported along with the text > control text so it's ignored. > - for (AccessibilityObject* parent = parentObject(); parent; parent = > parent->parentObject()) { > - if (parent->roleValue() == AccessibilityRole::TextField) > + bool checkForIgnored = true; > + for (RefPtr ancestor = parentObject(); ancestor; ancestor = > ancestor->parentObject()) { > + // Static text beneath TextControls is reported along with the > text control text so it's ignored. > + if (ancestor->roleValue() == AccessibilityRole::TextField) > return true; > > AG: do we need to account also for other roles that are isTextControl()? The comment implies so, but I'm not looking to change that behavior in this patch. I've added a FIXME for now.
Committed 270805@main (02e30d0b0337): <https://commits.webkit.org/270805@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 468606 [details].