Bug 229415 – AX: Return radiobuttons part of ad-hoc radiogroups from AX search queries

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

[Tyler Wilcock]

Per WAI-ARIA authoring best practices, radiobuttons should always be wrapped in a `role="radiogroup"` container.

https://www.w3.org/TR/wai-aria-practices-1.2/#wai-aria-roles-states-and-properties-16

> “The radio buttons are contained in or owned by an element with role radiogroup.”

However, many real-world documents don't do this, instead forming "ad-hoc" radiogroups by connecting individual radiobuttons by `name` attribute (which is allowed by HTML):

<form>
<input type="radio" name="color” value="red">Red</br>
<input type="radio" name="color” value="blue">Blue</br>
</form>

<form>
<input type="radio" name="size” value="small">Small</br>
<input type="radio" name="size” value="large">Large</br>
</form>

When searching by AXRadioGroupSearchKey, WebKit currently cannot find these ad-hoc radiogroups. With this bug, we should make WebKit able to find them.

rdar://38830789

[Radar WebKit Bug Importer]

<rdar://problem/82256435>

[Tyler Wilcock]

Created attachment 436247 [details]
Patch

[Andres Gonzalez]

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

--- a/Source/WebCore/ChangeLog
+++ a/Source/WebCore/ChangeLog

+        connected only by `name` attribute, missing an appropriate role="radiogroup"
+        wrapper.
I would use the word container or parent instead of wrapper, since wrapper in this context usually refers to the AXObject platform wrapper, e.g., WebAccessibilityObjectWrapper.

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp

+static bool objectIsRadioButtonInDifferentGroup(AXCoreObject* axObject, AXCoreObject* referenceObject)

objectIsRadioButtonInDifferentGroup -> areRadioButtonsInDifferentGroups or areRadioButtonsInSameGroup ???

+    return axObject->element()->getNameAttribute() != referenceObject->element()->getNameAttribute();

What about if they have a radiogroup container? If this is just for ad-hoc groups, let's make it explicit in the function name.

-static bool isAccessibilityObjectSearchMatch(AXCoreObject* axObject, AccessibilitySearchCriteria const& criteria)
+static bool isAccessibilityObjectSearchMatch(AXCoreObject* axObject, AccessibilitySearchCriteria const& criteria, AccessibilitySearchContext const& context)
Can we make the starting object a member of AccessibilitySearchCriteria instead of adding a new struct?

[chris fleizach]

Comment on attachment 436247 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:3654
> +    if (!axObject->element() || !referenceObject->element())

we can probably combine these checks into the ones above and save this extra check

however, are we guaranteed to have an element() if it's already a radio button?

> Source/WebCore/accessibility/AccessibilityObject.cpp:3870
> +    AccessibilitySearchContext context { startObject };

should (or is) startObject already part of the searchCriteria?

[Andres Gonzalez]

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

--- a/LayoutTests/ChangeLog
+++ a/LayoutTests/ChangeLog

+        connected only by `name` attributes, missing an appropriate role="radiogroup"
+        wrapper.

wrapper -> container or parent

+        * accessibility/mac/search-predicate-for-adhoc-radio-groups-expected.txt: Added.
+        * accessibility/mac/search-predicate-for-adhoc-radio-groups.html: Added.
+

Shouldn't we have an iOS test? on iOS we have the following method in the WebAccessibilityObjectWrapper interface:

- (NSArray<WebAccessibilityObjectWrapper *> *)accessibilityFindMatchingObjects:(NSDictionary *)parameters

--- a/LayoutTests/accessibility/mac/search-predicate-for-adhoc-radio-groups.html
+++ a/LayoutTests/accessibility/mac/search-predicate-for-adhoc-radio-groups.html

+<script src="../../resources/js-test-pre.js"></script>

I think we can now include just js-test.js, and we don't need the -pre and the -post.

+<p id="description"></p>
+<div id="console"></div>

Don't need these two lines any more if you include js-test.js.

+        shouldBe("resultElement.stringAttributeValue('AXDOMIdentifier')", "'circle'");

Instead of resultElement.stringAttributeValue('AXDOMIdentifier') you can do just resultElement.domIdentifier.

+<script src="../../resources/js-test-post.js"></script>

Don't need this any more if you include js-test.js.

Are we covering the case of radiogroup containers in a separate test?

[Tyler Wilcock]

Thanks for the review!

> objectIsRadioButtonInDifferentGroup -> areRadioButtonsInDifferentGroups or areRadioButtonsInSameGroup ???

The semantics of this function can't be the same as what one would think with `areRadioButtonsInDifferentGroups`, as that name implies the function returns false if either object is not a radio button, but we want to return true if the `referenceObject` is not a radio button and `axObject` is.

I'll go with isRadioButtonInDifferentAdhocGroup for now, but if you have other ideas let me know.

> however, are we guaranteed to have an element() if it's already a radio button?

Yeah, that seems reasonable. I'll remove the check.

> Are we covering the case of radiogroup containers in a separate test?

accessibility/mac/search-predicate.html tests a search for role=radiogroup containers:

https://github.com/WebKit/WebKit/blob/989ff515ce6e103271072dd1b397ac43572a910c/LayoutTests/accessibility/mac/search-predicate.html#L225

Is that satisfactory, or are you looking for something here too?

[Tyler Wilcock]

Created attachment 436303 [details]
Patch

[chris fleizach]

Comment on attachment 436303 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        * accessibility/mac/search-predicate-for-adhoc-radio-groups-expected.txt: Added.

is this a Mac specific test? I don't think the uiSearch exists for any platform

should we move just into the Mac directory?

