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.

Sign up for GitHub

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

Jump to bottom

[Autocomplete] Fix VoiceOver not reading the correct activedescendant #24213

Merged
merged 5 commits into from Jan 1, 2021
Merged

[Autocomplete] Fix VoiceOver not reading the correct activedescendant #24213

merged 5 commits into from Jan 1, 2021

Conversation

inform880
Copy link
Contributor

@inform880 inform880 commented Dec 31, 2020 •
edited by mbrookes

I switched the key from index to the getOptionLabel function in order for a11y to appropriately read dropdown labels

Closes #24198

@inform880labs
I switched the key from index to the getOptionLabel function in order…
d9cb829 
… for a11y to appropriately read dropdown labels
@inform880 inform880 mentioned this pull request Dec 31, 2020
Closed
2 tasks
@mui-pr-bot
Copy link

mui-pr-bot commented Dec 31, 2020 •
edited

Details of bundle changes

Generated by 🚫 dangerJS against c632040

@mbrookes mbrookes added accessibility a11y bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! labels Dec 31, 2020
@mbrookes mbrookes changed the title Autocomplete dropdown a11y fix [Autocomplete[ Fix dropdown accessibility Dec 31, 2020
@inform880 inform880 changed the title [Autocomplete[ Fix dropdown accessibility [Autocomplete] Fix dropdown accessibility Dec 31, 2020
@inform880
Copy link
Contributor Author

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.

@github-actions github-actions bot mentioned this pull request Dec 31, 2020
Closed
@inform880
Copy link
Contributor Author

Thanks so much for helping @oliviertassinari , you're great!

eps1lon
eps1lon reviewed Jan 1, 2021
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment •
edited

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
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 1, 2021 •
edited

@eps1lon What are you referring to by "re-ordering"? The diff without spaces doesn't seem to show a reordering.

eps1lon
eps1lon requested changes Jan 1, 2021
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a 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.

packages/material-ui/src/useAutocomplete/useAutocomplete.test.js Outdated
describe('useAutocomplete', () => {
const render = createClientRender();

it('should use unique keys for the option', () => {
Copy link
Member

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?

Copy link
Member

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?

Suggested change
it('should use unique keys for the option', () => {
it('should use a unique and deterministic key for each option', () => {

Copy link
Member

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.

Copy link
Member

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.

@eps1lon
Copy link
Member

eps1lon commented Jan 1, 2021

Couple of thoughts how to fix the underlying issue:

  1. re-mount the listbox if the filter changes (e.g. use input.value as the key of the listbox)
  2. Use the index of the unfiltered options
    This would be ideal as far as I can tell if implementable

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 1, 2021 •
edited

People could render custom options without implementing getOptionLabel whose default implementation does not guarantee a string.

@eps1lon Are you sure? getOptionLabel is guaranteed to return a string and defined thanks to:

https://github.com/mui-org/material-ui/blob/2bb1f1ca754bc098f66aba0016b283511ab3dec8/packages/material-ui/src/useAutocomplete/useAutocomplete.d.ts#L14

https://github.com/mui-org/material-ui/blob/2bb1f1ca754bc098f66aba0016b283511ab3dec8/packages/material-ui/src/useAutocomplete/useAutocomplete.js#L108-L119

@eps1lon
Copy link
Member

eps1lon commented Jan 1, 2021

getOptionLabel is guaranteed to return a string and defined thanks to:

We'll see if people notice that.

@eps1lon

This comment has been minimized.

Sign in to view
oliviertassinari
oliviertassinari approved these changes Jan 1, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a 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

@oliviertassinari oliviertassinari changed the title [Autocomplete] Fix dropdown accessibility [Autocomplete] Fix VoiceOver not reading the correct activedescendant Jan 1, 2021
@oliviertassinari oliviertassinari merged commit 8ffde9e into mui:next Jan 1, 2021
@oliviertassinari
Copy link
Member

@inform880 Thanks again for bringing the issue to our attention.

@oliviertassinari oliviertassinari mentioned this pull request Jun 27, 2021
Closed
@oliviertassinari oliviertassinari mentioned this pull request Nov 12, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon approved these changes

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
accessibility a11y bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Autocomplete] for Voiceover on Mac is reading incorrect labels on any browser
6 participants
@inform880 @mui-pr-bot @oliviertassinari @eps1lon @mbrookes @inform880labs