Bug 138566 – AX: Add support for ARIA 1.1 attribute 'aria-modal' for dialog and alertdialog

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

[James Craig]

AX: Add support for ARIA 1.1 attribute 'aria-modal' for dialog and alertdialog
http://rawgit.com/w3c/aria/master/aria/aria.html#aria-modal

aria-modal=false (default, dialog is not modal)
aria-modal=true (dialog is modal)

Current UA/AT text is: "When a modal element is displayed, assistive technologies SHOULD navigate to the element unless focus has explicitly been set elsewhere. Assistive technologies MAY limit navigation to the modal element's contents. If focus moves to an element outside the modal element, assistive technologies SHOULD NOT limit navigation to to the modal element."

[Radar WebKit Bug Importer]

<rdar://problem/18926708>

[James Craig]

VO should automatically navigate to new dialogs even they are not modal, so WebKit should throw additional "DialogCreated" notifications if necessary.

[James Craig]

(In reply to comment #2)
> ...to new dialogs even they are not modal...

Even *if* they are not modal.

[James Craig]

*** Bug 146013 has been marked as a duplicate of this bug. ***

[Nan Wang]

Created attachment 264430 [details]
patch

[chris fleizach]

Comment on attachment 264430 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:199
> +    if (!visibleNodes.isEmpty()) {

we should write this as
if (visibleNodes.isEmpty())
   return;

early returns are preferred

> Source/WebCore/accessibility/AXObjectCache.cpp:200
> +        // If any of the node is keyboard focused, we want to pick that.

If any of the nodes "are" keyboard focused

> Source/WebCore/accessibility/AXObjectCache.cpp:227
> +    if (equalIgnoringCase(downcast<Element>(*node).fastGetAttribute(aria_hiddenAttr), "true"))

we might want to use isNodeAriaVisible() here

> Source/WebCore/accessibility/AccessibilityObject.cpp:1857
> +    for (Node* node = this->node(); node; node = node->parentNode()) {

there might be some ancestorsOfKind method that is better here

> Source/WebCore/accessibility/AccessibilityObject.cpp:1864
> +bool AccessibilityObject::shouldIgnoreWhenAriaModalOn() const

I think we should name this method ignoredFromARIAModalPresence()
that's not much better than your name -- i just don't like the "aria modal on" formulation which is a bit weird since aria modal isn't something that gets turned on...

> LayoutTests/accessibility/aria-modal-multiple-dialogs.html:81
> +		dialog.style.display = 'block';

these test files have tabs in them, you should change to spaces

[Nan Wang]

Comment on attachment 264430 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1857
>> +    for (Node* node = this->node(); node; node = node->parentNode()) {
> 
> there might be some ancestorsOfKind method that is better here

I see there's a ancestorsOfType method. But shouldn't we know the type of the ancestor first? Here the ariaModalNode can be any type right? Or is there another function to get the type of a node?

[chris fleizach]

Comment on attachment 264430 [details]
patch

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

>>> Source/WebCore/accessibility/AccessibilityObject.cpp:1857
>>> +    for (Node* node = this->node(); node; node = node->parentNode()) {
>> 
>> there might be some ancestorsOfKind method that is better here
> 
> I see there's a ancestorsOfType method. But shouldn't we know the type of the ancestor first? Here the ariaModalNode can be any type right? Or is there another function to get the type of a node?

maybe use elementAncestors() and get the this->element() instead of the Node

[Nan Wang]

Comment on attachment 264430 [details]
patch

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

>>>> Source/WebCore/accessibility/AccessibilityObject.cpp:1857
>>>> +    for (Node* node = this->node(); node; node = node->parentNode()) {
>>> 
>>> there might be some ancestorsOfKind method that is better here
>> 
>> I see there's a ancestorsOfType method. But shouldn't we know the type of the ancestor first? Here the ariaModalNode can be any type right? Or is there another function to get the type of a node?
> 
> maybe use elementAncestors() and get the this->element() instead of the Node

In this case, don't we have to traverse farther than we need?

[Nan Wang]

Created attachment 264466 [details]
patch

[Nan Wang]

Created attachment 264467 [details]
patch

Removed tabs in test files.

[chris fleizach]

Comment on attachment 264467 [details]
patch

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

thanks!

> Source/WebCore/accessibility/AXObjectCache.cpp:145
> +    , m_currentAriaModalNode(nullptr)

we should remove the pointer to m_currentAriaModalNode in our method that detects when nodes disappear.
you can imagine if someone removed a tree that contains m_currentAriaModalNode, we could have a stale pointer

> Source/WebCore/accessibility/AXObjectCache.cpp:164
> +    m_currentAriaModalNode = nullptr;

this is set again to null in updateCurrentAriaModalNode() so it seems unnecessary here

> Source/WebCore/accessibility/AXObjectCache.cpp:217
> +    if (!node || !is<Element>(node))

!is<Element>(node) should take care of !node so i think we can drop that

> Source/WebCore/accessibility/AXObjectCache.cpp:1349
> +    if (!node || !is<Element>(node))

ditto about !node

> Source/WebCore/accessibility/AXObjectCache.cpp:1355
> +    stopCachingComputedObjectAttributes();

do you need to start caching again at the end of this method?

> Source/WebCore/accessibility/AXObjectCache.cpp:1360
> +            m_ariaModalNodesSet.add(node);

since m_ariaModalNodesSet is already a set i don't think you need to check contains() first

> Source/WebCore/accessibility/AXObjectCache.cpp:1364
> +        if (m_ariaModalNodesSet.contains(node))

ditto about contains()

> Source/WebCore/accessibility/AXObjectCache.h:306
> +    ListHashSet<RefPtr<Node>> m_ariaModalNodesSet;

do we want to RefPtr the nodes here? i don't think we retain other Nodes. We just have to be careful about cleaning up these things

you can imagine if the Document node got into this list we'd have a retain cycle

> Source/WebCore/accessibility/AccessibilityObject.cpp:1867
> +bool AccessibilityObject::ignoredFromARIAModalPresence() const

can you add a comment what this method does

> Source/WebCore/accessibility/AccessibilityObject.cpp:1870
> +    if (!node() || !node()->parentNode())

is this necessary? it seems like the below checks will take care of these cases

[Nan Wang]

Comment on attachment 264467 [details]
patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:145
>> +    , m_currentAriaModalNode(nullptr)
> 
> we should remove the pointer to m_currentAriaModalNode in our method that detects when nodes disappear.
> you can imagine if someone removed a tree that contains m_currentAriaModalNode, we could have a stale pointer

In ariaModalNode() to get the current modal node, isNodeVisible(m_currentAriaModalNode) will check if the node is still there and set it to nullptr in updateCurrentAriaModalNode(). So that makes sure we are not accessing a stale pointer. Do you think that's enough?

[Nan Wang]

Comment on attachment 264467 [details]
patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:145
>> +    , m_currentAriaModalNode(nullptr)
> 
> we should remove the pointer to m_currentAriaModalNode in our method that detects when nodes disappear.
> you can imagine if someone removed a tree that contains m_currentAriaModalNode, we could have a stale pointer

In ariaModalNode() to get the current modal node, isNodeVisible(m_currentAriaModalNode) will check if the node is still there and set it to nullptr in updateCurrentAriaModalNode(). So that makes sure we are not accessing a stale pointer. Do you think that's enough?

[chris fleizach]

Comment on attachment 264467 [details]
patch

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

>>>> Source/WebCore/accessibility/AXObjectCache.cpp:145
>>>> +    , m_currentAriaModalNode(nullptr)
>>> 
>>> we should remove the pointer to m_currentAriaModalNode in our method that detects when nodes disappear.
>>> you can imagine if someone removed a tree that contains m_currentAriaModalNode, we could have a stale pointer
>> 
>> In ariaModalNode() to get the current modal node, isNodeVisible(m_currentAriaModalNode) will check if the node is still there and set it to nullptr in updateCurrentAriaModalNode(). So that makes sure we are not accessing a stale pointer. Do you think that's enough?
> 
> In ariaModalNode() to get the current modal node, isNodeVisible(m_currentAriaModalNode) will check if the node is still there and set it to nullptr in updateCurrentAriaModalNode(). So that makes sure we are not accessing a stale pointer. Do you think that's enough?

i don't think so. this call will cause a segfault
!is<Element>(node))

because node will be garbage memory

we should also add a test for this case

[Nan Wang]

Created attachment 264613 [details]
patch

Added cleanup code for m_currentAriaModalNode and m_ariaModalNodesSet when nodes get removed. Also added test in aria-modal.html

For top node check in ignoredFromARIAModalPresence()
if (!node() || !node()->parentNode()), I think it's still necessary. Without this, when there's a dialog displayed and top node will be ignored because isAriaModalDescendant() will return false.

[WebKit Commit Bot]

Comment on attachment 264613 [details]
patch

Clearing flags on attachment: 264613

Committed r191931: <http://trac.webkit.org/changeset/191931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The tests added with this patch are have been failing on win since landing:

Run:
<https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/54892>
Results:
<https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r191952%20(54892)/results.html>

[Nan Wang]

Created attachment 264702 [details]
patch

Skip failing tests on win.

[WebKit Commit Bot]

Comment on attachment 264702 [details]
patch

Clearing flags on attachment: 264702

Committed r191972: <http://trac.webkit.org/changeset/191972>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Scheuhammer]

What are the AXAPI mappings for aria-modal in the Core-AAM, for both "true" and "false" values:

http://rawgit.com/w3c/aria/master/core-aam/core-aam.html#ariaModalTrue
http://rawgit.com/w3c/aria/master/core-aam/core-aam.html#ariaModalFalse


Put another way, what does the AccessibilityInspector tool show for an accessible object in the two cases?

Thanks.

[Joanmarie Diggs]

Chris: Another one for which an official mapping (i.e. to put in a Core AAM spec table for AXAPI) is needed.

Looking at the code, it seems like the "mapping"/exposure for aria-modal is to prune the tree, so the background content is no longer exposed. Could you please confirm or correct this so Joseph (CCed) can add the mapping for your platform to his spec?

Thanks!!

[chris fleizach]

(In reply to comment #24)
> Chris: Another one for which an official mapping (i.e. to put in a Core AAM
> spec table for AXAPI) is needed.
> 
> Looking at the code, it seems like the "mapping"/exposure for aria-modal is
> to prune the tree, so the background content is no longer exposed. Could you
> please confirm or correct this so Joseph (CCed) can add the mapping for your
> platform to his spec?
> 
> Thanks!!

that is correct

[Joseph Scheuhammer]

(In reply to comment #25)
> (In reply to comment #24)
> > Chris: Another one for which an official mapping (i.e. to put in a Core AAM
> > spec table for AXAPI) is needed.
> > 
> > Looking at the code, it seems like the "mapping"/exposure for aria-modal is
> > to prune the tree, so the background content is no longer exposed. Could you
> > please confirm or correct this so Joseph (CCed) can add the mapping for your
> > platform to his spec?
> > 
> > Thanks!!
> 
> that is correct

Thanks, Chris.

(and Joanie).