Bug 255371 – Make AXCoreObject::visibleChildren, AXCoreObject::isPresentationalChildOfAriaRole, and AXCoreObject::ariaRoleHasPresentationalChildren work for display:contents elements

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

[Tyler Wilcock]

None of these methods inherently require a renderer, but are still only implemented for AccessibilityRenderObject.

[Radar WebKit Bug Importer]

<rdar://problem/107962942>

[Tyler Wilcock]

Created attachment 465874 [details]
Patch

[chris fleizach]

Comment on attachment 465874 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityListBox.cpp:128
> +    for (unsigned i = 0; i < m_children.size(); i++) {

can we cache size() so we don't call it repeatedly on each iteration

[Tyler Wilcock]

Created attachment 465896 [details]
Patch

[Tyler Wilcock]

(In reply to chris fleizach from comment #3)
> Comment on attachment 465874 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465874&action=review
> 
> > Source/WebCore/accessibility/AccessibilityListBox.cpp:128
> > +    for (unsigned i = 0; i < m_children.size(); i++) {
> 
> can we cache size() so we don't call it repeatedly on each iteration
Fixed, thanks.

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Comment on attachment 465874 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465874&action=review
> 
> > Source/WebCore/accessibility/AccessibilityListBox.cpp:128
> > +    for (unsigned i = 0; i < m_children.size(); i++) {
> 
> can we cache size() so we don't call it repeatedly on each iteration

No need to do that, size() is an inline accessor in std-like containers like Vector.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Created attachment 465896 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp

+AXCoreObject::AccessibilityChildrenVector AccessibilityNodeObject::visibleChildren()
+{
+    // Only listboxes are asked for their visible children.
+    // Native list boxes would be AccessibilityListBoxes, so only check for aria list boxes.
+    if (ariaRoleAttribute() != AccessibilityRole::ListBox)
+        return { };

Can we unify native and ARIA list boxes in the AXListbox class?

+    for (const auto& child : children()) {
+        if (child->isOffScreen())

isOffScreen() ? I would think that visible child would be on screen.

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp

+bool AccessibilityObject::isPresentationalChildOfAriaRole() const
+{
+    // Walk the parent chain looking for a parent that has presentational children
+    AccessibilityObject* parent;
+    for (parent = parentObject(); parent && !parent->ariaRoleHasPresentationalChildren(); parent = parent->parentObject())
+    { }

Can we do this with findAncestor?

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp

-    AccessibilityChildrenVector visibleChildren;
-    object.visibleChildren(visibleChildren);
-    setObjectVectorProperty(AXPropertyName::VisibleChildren, visibleChildren);
+    setObjectVectorProperty(AXPropertyName::VisibleChildren, object.visibleChildren());

Since this is used only for list boxes, should we just cache it in that case? Or maybe better, only if it is not an empty Vector?

[Tyler Wilcock]

Created attachment 465899 [details]
Patch

[Tyler Wilcock]

> --- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> 
> +AXCoreObject::AccessibilityChildrenVector
> AccessibilityNodeObject::visibleChildren()
> +{
> +    // Only listboxes are asked for their visible children.
> +    // Native list boxes would be AccessibilityListBoxes, so only check for
> aria list boxes.
> +    if (ariaRoleAttribute() != AccessibilityRole::ListBox)
> +        return { };
> 
> Can we unify native and ARIA list boxes in the AXListbox class?
ARIA list boxes may not have a renderer, while AccessibilityListBox is a subclass of AccessibilityRenderObject, so I don't think so. We could consider removing the AccessibilityListBox implementation in the future, but let's save that for a future patch.

> +    for (const auto& child : children()) {
> +        if (child->isOffScreen())
> 
> isOffScreen() ? I would think that visible child would be on screen.
Wow, yeah, this was totally wrong. It's interesting because we don't have any layout tests covering this attribute for listboxes (ARIA or native). And after adding some logging, VoiceOver doesn't seem to request it for ARIA or native listboxes. Maybe we can consider removing it entirely in a future patch (pending more investigation of how it's used).

> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> 
> -    AccessibilityChildrenVector visibleChildren;
> -    object.visibleChildren(visibleChildren);
> -    setObjectVectorProperty(AXPropertyName::VisibleChildren,
> visibleChildren);
> +    setObjectVectorProperty(AXPropertyName::VisibleChildren,
> object.visibleChildren());
> 
> Since this is used only for list boxes, should we just cache it in that
> case? Or maybe better, only if it is not an empty Vector?
Added a check for isListBox(). Not caching in the case of an empty Vector should be accomplished by a future patch where we don't cache any property of type T if it is equal to T().

Addressed all other review comments. Please re-review when you get a moment. Thanks!

[EWS]

Committed 263025@main (ee7f5c504b70): <https://commits.webkit.org/263025@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465899 [details].