Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ember-power-select screen reader accessibility #1481

Merged
merged 5 commits into from
Nov 22, 2021

Conversation

calvin-fb
Copy link

@calvin-fb calvin-fb commented Nov 18, 2021

The current implementation doesn't appear to work well with screen readers. Specifically, screen readers are unable to recognize and read out the options because of missing aria attributes.

This PR fixes a few issues from the previous checklist created by someone else "[a11y] Refine ARIA implementation": #806

It closes these two issues from that list:

  • Implement aria-activedescendant on the combobox. This property allows focus to stay on the trigger but tells the screen reader which option in the listbox is currently highlighted.
  • Remove aria-controls from ul elements inside the listbox. (This relationship was backwards. The trigger controls the options list, not the other way around.)

I also compared ember-power-select to the reference implementation provided by the W3C. In my testing, this reference implementation works very well with screen readers: https://w3c.github.io/aria-practices/examples/combobox/combobox-select-only.html

This PR by itself doesn't fix all screen reader issues. This is merely a step in the right direction. For one, users will have to pass in @triggerRole="combobox".

@aabzhanova
Copy link

👍

@calvin-fb calvin-fb force-pushed the accessibility-improvements branch 4 times, most recently from f7c8243 to e361ce4 Compare November 19, 2021 15:38
@@ -413,7 +421,7 @@ export default class PowerSelect extends Component<PowerSelectArgs> {
if (this.args.scrollTo) {
return this.args.scrollTo(option, select);
}
let optionsList = document.querySelector(`[aria-controls="ember-power-select-trigger-${select.uniqueId}"]`) as HTMLElement;
let optionsList = document.querySelector(`#ember-power-select-options-${select.uniqueId}`) as HTMLElement;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If querying for an ID, maybe use document.getElementById

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 👍

@cibernox
Copy link
Owner

Hey, this looks great!
Only one question comes to mind: I assume aria-owns is set to a blank string to be sure it overrides the default aria-owns in the underlying ember-basic dropdown. Is aria-owns incorrect in every case or only for selects? I wonder because maybe if it's a bad pattern that could be fixed in ember-basic-dropdown instead and both addons would improve.

@calvin-fb
Copy link
Author

Only one question comes to mind: I assume aria-owns is set to a blank string to be sure it overrides the default aria-owns in the underlying ember-basic dropdown. Is aria-owns incorrect in every case or only for selects? I wonder because maybe if it's a bad pattern that could be fixed in ember-basic-dropdown instead and both addons would improve.

I'm actually not sure. Personally I haven't seen any issues with using aria-owns for ember-basic-dropdown but I'm no expert. I figured it was less risky to just change it here for now.

@cibernox
Copy link
Owner

It is less risky indeed and if we end up making that change in ember-basic-dropdown having this override would be unnecessary but not harmful, so I'm good with merging this as is. I'll check if any of the CI errors seems legit, but their are because the addon has some issues with Ember 4.0

@calvin-fb
Copy link
Author

@cibernox Apologies, I was doing some testing and realized that more changes needed to be made. I've moved the aria properties from the trigger to the searchbox when searchEnabled is true. The way I had it before worked well when the search wasn't present but when the search was visible it got a little confused. I've added a new test for this.

@cibernox
Copy link
Owner

Cool. I just ran the test suite locally and i found no errors. If the test suite is also green after these last changes I'll merged it tonight

@cibernox cibernox merged commit 742200b into cibernox:master Nov 22, 2021
@cibernox
Copy link
Owner

Thanks for this! I don't know about jaws but in voice over it's so much better. I'll release a new version

@cibernox
Copy link
Owner

Merged. I'll let it sit in master for a few days in case you find another edge case. If after a few days you don't find any other problem I'll release 5.0 (I don't think this will be a breaking change in practice for virtually anyone, but I prefer to bump one mayor version to be sure)

@calvin-fb
Copy link
Author

Awesome, thanks @cibernox!

@aabzhanova
Copy link

@cibernox thank you so much Miguel for getting this reviewed and merged quickly! 🎉 We really appreciate that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants