Bug 239848 – Code cleanup in preparation for refactoring of computing relationships between AXObjects.

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

[Andres Gonzalez]

Code cleanup in preparation for refactoring of computing relationships between AXObjects.

[Radar WebKit Bug Importer]

<rdar://problem/92455544>

[Andres Gonzalez]

Created attachment 458523 [details]
Patch

[Tyler Wilcock]

Comment on attachment 458523 [details]
Patch

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

Seems like a really nice improvement!

> Source/WebCore/accessibility/AccessibilityObject.cpp:4018
> +    if (!labelFor.size())

Could this be labelFor.isEmpty()?

[chris fleizach]

Comment on attachment 458523 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:3953
> +AXCoreObject::AccessibilityChildrenVector AccessibilityObject::activeDescendantOf() const

this feels like it should be plural
activeDescendantsOf

> Source/WebCore/accessibility/AccessibilityObject.h:307
> +    AccessibilityChildrenVector descriptionFor() const override;

I don't feel like these methods indicate they're returning elements. descriptionFor() sounds like it could be a string returning function

can we call it something like "descriptionElementsFor()" or "errorMessageElementsFor()"

[Andres Gonzalez]

Created attachment 458604 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 458523 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458523&action=review
> 
> Seems like a really nice improvement!
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:4018
> > +    if (!labelFor.size())
> 
> Could this be labelFor.isEmpty()?

Yes, using isEmpty() in more places where it makes the code more legible.

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 458523 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458523&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:3953
> > +AXCoreObject::AccessibilityChildrenVector AccessibilityObject::activeDescendantOf() const
> 
> this feels like it should be plural
> activeDescendantsOf

Renamed to activeDescendantOfObjects. ActiveDescendant is singular because it refers to this object, and the method returns the objects that "contain" this as their active descendant. Hopefully the naming now is a bit more clear. Let me know.

> 
> > Source/WebCore/accessibility/AccessibilityObject.h:307
> > +    AccessibilityChildrenVector descriptionFor() const override;
> 
> I don't feel like these methods indicate they're returning elements.
> descriptionFor() sounds like it could be a string returning function
> 
> can we call it something like "descriptionElementsFor()" or
> "errorMessageElementsFor()"

Chose Objects as part of the name to avoid confusion with DOM Elements. Notice the symmetric methods for a given relationship. For instance, we have labelledByObjects and labelForObjects. Hopefully this is a bit more clear now. Let me know. Thanks.

[Andres Gonzalez]

Created attachment 458607 [details]
Patch

Fix for iOS build.

[Tyler Wilcock]

Comment on attachment 458607 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1084
> +    linkedUIElements = flowToObjects();

Will this overwrite existing elements in `linkedUIElements`? If so, should we concat these vectors instead?

> Source/WebCore/accessibility/AccessibilityTableRow.cpp:154
> +    if (ownedObjects.size()) {

Would this be better as !ownedObjects.isEmpty()?

[Andres Gonzalez]

Created attachment 458618 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #9)
> Comment on attachment 458607 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458607&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1084
> > +    linkedUIElements = flowToObjects();
> 
> Will this overwrite existing elements in `linkedUIElements`? If so, should
> we concat these vectors instead?

It is never used to concat linked objects. So while at it, also removed the out parameter and renamed to linkedObjects() like all the other relations methods.

> 
> > Source/WebCore/accessibility/AccessibilityTableRow.cpp:154
> > +    if (ownedObjects.size()) {
> 
> Would this be better as !ownedObjects.isEmpty()?

In this case I think either one is fine, but I opted for the shortest with no negation.

[Andres Gonzalez]

Created attachment 458639 [details]
Patch

[EWS]

Committed r293650 (250154@main): <https://commits.webkit.org/250154@main>

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