| Summary: | AX: nested role="presentation" elements break role="tree" behavior | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sufian Rhazi <srhazi> | ||||||||||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, spanage, todd.kloots, tyler_w, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | Safari 14 | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | macOS 11 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Sufian Rhazi
2021-07-13 08:46:28 PDT
Confirmed that this still reproduces on 13.1 Beta (22C5044e) Created attachment 464114 [details]
Patch
Created attachment 464122 [details]
Patch
Comment on attachment 464122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464122&action=review > LayoutTests/accessibility/mac/nested-tree-presentation-roles.html:40 > + window.jsTestIsAsync = true; This test shouldn't need to be async because there is no JS causing dynamic changes. We like to avoid making tests async when possible because doing so unnecessarily can mask issues in ITM. Created attachment 464142 [details]
Patch
Comment on attachment 464142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464142&action=review Sorry for missing these nits on the last round of review. > COMMIT_MESSAGE:7 > +Trees can have arbitrary nested of presentational roles. Our checks before were too strict. "Trees can have arbitrary nested of presentational roles." The grammar is a little off here. Maybe it could read something like: Trees can validly have arbitrary levels of nesting of role="presentational" elements. > LayoutTests/accessibility/mac/nested-tree-presentation-roles.html:41 > + var slider = accessibilityController.accessibleElementById("item1"); This variable isn't ever used in the test. > LayoutTests/accessibility/mac/nested-tree-presentation-roles.html:43 > + output += expect("accessibilityController.accessibleElementById('item1').boolAttributeValue('AXSelected')", "false"); Can probably remove all newlines between this one and "if (window.accessibilityController) {" > > COMMIT_MESSAGE:7
> > +Trees can have arbitrary nested of presentational roles. Our checks before were too strict.
>
> "Trees can have arbitrary nested of presentational roles."
>
> The grammar is a little off here. Maybe it could read something like: Trees
> can validly have arbitrary levels of nesting of role="presentational"
> elements.
Or maybe just "Trees can have arbitrary nesting of presentational roles"
Created attachment 464222 [details]
Patch
(In reply to chris fleizach from comment #9) > Created attachment 464222 [details] > Patch diff --git a/LayoutTests/accessibility/mac/nested-tree-presentation-roles.html b/LayoutTests/accessibility/mac/nested-tree-presentation-roles.html new file mode 100644 index 000000000000..6d83d363fa0a --- /dev/null +++ b/LayoutTests/accessibility/mac/nested-tree-presentation-roles.html + output += expect("accessibilityController.accessibleElementById('item1').boolAttributeValue('AXSelected')", "false"); This attribute can return a false value even if it fails to retrieve the value from the underlying AX object.: bool AccessibilityUIElement::boolAttributeValue(NSString *attribute) const { BEGIN_AX_OBJC_EXCEPTIONS auto value = attributeValue(attribute); if ([value isKindOfClass:[NSNumber class]]) return [value boolValue]; END_AX_OBJC_EXCEPTIONS return false; } To make it a stricter test, we can make aria-checked="true" for item1 and check for that instead. You can get rid of all the class and tabindex attributes that are not used. Created attachment 464322 [details]
Patch
Committed 258435@main (3f6b03504654): <https://commits.webkit.org/258435@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 464322 [details]. Many thanks to you Chris, Tyler and Andres for getting this one in! ~S Verified fixed. Tested using Mac OS 14.1.1 in both Chrome (Version 119.0.6045.199) and Safari (Version 17.1 (19616.2.9.11.7)). Tested using a forked version of Sufian's original example (https://codepen.io/toddlr/pen/gOqBmaq) as it makes it far easier to confirm checked state is reported correctly by VO. |