Summary: | AX: Add support for ARIA 1.1 attribute 'aria-modal' for dialog and alertdialog | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, clown, commit-queue, dmazzoni, esprehn+autocc, gyuyoung.kim, jdiggs, mario, n_wang, ryanhaddad, samuel_white, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 145646 | ||||||||||||||
Attachments: |
|
Description
James Craig
2014-11-10 09:16:13 PST
VO should automatically navigate to new dialogs even they are not modal, so WebKit should throw additional "DialogCreated" notifications if necessary. (In reply to comment #2) > ...to new dialogs even they are not modal... Even *if* they are not modal. *** Bug 146013 has been marked as a duplicate of this bug. *** Created attachment 264430 [details]
patch
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 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 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 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? Created attachment 264466 [details]
patch
Created attachment 264467 [details]
patch
Removed tabs in test files.
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 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 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 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 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 on attachment 264613 [details] patch Clearing flags on attachment: 264613 Committed r191931: <http://trac.webkit.org/changeset/191931> All reviewed patches have been landed. Closing bug. 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> Created attachment 264702 [details]
patch
Skip failing tests on win.
Comment on attachment 264702 [details] patch Clearing flags on attachment: 264702 Committed r191972: <http://trac.webkit.org/changeset/191972> All reviewed patches have been landed. Closing bug. 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. 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!! (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 (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). |