-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[Autocomplete] Fix VoiceOver not reading the correct activedescendant #24213
[Autocomplete] Fix VoiceOver not reading the correct activedescendant #24213
Conversation
… for a11y to appropriately read dropdown labels
Anyone know why my checks are failing? I'm sorry, I'm new at this and clicking details just links me to the circleCI homepage. |
Thanks so much for helping @oliviertassinari , you're great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff is all over the place due to re-ordering which makes it hard to review what exactly changed. Could you revert the re-ordering if it wasn't necessary? Not due to re-ordering but whitespace.
packages/material-ui/src/useAutocomplete/useAutocomplete.test.js
Outdated
Show resolved
Hide resolved
@eps1lon What are you referring to by "re-ordering"? The diff without spaces doesn't seem to show a reordering. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked into the API of Autocomplete
again and I don't think we can use getOptionLabel
here because we also have renderOption
. People could render custom options without implementing getOptionLabel
whose default implementation does not guarantee a string.
describe('useAutocomplete', () => { | ||
const render = createClientRender(); | ||
|
||
it('should use unique keys for the option', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the case before, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, ok, I see the description can be interpreted in two different ways. Maybe?
it('should use unique keys for the option', () => { | |
it('should use a unique and deterministic key for each option', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was also the case before?
The assertions do not match the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe c632040
(#24213) is closer to what we actually want to tell VoiceOver.
Couple of thoughts how to fix the underlying issue:
|
@eps1lon Are you sure? |
We'll see if people notice that. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, smart test case
@inform880 Thanks again for bringing the issue to our attention. |
I switched the key from index to the getOptionLabel function in order for a11y to appropriately read dropdown labels
Closes #24198