[Webkit-unassigned] [Bug 138566] AX: Add support for ARIA 1.1 attribute 'aria-modal' for dialog and alertdialog

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=138566

--- Comment #12 from chris fleizach <cfleizach at apple.com> ---
Comment on attachment 264467
  --> https://bugs.webkit.org/attachment.cgi?id=264467
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151102/fc1506d1/attachment.html>