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.
<rdar://problem/89442583>
Created attachment 453145 [details] Patch
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?
> > 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?
(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.
(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?
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.
> 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.
(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
> 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.
Created attachment 453297 [details] Patch
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
(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.
Created attachment 453301 [details] Patch
(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.
Created attachment 453353 [details] Alternate approach
New approach looks better than the last one.
(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?
Created attachment 453415 [details] Patch
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].