-
Notifications
You must be signed in to change notification settings - Fork 21
Description
The README says this.
Markup requirements:
- Each option needs to have role="option" and a unique id
- The list should have role="listbox"
When someone misses this and omits these attributes, screen reader accessibility is silently degraded. When this package is being used as a dependency of @github/text-expander-element, it's especially likely for a developer to miss this documentation and pass in an invalid element.
We had a bug like this over on Forgejo recently and the missing IDs on the combobox options were identified as the root cause.
There's already some code here that seems to accept the possibility that callers might pass in invalid ARIA combobox widgets, and attempts to resolve the most obvious mistake.
Lines 48 to 50 in ce22bbc
| if (!list.id) { | |
| list.id = `combobox-${Math.random().toString().slice(2, 6)}` | |
| } |
Could we consider doing the same for the list items? If we could add a a similar !target.id check before this usage of target.id and either assign it a random ID like the above code does or print a console warning, I think it could have a good positive impact on the screen reader accessibility of applications that consume this package. It's a little easy to mess up and not notice right now and we have an opportunity here to teach people about the importance of these IDs and aria-activedescendant and maybe even get them a little bit stoked about their new accessibility knowledge.
Line 129 in ce22bbc
| this.input.setAttribute('aria-activedescendant', target.id) |
I'm happy to send pull requests for both of these approaches (sanitise by adding a random ID when no iD is present and console.warn when no ID is found) if you think it'd be easier to evaluate the trade-offs with code in front of you. Seemed best to reach out in an issue first anyway. I can't tell at all from where I'm sitting which approach would be preferable.