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.
<rdar://problem/84534428>
Created attachment 442112 [details] Patch
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
Created attachment 442120 [details] Patch
(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.
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
Created attachment 442164 [details] Patch
(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.
Tools/Scripts/svn-apply failed to apply attachment 442164 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 442314 [details] Patch
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].
(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.
(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
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().
(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.