AX: AXCoreObject::textUnderElement always returns an empty string for display:contents elements

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

AXCoreObject::textUnderElement always returns an empty string for display:contents elements

[Radar WebKit Bug Importer]

<rdar://problem/98032693>

[Tyler Wilcock]

Created attachment 461372 [details]
Patch

[Tyler Wilcock]

Created attachment 461380 [details]
Patch

[Tyler Wilcock]

Created attachment 461381 [details]
Patch

[Darin Adler]

Comment on attachment 461381 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:3729
> +    if (auto* node = this->node())
> +        style = node->computedStyle();
> +    else if (auto* renderer = this->renderer())
> +        style = &renderer->style();

Getting this from the renderer is faster, so I think we want to do that check first. Unless there is a correctness reason for doing it in the other order. I would write this.

    const RenderStyle* style = nullptr;
    if (auto* renderer = this->renderer())
        style = &renderer->style();
    if (!style) {
        if (auto* node = this->node())
            style = node->computedStyle();
        if (!style)
            return true;
    }

You might prefer to do the details differently, but the order affects performance, so let’s do renderer first.

[Tyler Wilcock]

(In reply to Darin Adler from comment #5)
> Comment on attachment 461381 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=461381&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:3729
> > +    if (auto* node = this->node())
> > +        style = node->computedStyle();
> > +    else if (auto* renderer = this->renderer())
> > +        style = &renderer->style();
> 
> Getting this from the renderer is faster, so I think we want to do that
> check first. Unless there is a correctness reason for doing it in the other
> order.
Good to know, thanks! No correctness issue in the ordering. Your suggested code looks good, I'll do it that way.

[Tyler Wilcock]

Created attachment 461412 [details]
Patch

[Tyler Wilcock]

Created attachment 461416 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #8)
> Created attachment 461416 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp


 bool AccessibilityObject::isDOMHidden() const
 {
-    RenderObject* renderer = this->renderer();
-    if (!renderer)
-        return true;
-
-    const RenderStyle& style = renderer->style();
-    return style.display() == DisplayType::None || style.visibility() != Visibility::Visible;
+    if (auto* style = this->style())
+        return style->display() == DisplayType::None || style->visibility() != Visibility::Visible;
+    return true;
 }

Can there be an AX object that has no style and shouldn't be hidden? e.g., one of the AX mocked objects that have no backing Element or RenderObject. All depends on how isDOMHidden() is being used.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Tyler Wilcock from comment #8)
> > Created attachment 461416 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
> 
> 
>  bool AccessibilityObject::isDOMHidden() const
>  {
> -    RenderObject* renderer = this->renderer();
> -    if (!renderer)
> -        return true;
> -
> -    const RenderStyle& style = renderer->style();
> -    return style.display() == DisplayType::None || style.visibility() !=
> Visibility::Visible;
> +    if (auto* style = this->style())
> +        return style->display() == DisplayType::None || style->visibility()
> != Visibility::Visible;
> +    return true;
>  }
> 
> Can there be an AX object that has no style and shouldn't be hidden? e.g.,
> one of the AX mocked objects that have no backing Element or RenderObject.
> All depends on how isDOMHidden() is being used.
Possibly, but I think we'll need to address that specific situation when it arises. Those objects will behave the same before and after this patch.

[EWS]

Committed 253154@main (828257ce94c0): <https://commits.webkit.org/253154@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 461416 [details].

[Adrian Roselli]

Still an issue through Safari 17 / TP 180, new bug as of April 2023:
https://bugs.webkit.org/show_bug.cgi?id=255149

[Tyler Wilcock]

(In reply to Adrian Roselli from comment #12)
> Still an issue through Safari 17 / TP 180, new bug as of April 2023:
> https://bugs.webkit.org/show_bug.cgi?id=255149
The symptom of this bug was that the text underneath a display:contents element was not exposed to assistive technologies. So for example:

<button style="display:contents">Click me</button>

Would result in VoiceOver not reading any name for the button (regardless of its focusability).

Conversely, https://bugs.webkit.org/show_bug.cgi?id=255149 relates to the focusability of display:contents elements, which is a separate issue. Want to make sure we're not conflating these two.

[Adrian Roselli]

Ah, so this addresses the unannounced accName in Safari TP 151 (and Safari 16) that was fixed for Safari TP 152 (and Safari 16.5). Got it.

My mistake. Sorry for adding noise.

[Tyler Wilcock]

(In reply to Adrian Roselli from comment #14)
> Ah, so this addresses the unannounced accName in Safari TP 151 (and Safari
> 16) that was fixed for Safari TP 152 (and Safari 16.5). Got it.
> 
> My mistake. Sorry for adding noise.
No worries! From what I see on my end, this fix would've shipped with Safari 16, but the symptoms do sound like what you're describing.

[Tyler Wilcock]

(In reply to Tyler Wilcock from comment #15)
> (In reply to Adrian Roselli from comment #14)
> > Ah, so this addresses the unannounced accName in Safari TP 151 (and Safari
> > 16) that was fixed for Safari TP 152 (and Safari 16.5). Got it.
> > 
> > My mistake. Sorry for adding noise.
> No worries! From what I see on my end, this fix would've shipped with Safari
> 16, but the symptoms do sound like what you're describing.
Sorry, Safari 16.1, not Safari 16.

[Otis Chapman]

(In reply to Tyler Wilcock from comment #13)
> (In reply to Adrian Roselli from comment #12)
> > Still an issue through Safari 17 / TP 180, new bug as of April 2023:
> > https://bugs.webkit.org/show_bug.cgi?id=255149 https://geometry-lite.io
> The symptom of this bug was that the text underneath a display:contents
> element was not exposed to assistive technologies. So for example:
> 
> <button style="display:contents">Click me</button>
> 
> Would result in VoiceOver not reading any name for the button (regardless of
> its focusability).
> 
> Conversely, https://bugs.webkit.org/show_bug.cgi?id=255149 relates to the
> focusability of display:contents elements, which is a separate issue. Want
> to make sure we're not conflating these two.

Since the renderer can provide this more quickly, I believe we should do that check first. Unless there's a good reason to do it the opposite way around. This is what I would write.


    const RenderStyle* style = nullptr;
    if (auto* renderer = this->renderer())
        style = &renderer->style();
    if (!style) {
        if (auto* node = this->node())
            style = node->computedStyle();
        if (!style)
            return true;
    }

[Lily Smith]

Still an issue through Safari 17 / TP 180, new bug as of April 2023:
https://bugs.webkit.org/show_bug.cgi?id=255149https://mapdirections.app/

[Lily Smith]

Still an issue through Safari 17 / TP 180, new bug as of April 2023:
https://bugs.webkit.org/show_bug.cgi?id=255149 https://mapdirections.app/