[Andres Gonzalez]

(In reply to chris fleizach from comment #8)
> Comment on attachment 436303 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436303&action=review
> 
> > LayoutTests/ChangeLog:13
> > +        * accessibility/mac/search-predicate-for-adhoc-radio-groups-expected.txt: Added.
> 
> is this a Mac specific test? I don't think the uiSearch exists for any
> platform
> 
> should we move just into the Mac directory?

On iOS we have:

- (NSArray<WebAccessibilityObjectWrapper *> *)accessibilityFindMatchingObjects:(NSDictionary *)parameters

[chris fleizach]

(In reply to Andres Gonzalez from comment #9)
> (In reply to chris fleizach from comment #8)
> > Comment on attachment 436303 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=436303&action=review
> > 
> > > LayoutTests/ChangeLog:13
> > > +        * accessibility/mac/search-predicate-for-adhoc-radio-groups-expected.txt: Added.
> > 
> > is this a Mac specific test? I don't think the uiSearch exists for any
> > platform
> > 
> > should we move just into the Mac directory?
> 
> On iOS we have:
> 
> - (NSArray<WebAccessibilityObjectWrapper *>
> *)accessibilityFindMatchingObjects:(NSDictionary *)parameters

Does this test pass on iOS?

[Tyler Wilcock]

(In reply to chris fleizach from comment #8)
> Comment on attachment 436303 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436303&action=review
> 
> > LayoutTests/ChangeLog:13
> > +        * accessibility/mac/search-predicate-for-adhoc-radio-groups-expected.txt: Added.
> 
> is this a Mac specific test? I don't think the uiSearch exists for any
> platform
> 
> should we move just into the Mac directory?

Sorry, the Changelog is stale — Andres suggested we also test this on iOS, so I moved it out of the Mac folder. There is this function in the AccessibilityUIElementIOS class:

RefPtr<AccessibilityUIElement> AccessibilityUIElement::uiElementForSearchPredicate(JSContextRef context, AccessibilityUIElement *startElement, bool isDirectionNext, JSValueRef searchKey, JSStringRef searchText, bool visibleOnly, bool immediateDescendantsOnly)
{
    NSDictionary *parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 5, searchKey, searchText, visibleOnly, immediateDescendantsOnly);
    ...
}

However, I just realized that this search is hardcoded to return 5 matching elements, vs. the same function in AccessibilityUIElementMac which returns only 1 matching element, so this won't work without parameterizing that value...

[Tyler Wilcock]

Correction: however this function is generated (it's not checked into source), it's getting a value of 5 for number of matching elements to return.

[chris fleizach]

(In reply to Tyler Wilcock from comment #12)
> Correction: however this function is generated (it's not checked into
> source), it's getting a value of 5 for number of matching elements to return.

maybe we can have different expectation files

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #11)
> (In reply to chris fleizach from comment #8)
> > Comment on attachment 436303 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=436303&action=review
> > 
> > > LayoutTests/ChangeLog:13
> > > +        * accessibility/mac/search-predicate-for-adhoc-radio-groups-expected.txt: Added.
> > 
> > is this a Mac specific test? I don't think the uiSearch exists for any
> > platform
> > 
> > should we move just into the Mac directory?
> 
> Sorry, the Changelog is stale — Andres suggested we also test this on iOS,
> so I moved it out of the Mac folder. There is this function in the
> AccessibilityUIElementIOS class:
> 
> RefPtr<AccessibilityUIElement>
> AccessibilityUIElement::uiElementForSearchPredicate(JSContextRef context,
> AccessibilityUIElement *startElement, bool isDirectionNext, JSValueRef
> searchKey, JSStringRef searchText, bool visibleOnly, bool
> immediateDescendantsOnly)
> {
>     NSDictionary *parameterizedAttribute =
> searchPredicateParameterizedAttributeForSearchCriteria(context,
> startElement, isDirectionNext, 5, searchKey, searchText, visibleOnly,
> immediateDescendantsOnly);
>     ...
> }
> 
> However, I just realized that this search is hardcoded to return 5 matching
> elements, vs. the same function in AccessibilityUIElementMac which returns
> only 1 matching element, so this won't work without parameterizing that
> value...

I would suggest making AccessibilityUIElement::uiElementForSearchPredicate to have the same signature in both Mac and iOS, so that a single test can be written.

[Tyler Wilcock]

> I would suggest making AccessibilityUIElement::uiElementForSearchPredicate
> to have the same signature in both Mac and iOS, so that a single test can be
> written.

OK, I actually should be able to do this. Will submit a new patch.

[Tyler Wilcock]

Created attachment 436309 [details]
Patch

[Tyler Wilcock]

Created attachment 436310 [details]
Patch

[Tyler Wilcock]

Created attachment 436322 [details]
Patch

[Tyler Wilcock]

Created attachment 436324 [details]
Patch

[Tyler Wilcock]

`domIdentifier` isn't implemented for DumpRenderTree UIAccessibilityElements. I'm going to try to add support for it, but if it's too finicky then I'll go back to stringAttributeValue('AXDOMIdentifier') which is supported in both cases.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #20)
> `domIdentifier` isn't implemented for DumpRenderTree
> UIAccessibilityElements. I'm going to try to add support for it, but if it's
> too finicky then I'll go back to stringAttributeValue('AXDOMIdentifier')
> which is supported in both cases.

you may also skip the test for Mac-wk1.

[Tyler Wilcock]

Created attachment 436339 [details]
Patch

[Tyler Wilcock]

Created attachment 436340 [details]
Patch

[EWS]

Committed r281556 (240923@main): <https://commits.webkit.org/240923@main>

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