Bug 240978 – AX: WebKit does not trap user focus inside modals that have been DOM moved

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

WebKit does not trap user focus inside modals that have been DOM moved. This pattern is used in the WAI-ARIA aria-modal usage example: https://w3c.github.io/aria-practices/examples/dialog-modal/dialog.html

[Radar WebKit Bug Importer]

<rdar://problem/93996539>

[Tyler Wilcock]

Created attachment 459796 [details]
Patch

[Tyler Wilcock]

Created attachment 459798 [details]
Patch

[Tyler Wilcock]

Created attachment 459933 [details]
Patch

[Andres Gonzalez]

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

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

-Element* AXObjectCache::currentModalNode()
+Element* AXObjectCache::recomputeCurrentModalNode()

Wouldn't it make more sense to name it updateCurrentModalNode or updateCurrentModalElement and have a void return? Since in most calls the returned value is not used. We have another method named modalNode that can do the update if needed and return the stored pointer.

+{
+    auto* previousModal = m_currentModalElement.get();
+    m_currentModalElement = recomputeCurrentModalNodeInner();
+    if (previousModal != m_currentModalElement.get()) {
+        childrenChanged(rootWebArea());
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+        // Because the presence of a modal affects every element on the page,
+        // regenerate the entire isolated tree with the next cache update.
+        m_deferredRegenerateIsolatedTree = true;
+#endif

Would it be more appropriate to do the update of the isolated tree in the childrenChanged or handleChildrenChanged as everything else? Perhaps we need to add an AXNotification for a new modal and update the isolated tree upon that notification.

+Element* AXObjectCache::recomputeCurrentModalNodeInner()

See comment above about update versus recompute prefix. I think Internal is more customary than Inner as a suffix for a private method name that is invoked by another possibly public method, but I don't feel strongly either way.

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

-    void handleFocusedUIElementChanged(Node* oldFocusedNode, Node* newFocusedNode);
+    enum class RecomputeModal : bool { No, Yes };
+    void handleFocusedUIElementChanged(Node* oldFocusedNode, Node* newFocusedNode, RecomputeModal = RecomputeModal::Yes);

RecomputeModal -> UpdateModal ?

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    bool m_deferredRegenerateIsolatedTree { false };
+#endif

If we handle this as an AXNotification as the rest of all isolated tree updates, I don't think we would need this.

[Tyler Wilcock]

Created attachment 460016 [details]
Patch

[EWS]

Committed r295366 (251374@main): <https://commits.webkit.org/251374@main>

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

[Giacomo Petri]

Hi,

there is still a bug on Safari mobile + VO. I'm not sure if the issue is related to this topic or more to this one https://bugs.webkit.org/show_bug.cgi?id=239295, but on mobile users are still not trapped within the modal.

Giacomo