AX: findModalNodes() and currentModalNode() should include modal <dialog>

[Tim Nguyen (:ntim)]

https://webkit-search.igalia.com/webkit/rev/367ea6f7763d697109fda42511484000a23de506/Source/WebCore/accessibility/AXObjectCache.cpp#253

[Radar WebKit Bug Importer]

<rdar://problem/82680992>

[Tim Nguyen (:ntim)]

Created attachment 437179 [details]
Patch

[chris fleizach]

Comment on attachment 437179 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        * accessibility/AXObjectCache.cpp:

can you add a new test for this case?

[chris fleizach]

Comment on attachment 437179 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:259
> +        bool isAriaModal = !equalLettersIgnoringASCIICase(element->attributeWithoutSynchronization(aria_modalAttr), "true");

this should change to 

!equalLetter -> equalLetter

> Source/WebCore/accessibility/AXObjectCache.cpp:261
> +        if ((hasDialogRole && isAriaModal) || (is<HTMLDialogElement>(element) && downcast<HTMLDialogElement>(element)->isModal()))

is there any provision for aria-model=false on <dialog modal> to override the native status?

[Andres Gonzalez]

(In reply to Tim Nguyen (:ntim) from comment #2)
> Created attachment 437179 [details]
> Patch

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

+        bool isAriaModal = !equalLettersIgnoringASCIICase(element->attributeWithoutSynchronization(aria_modalAttr), "true");

Should be :

+        bool isAriaModal = equalLettersIgnoringASCIICase(element->attributeWithoutSynchronization(aria_modalAttr), "true");

Could you add a helper function, bool isModalElement, that does:

+        bool hasDialogRole = nodeHasRole(element, "dialog") || nodeHasRole(element, "alertdialog");
+        bool isAriaModal = equalLettersIgnoringASCIICase(element->attributeWithoutSynchronization(aria_modalAttr), "true");

+        return (hasDialogRole && isAriaModal) || (is<HTMLDialogElement>(element) && downcast<HTMLDialogElement>(element)->isModal());

or equivalent?

[Tim Nguyen (:ntim)]

(In reply to chris fleizach from comment #4)
> Comment on attachment 437179 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437179&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:259
> > +        bool isAriaModal = !equalLettersIgnoringASCIICase(element->attributeWithoutSynchronization(aria_modalAttr), "true");
> 
> this should change to 
> 
> !equalLetter -> equalLetter

Good catch, thanks!

> > Source/WebCore/accessibility/AXObjectCache.cpp:261
> > +        if ((hasDialogRole && isAriaModal) || (is<HTMLDialogElement>(element) && downcast<HTMLDialogElement>(element)->isModal()))
> 
> is there any provision for aria-model=false on <dialog modal> to override
> the native status?

dialog.showModal() will make the content behind inert and make only the dialog interactive per the HTML spec. 

From: https://www.w3.org/TR/wai-aria-practices-1.1/examples/dialog-modal/dialog.html

> Applying the aria-modal property to the dialog element replaces the technique of using aria-hidden on the background for informing assistive technologies that content outside a dialog is inert.

So no, aria-modal=false should not have an effect on this, since the content behind will be inert either way, according to the HTML spec.

[Tim Nguyen (:ntim)]

Created attachment 437278 [details]
Patch

[James Craig]

> is there any provision for aria-model=false on <dialog modal> to override
> the native status?

In general, equivalent host language attributes "win", but ARIA roles always "win." There's a little ambiguity in this case, since the `is modal` flag of the dialog API is not an attribute, but in general, I would not expect `aria-modal` to have any effect on the dialog element.

There's more detail about conflict resolution in the ARIA spec, and I'll file an ARIA issue about the ambiguity.
https://w3c.github.io/aria/#host_general_conflict

[James Craig]

Filed ARIA 1610 https://github.com/w3c/aria/issues/1610

[Andres Gonzalez]

(In reply to Tim Nguyen (:ntim) from comment #7)
> Created attachment 437278 [details]
> Patch

--- a/LayoutTests/accessibility/dialog-showModal.html
+++ a/LayoutTests/accessibility/dialog-showModal.html

+            window.okButton = accessibilityController.accessibleElementById("ok");
+            shouldBeFalse("okButton.isIgnored");

This should be false when the dialog is modal or not because it is part of the dialog. Why are we checking this only if it is not modal? Perhaps it makes more sense to check whether an element int the background is not ignored, which you are already doing in backgroundAccessible.

Is it really a good idea that we convey that there is a modal dialog if it is aria-hidden or opacity 0?

[Tim Nguyen (:ntim)]

(In reply to Andres Gonzalez from comment #10)
> (In reply to Tim Nguyen (:ntim) from comment #7)
> > Created attachment 437278 [details]
> > Patch
> 
> --- a/LayoutTests/accessibility/dialog-showModal.html
> +++ a/LayoutTests/accessibility/dialog-showModal.html
> 
> +            window.okButton =
> accessibilityController.accessibleElementById("ok");
> +            shouldBeFalse("okButton.isIgnored");
> 
> This should be false when the dialog is modal or not because it is part of
> the dialog. Why are we checking this only if it is not modal? Perhaps it
> makes more sense to check whether an element int the background is not
> ignored, which you are already doing in backgroundAccessible.
> 
> Is it really a good idea that we convey that there is a modal dialog if it
> is aria-hidden or opacity 0?

Yes, because the content behind is rendered inert per-HTML spec, and there's also a backdrop behind blocking any interaction.

[Tim Nguyen (:ntim)]

Created attachment 437309 [details]
Patch

[Tim Nguyen (:ntim)]

Committed r282022 (241327@main): <https://commits.webkit.org/241327@main>