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

Fix a11y issues with inputs #1306

Merged
merged 4 commits into from
Sep 22, 2022

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Dec 4, 2019

This PR fixes (or at least allows to fix) lingering accessibility issues reported by axe (via ember-a11y-testing):

It builds on top of #1305 - I can change that, if required.

With these changes, I could get axe to pass - as long as I added a label, like this:

<label id='my-label'>Label here</label>
<PowerSelect
  @ariaLabelledBy='my-label'
  ...
>

IMHO it would be nice to add this somewhere to the docs, maybe to the cookbook? But that is a separate issue :)

@mydea mydea force-pushed the fix-multi-input-label branch from 8cc0ffc to 2988db7 Compare February 17, 2020 13:36
@mydea
Copy link
Contributor Author

mydea commented Feb 25, 2020

Anything I can do to help move this along (or change)? Would love to see this land :)

@mydea mydea force-pushed the fix-multi-input-label branch from 2988db7 to fa3ed70 Compare May 18, 2020 10:17
@MarcoZehe
Copy link

I'd also be interested in seeing this merged. Ghost uses this component and has some serious issues with keyboard navigation when the PowerSelect is open, and labeling. @cibernox can we work together to move this along? Also with #1305 .

@mydea mydea force-pushed the fix-multi-input-label branch from fa3ed70 to f3dacf9 Compare May 28, 2020 09:08
@mydea mydea force-pushed the fix-multi-input-label branch from f3dacf9 to 19adde4 Compare July 3, 2020 07:10
@mydea
Copy link
Contributor Author

mydea commented Jul 3, 2020

I have updated this a bit, also adding id, aria-label/aria-labelledby to all "empty" listboxes (when showing the search or empty message).

@backspace
Copy link

Thanks for opening this, I skipped an accessibility audit in tests that use Power Select for now because it seemed like it would involve too many template overrides. I know @cibernox is always busy but let us know if there’s any way we can help this move along 💞

@mydea mydea force-pushed the fix-multi-input-label branch from b70d72b to 9dec058 Compare October 12, 2020 08:04
@mydea mydea force-pushed the fix-multi-input-label branch from 9dec058 to 24f9f40 Compare November 10, 2020 07:59
@RobbieTheWagner
Copy link
Collaborator

@mydea would you be able to rebase this and fix the conflicts please?

@mydea
Copy link
Contributor Author

mydea commented Sep 5, 2022

sorry, took some time, but managed to rebase it.

FYI, there are still some major accessibility issues, but they are harder to fix... The main problem I see is that there are nested interactive elements in the multi select (the trigger is interactive, and then inside of it there are both the "x" remove buttons as well as the search input).

IMHO the way to "solve" this is to refactor the multi-select to also have the search in the dropdown, similar to the single select. But this is of course a larger change. I wrote some thoughts together here: #1539

@RobbieTheWagner
Copy link
Collaborator

@mydea I merged #1305 so unfortunately you will need to rebase this again. Sorry! Do we need to label any of this stuff as breaking changes?

mydea and others added 4 commits September 7, 2022 15:16
Also remove `type="listbox"` from search input, as that is redundant and axe complains about it.
Otherwise, axe complains that the aria-controls element it refers to doesn't exist.
Otherwise, the input has no label.
This also removes the role="combobox" from the input, as that is redundant for input type="search" and axe also complains about it.
@mydea mydea force-pushed the fix-multi-input-label branch from 2f35484 to 04e58eb Compare September 7, 2022 13:16
@mydea
Copy link
Contributor Author

mydea commented Sep 8, 2022

I rebased, and I think this should be backwards compatible, as it only adds/remove some attributes, but doesn't change any structure.

@RobbieTheWagner RobbieTheWagner merged commit ac29f10 into cibernox:master Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants