dequelabs / axe-core

Accessibility engine for automated Web UI testing
https://www.deque.com/axe/
Mozilla Public License 2.0
5.97k stars 776 forks source link

Permit ARIA 1.2 combobox (without textbox child) #2505

Closed stevenyxu closed 3 years ago

stevenyxu commented 4 years ago

Expectation: A compliant ARIA 1.2 (draft) combobox should not raise axe violations.

Actual: ARIA 1.2 combobox raises aria-required-children violation. Demo: https://w3c.github.io/aria-practices/examples/combobox/combobox-select-only.html. Currently raises 1 violation, aria-required-children.

Motivation: ARIA 1.2 is redefining combobox (draft spec) to no longer require a textbox child because of various issues with the 1.1 combobox. Aside from resolving long-standing labelling and SR support issues, this is particularly useful for web applications that want to emulate a native select or provide a rich selection widget (e.g. a form select with a popup that includes multiselection, options filter, select all/none). The current common practice is to use a popup button as the trigger rather than a combobox, but this is problematic in a form setting because button has problems with aria-required (prohibited by spec, flagged by axe, though supported in practice) and aria-invalid (permitted by spec, but not supported in practice). I've tested the new pattern with combobox trigger on JAWS, NVDA, ChromeVox on ChromeOS, VoiceOver, iOS VoiceOver, and Android TalkBack, and they all correctly announce current value, requiredness, and invalidness. In light of the apparent widespread support and that it's in a draft spec, it would be nice to permit it in axe. A couple of projects I'm involved with including Angular Material have already cut over to the new pattern because of the clear a11y wins, and we're currently suppressing (in tooling) and manually ignoring (when using axe extension) the axe violation.


axe-core version: 3.5.5
axe-webdriver, extension or other integration version: 4.5.3

Browser and Assistive Technology versions

For Tooling issues:
- Platform:  Tested on Chrome 84 OS X, reproducible running axe-core as a standalone library on node v10.
straker commented 4 years ago

Thanks for the issue. We've been discussing this very pattern over in #2478. The TL;DR is that we want to support the pattern, but we need to figure out what to do with the old 1.1 pattern first.

stevenyxu commented 4 years ago

Thanks @straker. I took a look at #2478 and just want to understand the 1.1 blocker a bit better. Would it be feasible to support role=combobox aria-controls=some-popup-id now and exempt this node from having a required textbox and/or popup child? Thus axe permits the following (using listbox without option children for simplicity, but would extend to other popups):

From what I can tell, adding support (in the form of not flagging as a violation) for the 1.2 pattern could be done independently of whether the 1.1 pattern should be sunset here. The practice of putting aria-controls on the combobox node itself is new compared to both 1.0 and 1.1. Is there some edge case that prevents this from being feasible?

straker commented 4 years ago

The main blocker is that it will require special casing and an edge case of guessing.

First, which roles require owning other roles is dictated by our aira-roles standards object. In ARIA 1.0 and 1.1, the combobox is required to own a listbox and textbox (with 1.1 also allowing grid, tree, and dialog if used with aria-haspopup). However, in the 1.2 pattern the comboxbox is not required to own anything. We cannot support two different role specs at the same time very easily and we have to figure out what to do there. This is the main blocker and what we mean when we say we have to figure out what to do with the old 1.1 pattern.

Secondly, there is an edge case when the input has both aria-controls and aria-owns. Neither the 1.0 nor 1.2 pattern disallow this (the 1.2 pattern only says authors are strongly recommended to use aria-controls). Knowing which case we should flag that is will be difficult and potentially full of false positives.

If we flag it as a 1.0 pattern then we need to make sure it owns a listbox, if it doesn't we flag it as a missing required-child. But if the author intended the 1.2 pattern it should pass. If the author intended the 1.2 pattern, should we tell them to remove the aria-owns attribute? aria-owns is a global attribute so allowed anywhere but goes against the design of the pattern. There's a few more edge case things we need to figure out.

Hopefully that helps clarify the blocker. We have this planned to be fixed for our 4.1.0 release, so we should be working on it in the coming weeks.

stevenyxu commented 4 years ago

Thanks for the discussion, @straker. That make sense to me. It's unfortunate the singular churn of combobox role makes a static mapping hard for axe. I can imagine creating a different ruleset for [role=combobox][aria-controls] and [role=combobox]:not([aria-controls]) but I'm sure there may be more graceful ways to handle this. Looking forward to seeing what your team comes up with here.

