Bug 239848

<rdar://problem/92455544>

Created attachment 458523 [details]
Patch

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

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()"

Created attachment 458604 [details]
Patch

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

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

Created attachment 458607 [details]
Patch

Fix for iOS build.

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

Created attachment 458618 [details]
Patch

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

Created attachment 458639 [details]
Patch

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