Bug 264861

Summary: AX: AccessibilityRenderObject::computeAccessibilityIsIgnored performs an unnecessary ancestry traversal for RenderTexts
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Tyler Wilcock
Reported 2023-11-14 21:56:47 PST
AccessibilityRenderObject::computeAccessibilityIsIgnored for RenderTexts traverses the ancestry twice. We can combine this into one traversal.
Attachments
Patch (4.38 KB, patch)
2023-11-14 22:03 PST, Tyler Wilcock
no flags
Patch (4.33 KB, patch)
2023-11-14 22:52 PST, Tyler Wilcock
no flags
Patch (4.49 KB, patch)
2023-11-15 09:31 PST, Tyler Wilcock
no flags
Patch (4.59 KB, patch)
2023-11-15 09:55 PST, Tyler Wilcock
no flags
Radar WebKit Bug Importer
Comment 1 2023-11-14 21:56:54 PST
Tyler Wilcock
Comment 2 2023-11-14 22:03:46 PST
chris fleizach
Comment 3 2023-11-14 22:42:04 PST
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
chris fleizach
Comment 4 2023-11-14 22:42:16 PST
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
Tyler Wilcock
Comment 5 2023-11-14 22:52:14 PST
Tyler Wilcock
Comment 6 2023-11-14 22:54:34 PST
(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).
Andres Gonzalez
Comment 7 2023-11-15 06:43:56 PST
(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())
Tyler Wilcock
Comment 8 2023-11-15 09:31:02 PST
Tyler Wilcock
Comment 9 2023-11-15 09:55:21 PST
Tyler Wilcock
Comment 10 2023-11-15 09:56:29 PST
> - // 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.
EWS
Comment 11 2023-11-15 20:59:24 PST
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].
Note You need to log in before you can comment on or make changes to this bug.