Bug 255371

<rdar://problem/107962942>

Created attachment 465874 [details]
Patch

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

Created attachment 465896 [details]
Patch

(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.

(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.

(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?

Created attachment 465899 [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?
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!

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].