AX: AccessibilityRenderObject::computeAccessibilityIsIgnored performs an unnecessary ancestry traversal for RenderTexts

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

AccessibilityRenderObject::computeAccessibilityIsIgnored for RenderTexts traverses the ancestry twice. We can combine this into one traversal.

[Radar WebKit Bug Importer]

<rdar://problem/118437052>

[Tyler Wilcock]

Created attachment 468599 [details]
Patch

[chris fleizach]

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 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]

Created attachment 468600 [details]
Patch

[Tyler Wilcock]

(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]

(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]

Created attachment 468605 [details]
Patch

[Tyler Wilcock]

Created attachment 468606 [details]
Patch

[Tyler Wilcock]

> -        // 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]

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].