Bug 237163 – AX: WebKit should ignore empty modals rather than trapping focus inside them

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

Given this markup:

<div role="dialog" aria-modal="true">
    <div aria-hidden="true">
        <button>Close modal (inside modal)</button>
    </div>
</div>

There is no accessible content inside this modal, but WebKit traps user focus inside, making all of the rest of the page completely inaccessible.

medium.com articles (e.g. https://medium.com/@emilymenonbender/no-llms-arent-like-people-with-disabilities-and-it-s-problematic-to-argue-that-they-are-a2ac0df0e435) have similar markup, and are thus entirely inaccessible.

[Radar WebKit Bug Importer]

<rdar://problem/89442583>

[Tyler Wilcock]

Created attachment 453145 [details]
Patch

[chris fleizach]

Comment on attachment 453145 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:277
> +    RenderObject* renderer = node->renderer();

auto*

> Source/WebCore/accessibility/AXObjectCache.cpp:305
> +{

This seems very expensive now. Determining aria modal will have to visit every node. Is this something we could determine another way?

[Tyler Wilcock]

> > Source/WebCore/accessibility/AXObjectCache.cpp:305
> > +{
> 
> This seems very expensive now. Determining aria modal will have to visit
> every node. Is this something we could determine another way?
The only additional work we're doing is on elements within m_modalElementsSet, which is limited only to page elements with role="dialog" and aria-modal="true", or <dialog> elements.

And from there, the additional work we do is a scan of the direct children of the modal (i.e. one layer traversal vs. an entire subtree traversal).

And then the result is cached in m_currentModalElement, so we shouldn't have to re-compute this often.

So I don't think it's much more expensive. What do you think?

[Andres Gonzalez]

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

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

+static bool isNodeVisible(Node* node)
+{
+    if (!is<Element>(node))
+        return false;

I think static text will return false here, don't see the need for this check, only when you are going to check for element attributes like aria-hidden.

Ah I see down below that this was a class method and you turned it into a static function. Still I don't think that the check for is<Element> is good.

[chris fleizach]

(In reply to Tyler Wilcock from comment #4)
> > > Source/WebCore/accessibility/AXObjectCache.cpp:305
> > > +{
> > 
> > This seems very expensive now. Determining aria modal will have to visit
> > every node. Is this something we could determine another way?
> The only additional work we're doing is on elements within
> m_modalElementsSet, which is limited only to page elements with
> role="dialog" and aria-modal="true", or <dialog> elements.
> 
> And from there, the additional work we do is a scan of the direct children
> of the modal (i.e. one layer traversal vs. an entire subtree traversal).

Will a one level traversal be enough or do we need to keep digging to determine if we still have accessible content?

> 
> And then the result is cached in m_currentModalElement, so we shouldn't have
> to re-compute this often.
> 
> So I don't think it's much more expensive. What do you think?

[Aakash Jain]

I cancelled https://ews-build.webkit.org/#/builders/68/builds/9127 to speed up ios-wk2  queue. It finished running layout-test and indicated some failures, please have a look.

[Tyler Wilcock]

> Will a one level traversal be enough or do we need to keep digging to
> determine if we still have accessible content?
One layer is enough, since aria-hidden="true", visibility: hidden, and display: none all inherently make the layers beneath also invisible to AX. So we only need to find one thing in the first layer that is none of those to consider the modal valid.

[chris fleizach]

(In reply to Tyler Wilcock from comment #8)
> > Will a one level traversal be enough or do we need to keep digging to
> > determine if we still have accessible content?
> One layer is enough, since aria-hidden="true", visibility: hidden, and
> display: none all inherently make the layers beneath also invisible to AX.
> So we only need to find one thing in the first layer that is none of those
> to consider the modal valid.

What if we had (apologies for code typed on a phone)

<div modal=true>
    <div>
       <div>
          <div hidden>
             Content

[Tyler Wilcock]

> What if we had (apologies for code typed on a phone)
> 
> <div modal=true>
>     <div>
>        <div>
>           <div hidden>
>              Content
You're right, we do need to traverse multiple levels in this case. I'll fix it.

[Tyler Wilcock]

Created attachment 453297 [details]
Patch

[chris fleizach]

Comment on attachment 453297 [details]
Patch

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

> Source/WebCore/accessibility/ShouldConsiderModals.h:30
> +enum class ShouldConsiderModals : uint8_t { No, Yes };

I think we could drop the should from this and still have same clarity. 

Also can we put this enum in some accessibility file? Seems like a little for just one yes/no

[Tyler Wilcock]

(In reply to chris fleizach from comment #12)
> Comment on attachment 453297 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453297&action=review
> 
> > Source/WebCore/accessibility/ShouldConsiderModals.h:30
> > +enum class ShouldConsiderModals : uint8_t { No, Yes };
> 
> I think we could drop the should from this and still have same clarity. 
> 
> Also can we put this enum in some accessibility file? Seems like a little
> for just one yes/no
I tried putting it in AccessibilityObjectInterface.h, which worked everywhere except when it's used in AXObjectCache.cpp which doesn't include the interface header. They don't share any includes that would make sense to put this new enum in, so I went for a separate file.

ConsiderModals sounds good to me, will update.

[Tyler Wilcock]

Created attachment 453301 [details]
Patch

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h

-    virtual AccessibilityObjectInclusion defaultObjectInclusion() const = 0;
-    virtual bool accessibilityIsIgnoredByDefault() const = 0;
+    virtual AccessibilityObjectInclusion defaultObjectInclusion(ConsiderModals = ConsiderModals::Yes) const = 0;
+    virtual bool accessibilityIsIgnoredByDefault(ConsiderModals = ConsiderModals::Yes) const = 0;

I think this is the wrong implementation. If modal elements need to be special cased, it would be a lot cleaner to have a subclass of AccessibilityObject for them.

[Tyler Wilcock]

Created attachment 453353 [details]
Alternate approach

[chris fleizach]

New approach looks better than the last one.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #16)
> Created attachment 453353 [details]
> Alternate approach

Yes, much better approach.

--- a/LayoutTests/accessibility/ignore-modals-without-any-content.html
+++ a/LayoutTests/accessibility/ignore-modals-without-any-content.html

+    async function findTextInsideId(id) {
+        const containerElement = accessibilityController.accessibleElementById(id);
+        testOutput += `\nBeginning search from #${id} element.\n`;
+
+        let searchResult = null;
+        while (true) {
+            searchResult = containerElement.uiElementForSearchPredicate(searchResult, true, "AXAnyTypeSearchKey", "", false);
+            if (!searchResult)
+                break;
+
+            if (searchResult.role.includes("StaticText")) {
+                testOutput += `\n${searchResult.role}`;
+                const textContent = accessibilityController.platformName === "ios" ? searchResult.description : searchResult.stringValue;
+                testOutput += `\n${textContent}\n`;
+            }
+        }
+    }
+

Do we need a while loop here?

[Tyler Wilcock]

Created attachment 453415 [details]
Patch

[EWS]

Committed r290630 (247903@main): <https://commits.webkit.org/247903@main>

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