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

[Autocomplete] Cannot delete multiple Chips with Escape Key #19709

Closed
2 tasks done
jcafiero opened this issue Feb 14, 2020 · 8 comments · Fixed by #20065
Closed
2 tasks done

[Autocomplete] Cannot delete multiple Chips with Escape Key #19709

jcafiero opened this issue Feb 14, 2020 · 8 comments · Fixed by #20065
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@jcafiero
Copy link
Contributor

jcafiero commented Feb 14, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Currently, if you hit the Escape key while focused in a multi-select Autocomplete with the clearOnEscape prop it will not clear the Chips from the window.

One behavior I did notice was that if there is still active text in the field, hitting the Escape Key twice would clear everything from the Autocomplete.

Expected Behavior 🤔

If you hit the Escape key while focused in a multi-select Autocomplete with the clearOnEscape prop, it should clear the Chips, similar to how clicking on the 'X' icon will clear all of the Chips from the field. This behavior should be consistent whether or not there is partially filled text or not in the Autocomplete.

Steps to Reproduce 🕹

Here is a CodeSandbox: https://codesandbox.io/s/cranky-dhawan-2smfv?fontsize=14&hidenavigation=1&theme=dark

Steps:

  1. Tab in to the Autocomplete in the example.
  2. Hit the Escape key.
  3. Note the Chips are still there.
  4. Click on the clear icon in the Autocomplete component.
  5. Note that the Chips are deleted.

Context 🔦

This is an accessibility bug that should be resolved to give keyboard users the same experience.

Your Environment 🌎

Tech Version
Material-UI v4.9.2
React latest
Browser Chrome
@oliviertassinari oliviertassinari added ready to take Help wanted. Guidance available. There is a high chance the change will be accepted component: autocomplete This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Feb 14, 2020
@oliviertassinari
Copy link
Member

@jcafiero Brilliant, thanks for reporting this bug. It should be an easy one to fix and write tests for. Feel free to go ahead :).

@chaudharykiran
Copy link
Contributor

chaudharykiran commented Feb 16, 2020

As my understanding, esc should delete all the selected chips same as x button click. If that is the case, I like to work on this issue. cc. @jcafiero @oliviertassinari

@jcafiero
Copy link
Contributor Author

@chaudharykiran that's fine with me. Yes, that is the desired behavior.

@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. and removed ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Feb 21, 2020
@oliviertassinari
Copy link
Member

For anyone looking at the problem, the fix should be about:

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 9957ed76d..2f61a763e 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -628,7 +628,7 @@ export default function useAutocomplete(props) {
           // Avoid the Modal to handle the event.
           event.stopPropagation();
           handleClose(event);
-        } else if (clearOnEscape && inputValue !== '') {
+        } else if (clearOnEscape && (inputValue !== '' || (multiple && value.length > 0))) {
           // Avoid Opera to exit fullscreen mode.
           event.preventDefault();
           // Avoid the Modal to handle the event.

Then, it's about adding a test case to prevent regressions in the future.

@yann120
Copy link
Contributor

yann120 commented Feb 25, 2020

Hi, can we do a PR to fix this tonight ?
I'm going to the meetup "Contribute by Doctolib" and I'm looking for 'good first issues' :)
Thanks

@oliviertassinari
Copy link
Member

@yann120 I was working at Doctolib, sounds great! 😆

@chaudharykiran
Copy link
Contributor

I am afraid, I may not be able to send PR for this tonight. cc. @oliviertassinari @yann120

@yann120
Copy link
Contributor

yann120 commented Feb 26, 2020

Hey @oliviertassinari , we fix it yesterday at Doctolib event with your advices. But we write a new test and we can't make it pass... But we are working on it and try to understand why 👍

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! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants