Bug 138566 - AX: Add support for ARIA 1.1 attribute 'aria-modal' for dialog and alertdialog
Summary: AX: Add support for ARIA 1.1 attribute 'aria-modal' for dialog and alertdialog
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 146013 (view as bug list)
Depends on:
Blocks: 145646
  Show dependency treegraph
 
Reported: 2014-11-10 09:16 PST by James Craig
Modified: 2016-04-06 06:20 PDT (History)
14 users (show)

See Also:


Attachments
patch (21.84 KB, patch)
2015-10-30 15:37 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (22.14 KB, patch)
2015-10-30 23:53 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (22.14 KB, patch)
2015-10-31 00:06 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (22.67 KB, patch)
2015-11-02 12:27 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (1.48 KB, patch)
2015-11-03 11:29 PST, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2014-11-10 09:16:13 PST
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."
Comment 1 Radar WebKit Bug Importer 2014-11-10 09:16:48 PST
<rdar://problem/18926708>
Comment 2 James Craig 2014-11-10 09:19:06 PST
VO should automatically navigate to new dialogs even they are not modal, so WebKit should throw additional "DialogCreated" notifications if necessary.
Comment 3 James Craig 2014-11-10 09:20:12 PST
(In reply to comment #2)
> ...to new dialogs even they are not modal...

Even *if* they are not modal.
Comment 4 James Craig 2015-06-16 02:30:49 PDT
*** Bug 146013 has been marked as a duplicate of this bug. ***
Comment 5 Nan Wang 2015-10-30 15:37:45 PDT
Created attachment 264430 [details]
patch
Comment 6 chris fleizach 2015-10-30 17:09:43 PDT
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
Comment 7 Nan Wang 2015-10-30 17:53:52 PDT
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?
Comment 8 chris fleizach 2015-10-30 18:04:37 PDT
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
Comment 9 Nan Wang 2015-10-30 19:17:39 PDT
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?
Comment 10 Nan Wang 2015-10-30 23:53:40 PDT
Created attachment 264466 [details]
patch
Comment 11 Nan Wang 2015-10-31 00:06:11 PDT
Created attachment 264467 [details]
patch

Removed tabs in test files.
Comment 12 chris fleizach 2015-11-01 22:41:49 PST
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
Comment 13 Nan Wang 2015-11-02 11:31:43 PST
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?
Comment 14 Nan Wang 2015-11-02 11:31:45 PST
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?
Comment 15 chris fleizach 2015-11-02 11:37:09 PST
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
Comment 16 Nan Wang 2015-11-02 12:27:46 PST
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.
Comment 17 WebKit Commit Bot 2015-11-02 17:53:31 PST
Comment on attachment 264613 [details]
patch

Clearing flags on attachment: 264613

Committed r191931: <http://trac.webkit.org/changeset/191931>
Comment 18 WebKit Commit Bot 2015-11-02 17:53:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Ryan Haddad 2015-11-03 10:05:43 PST
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>
Comment 20 Nan Wang 2015-11-03 11:29:48 PST
Created attachment 264702 [details]
patch

Skip failing tests on win.
Comment 21 WebKit Commit Bot 2015-11-03 13:29:48 PST
Comment on attachment 264702 [details]
patch

Clearing flags on attachment: 264702

Committed r191972: <http://trac.webkit.org/changeset/191972>
Comment 22 WebKit Commit Bot 2015-11-03 13:29:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Joseph Scheuhammer 2016-02-29 12:39:04 PST
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.
Comment 24 Joanmarie Diggs (irc: joanie) 2016-04-05 13:04:24 PDT
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!!
Comment 25 chris fleizach 2016-04-05 13:18:18 PDT
(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
Comment 26 Joseph Scheuhammer 2016-04-06 06:20:54 PDT
(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).