Bug 232130 – AX: AccessibilityObject::m_haveChildren and AXCoreObject::hasChildren() are misleadingly named

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

[Tyler Wilcock]

The names of both AccessibilityObject::m_haveChildren and AXCoreObect::hasChildren() imply that the given object has children, i.e. has one or more children. However, what these really mean is whether the object has tried to initialize its children. Both m_haveChildren and hasChildren() can be true for objects that have no children, which is confusing.

[Radar WebKit Bug Importer]

<rdar://problem/84534428>

[Tyler Wilcock]

Created attachment 442112 [details]
Patch

[chris fleizach]

Comment on attachment 442112 [details]
Patch

Do we need to actually store this value for isolated tree mode? Seems like we can just return true for this since it would always be initialized

[Tyler Wilcock]

Created attachment 442120 [details]
Patch

[Tyler Wilcock]

(In reply to chris fleizach from comment #3)
> Comment on attachment 442112 [details]
> Patch
> 
> Do we need to actually store this value for isolated tree mode? Seems like
> we can just return true for this since it would always be initialized
Good call, I just looked into it and I don't think we do. Uploaded a new patch.

[chris fleizach]

Comment on attachment 442120 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3493
> +    if (role == AccessibilityRole::SVGRoot && !childrenInitialized())

this looks like an incorrect usage due to poor naming. I think we want to return role is Image if it doesn't have children, not that it wasn't initialized

[Tyler Wilcock]

Created attachment 442164 [details]
Patch

[Tyler Wilcock]

(In reply to chris fleizach from comment #6)
> Comment on attachment 442120 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442120&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3493
> > +    if (role == AccessibilityRole::SVGRoot && !childrenInitialized())
> 
> this looks like an incorrect usage due to poor naming. I think we want to
> return role is Image if it doesn't have children, not that it wasn't
> initialized
Agreed — fixed with newest patch.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 442164 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Tyler Wilcock]

Created attachment 442314 [details]
Patch

[EWS]

Committed r284769 (243478@main): <https://commits.webkit.org/243478@main>

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

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/AccessibilitySpinButton.cpp
+++ a/Source/WebCore/accessibility/AccessibilitySpinButton.cpp
@@ -45,9 +45,9 @@ AccessibilitySpinButton::~AccessibilitySpinButton() = default;

 AXCoreObject* AccessibilitySpinButton::incrementButton()
 {
-    if (!m_haveChildren)
+    if (!m_childrenInitialized)
         addChildren();
-    if (!m_haveChildren)
+    if (!m_childrenInitialized)
         return nullptr;

Does this second check for m_childrenInitialized even make sense? Since addChildren should set it to true by definition.

Same for AccessibilitySpinButton::decrementButton.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #12)
> (In reply to Tyler Wilcock from comment #10)
> > Created attachment 442314 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilitySpinButton.cpp
> +++ a/Source/WebCore/accessibility/AccessibilitySpinButton.cpp
> @@ -45,9 +45,9 @@ AccessibilitySpinButton::~AccessibilitySpinButton() =
> default;
> 
>  AXCoreObject* AccessibilitySpinButton::incrementButton()
>  {
> -    if (!m_haveChildren)
> +    if (!m_childrenInitialized)
>          addChildren();
> -    if (!m_haveChildren)
> +    if (!m_childrenInitialized)
>          return nullptr;
> 
> Does this second check for m_childrenInitialized even make sense? Since
> addChildren should set it to true by definition.
> 
> Same for AccessibilitySpinButton::decrementButton.
Seems like this guards against the case where a caller used `incrementButton` without a cache active.

void AccessibilitySpinButton::addChildren()
{
    AXObjectCache* cache = axObjectCache();
    if (!cache)
        return;
       
    m_childrenInitialized = true;
    ...
}

This extra check was introduced by this bug: https://bugs.webkit.org/show_bug.cgi?id=157830

[Darin Adler]

Renaming is a good idea.

I notice that the name childrenInitialized() is ambiguous. Could be the name for function that is called when children are initialized, not necessarily a predicate that answers the question of whether the children are initialized. This ambiguity is why Cocoa naming encourages names like areChildrenInitialized().

[Tyler Wilcock]

(In reply to Darin Adler from comment #14)
> Renaming is a good idea.
> 
> I notice that the name childrenInitialized() is ambiguous. Could be the name
> for function that is called when children are initialized, not necessarily a
> predicate that answers the question of whether the children are initialized.
> This ambiguity is why Cocoa naming encourages names like
> areChildrenInitialized().
Yeah, fair point. areChildrenInitialized() makes sense to me. I can create a follow-up patch.