My vote regarding the aria-controls and aria-owns edge case would just be to steer them towards the clean 1.2 pattern, which appears to be widely supported from our testing both when the combobox is a plain popup trigger (in the <select> emulation case) and when it's a true input/contenteditable. If someone has done both atop the 1.0 pattern, it's as easy as removing aria-owns. If someone has done both atop the 1.1 pattern (unlikely because 1.1 already tells you to put aria-controls on the textbox inside), it's a bit more involved, but the mapping to the 1.2 pattern should still be straightforward. If someone has done both atop 1.2, then yeah removing aria-owns seems sensible and easy.

Anyway sounds like your team is on top of it, so thank you and let me know if you need any help or testing from our side.

backwardok commented 3 years ago

Given the poor support for the 1.1 combobox pattern, and the recommendation explicitly in 1.2 to not use the 1.1 pattern, could the combobox check for aria-required-children be removed? I also wonder if a patch version could be made for 3.x to address this issue without requiring updating to 4.x quite yet. In the interim, I have to disable this check altogether, which seems worse. Thanks!

AutoSponge commented 3 years ago

For another example, here's a demo using angular material that gets flagged in axe: https://angular-ph3vth-e514sq.stackblitz.io/. In this example, the listbox is dynamically created (axe complains it doesn't have the right child) then continues to flag in the open state. I assume it's because the listbox isn't the first child (nested in a div). The listbox is directly linked with aria-controls.

If this example is accessible, then the rule aria-required-children is a false positive.

smhigley commented 3 years ago

@straker / @WilcoFiers I did some testing, and I think axe wouldn't require any special casing or version detection to make this work well. I created a few combobox variations of the 1.1 and 1.2 patterns that mix and match having a child input and switching aria-controls for aria-owns: https://codepen.io/smhigley/full/zYoEMWP. (I left off 1.0, because if you switch aria-owns for aria-controls, it just becomes 1.2).

As you can see, there is no practical support problem with removing the child input for the 1.1 pattern, or leaving out aria-owns in favor of aria-controls in the 1.1 pattern. So, for all patterns, the following are true:

It seems like if axe removed the aria-required-children for combobox entirely, and added a check for either aria-owns or aria-controls regardless of pattern version, that should cover all cases.

This makes sense from a technical perspective: the thing doing the heavy lifting is aria-activedescendant, and it doesn't really matter if the combo/listbox association is done via aria-owns or aria-controls. And a 1.1 pattern without a child input is just a select-only 1.2 pattern with aria-owns :)

straker commented 3 years ago

Thanks for all the amazing tests! I talked with @WilcoFiers and we agree that your suggestion seems like the best approach. We'll see about getting this into the 4.2 release.

smhigley commented 3 years ago

Woot, thanks so much! 🎉

WilcoFiers commented 3 years ago

Unfortunately, we had to postpone this one, will take this up in 4.3.

straker commented 3 years ago

Just a heads up. This is going into the 4.3 release happening next week.

padmavemulapati commented 3 years ago

Validated with latest develop branch code base, aria-required-children violation is showing on combobox , able to reproduce it with the old axe-core version(4.0.2) test-script:

<div aria-controls=\"listbox1\" aria-expanded=\"false\" aria-haspopup=\"listbox\" aria-labelledby=\"combo1-label\" id=\"combo1\" class=\"combo-input\" role=\"combobox\" tabindex=\"0\">Choose a Fruit</div>

image

and now it got fixed: not seeing violations on aria-required-children on combobox image

vskh commented 3 years ago

Seems, this recent update might have changed some other aspects of combobox validation in a way that was not a problem before.

Suddenly, Fluent UI's dropdowns started to be highlighted with 'aria-required-children' rule based on missing 'aria-controls' attribute in collapsed state.

If I understand correctly the part of WAI-ARIA that the rule refers to in part applicable to combobox, it mentions that

When a combobox is expanded, authors MUST ensure it contains or owns an element that has a role of listbox, tree, grid, or dialog. This element is the combobox popup. When the combobox is expanded, authors MUST set aria-controls on the textbox element to a value that refers to the combobox popup element.

i.e. 'aria-controls' is only required when the combobox is in expanded state (which mentioned dropdown implementation complies with, as the listbox part is dynamically inserted and not present in the DOM before expansion). There are no other mentions of these attributes with regards to behavior in a collapsed state. But axe-core requires those attributes unconditionally.

I'd like to ask if my understanding is correct or am I missing something? Once confirmed, I'd be happy to work on a patch.

straker commented 3 years ago

Thanks for letting us know. It does indeed look like we missed that as part of the implementation. Would you mind opening a new issue so we can track it?

vskh commented 3 years ago

Ooops, sorry, did not see you've created one and opened https://github.com/dequelabs/axe-core/issues/3091. Will close as a dupe.