Bug 243486 - AX: AXCoreObject::textUnderElement always returns an empty string for display:contents elements
Summary: AX: AXCoreObject::textUnderElement always returns an empty string for display...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-08-03 00:04 PDT by Tyler Wilcock
Modified: 2023-10-07 17:20 PDT (History)
12 users (show)

See Also:


Attachments
Patch (5.99 KB, patch)
2022-08-03 00:10 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (6.08 KB, patch)
2022-08-03 09:33 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (6.83 KB, patch)
2022-08-03 09:50 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (7.23 KB, patch)
2022-08-04 18:57 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (8.07 KB, patch)
2022-08-04 20:44 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2022-08-03 00:04:11 PDT
AXCoreObject::textUnderElement always returns an empty string for display:contents elements
Comment 1 Radar WebKit Bug Importer 2022-08-03 00:05:37 PDT
<rdar://problem/98032693>
Comment 2 Tyler Wilcock 2022-08-03 00:10:12 PDT
Created attachment 461372 [details]
Patch
Comment 3 Tyler Wilcock 2022-08-03 09:33:25 PDT
Created attachment 461380 [details]
Patch
Comment 4 Tyler Wilcock 2022-08-03 09:50:35 PDT
Created attachment 461381 [details]
Patch
Comment 5 Darin Adler 2022-08-03 15:31:42 PDT
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.
Comment 6 Tyler Wilcock 2022-08-04 17:31:56 PDT
(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.
Comment 7 Tyler Wilcock 2022-08-04 18:57:57 PDT
Created attachment 461412 [details]
Patch
Comment 8 Tyler Wilcock 2022-08-04 20:44:57 PDT
Created attachment 461416 [details]
Patch
Comment 9 Andres Gonzalez 2022-08-05 08:58:02 PDT
(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.
Comment 10 Tyler Wilcock 2022-08-05 09:22:10 PDT
(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.
Comment 11 EWS 2022-08-05 11:16:39 PDT
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].
Comment 12 Adrian Roselli 2023-10-07 14:35:34 PDT
Still an issue through Safari 17 / TP 180, new bug as of April 2023:
https://bugs.webkit.org/show_bug.cgi?id=255149
Comment 13 Tyler Wilcock 2023-10-07 14:55:22 PDT
(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.
Comment 14 Adrian Roselli 2023-10-07 16:13:27 PDT
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.
Comment 15 Tyler Wilcock 2023-10-07 17:01:28 PDT
(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.
Comment 16 Tyler Wilcock 2023-10-07 17:20:08 PDT
(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.