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

[base-ui][material-ui][joy-ui][useAutocomplete] Correct keyboard navigation with multiple disabled options #38788

Conversation

VadimZvf
Copy link
Contributor

@VadimZvf VadimZvf commented Sep 3, 2023

Fixes #39281

The current implementation of the useAutocomplete hook has a little bug.
The looped navigation in the option list doesn't work properly.
When user navigates via keyboard in the select component with disabled options, he needs to press the down arrow twice if disabled option is in the end of the list.

Scenario:

  1. The user focused on the last not disabled option.
  2. User press on the down arrow button - the focus disappears (bug)
  3. After second press the focus appears on the first list option

Example before fix:

Screen.Recording.2023-09-03.at.14.26.02.mov

After fix:

Screen.Recording.2023-09-03.at.14.48.08.mov

Issue codesandbox: https://codesandbox.io/s/agitated-frost-qcdgxt?file=/Demo.tsx

Fix codesandbox: https://codesandbox.io/s/autumn-wood-tcc2jl?file=/package.json
Another fix of #39281: https://codesandbox.io/s/https-github-com-mui-material-ui-issues-39281-5g5qsf?file=/Demo.tsx

@mui-bot
Copy link

mui-bot commented Sep 3, 2023

Netlify deploy preview

https://deploy-preview-38788--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 577d849

@VadimZvf VadimZvf changed the title Correct keyboard navigation with multiple disabled options [Select][base-ui] Correct keyboard navigation with multiple disabled options Sep 3, 2023
@zannager zannager added component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Sep 4, 2023
@zannager zannager requested a review from mnajdova September 4, 2023 10:42
@mnajdova mnajdova changed the title [Select][base-ui] Correct keyboard navigation with multiple disabled options [useAutocomplete][base-ui] Correct keyboard navigation with multiple disabled options Sep 8, 2023
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The logic looks good to me. Regarding the tests, so far most of the tests were added in Material UI's Autocomplete component, as a way to test more the end to end behaviour, however this will likely change long term considering all components will use the hook, it would mean that we need to replicate the tests in all components. cc @michaldudak for thoughts on this.

@michaldudak
Copy link
Member

The goal is to test the Base UI unstyled component thoroughly, so we have coverage of both component and a hook. Material UI and Joy UI could have just their extensions tested.
Now, with Autocomplete, we're not there yet because we don't have the unstyled component yet, and we want to create another version of the hook. For now, I'd stick to testing Material UI's Autocomplete.

@VadimZvf VadimZvf requested a review from mnajdova October 14, 2023 18:00
@VadimZvf
Copy link
Contributor Author

Hi @mnajdova !
Can you have a look at my PR one more time, please?
I've moved tests to Autocomplete components tests.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@VadimZvf Looks good! Also nice tests 👍 I've left some minor comments.

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material package: joy-ui Specific to @mui/joy and removed component: select This is the name of the generic UI component, not the React module! labels Oct 17, 2023
@ZeeshanTamboli
Copy link
Member

Your PR fixes #39281. I linked it in the description.

@VadimZvf
Copy link
Contributor Author

Thank you for reviewing 🙏

CI task has failed, seems to me like something with the CI tool

@ZeeshanTamboli ZeeshanTamboli changed the title [useAutocomplete][base-ui] Correct keyboard navigation with multiple disabled options [base-ui][material-ui][joy-ui][useAutocomplete] Correct keyboard navigation with multiple disabled options Oct 19, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@VadimZvf Looks good! Thank you for your contribution!

@ZeeshanTamboli ZeeshanTamboli merged commit 52eaa03 into mui:master Oct 19, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base package: joy-ui Specific to @mui/joy package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] Pressing ArrowDown key behaves incorrectly when remaining options are disabled
7 participants