[material-ui][Chip] Fix focus issue related with the Escape event

[shrilsharma]

• I have followed (at least) the PR section of the contributing guide.

fixes #40510

[mui-bot]

Netlify deploy preview

• docs/data/material/migration/migration-v5/migration-v5.md

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 04e9e48

[DiegoAndai]

Thanks for working on this @shrilsharma.

This change does make sense to me as it gets the clickable Chip closer to a button:

• Focus is not lost when the Escape key is pressed
• Focus goes back accordingly when closing a Modal, Dialog, Drawer

But I'm hesitant as the functionality has been there forever. This is the farthest back I could track the blur() call: chaudharykiran@943f95f#diff-d1439e644fd94d980426ac362fcc36489ba82e228bab753e9a37dd96c61fded4R117.

What do you think @zanivan @mnajdova?

[shrilsharma]

Hi @DiegoAndai

What I understand from the code is that it was intentionally made to lose focus on the escape key down.

[DiegoAndai]

What I understand from the code is that it was intentionally made to lose focus on the escape key down.

Yes, you're correct. I'm wondering (and asking) if that's a behavior we wish to modify. As I mentioned, it makes sense to me to change it and make it closer to a button, but I want to discuss it with @zanivan and @mnajdova as the functionality is a longstanding one.

[zanivan]

What do you think @zanivan @mnajdova?

Unless there's a specific reason I'm unaware of, the chip's behavior should be similar to the button's in this case.

[DiegoAndai]

@mnajdova do you see any reason not to go forward with this change? Is there any historical context of why the Chip losing focus with the escape key behavior was implemented?

[mnajdova]

@mnajdova do you see any reason not to go forward with this change? Is there any historical context of why the Chip losing focus with the escape key behavior was implemented?

I am not aware of it, the logic seems to be here from the creation of the component. I would recommend checking if there are some regressions with the Autocomplete, as the component is used when the multiple prop is set.

[DiegoAndai]

I checked the multiple Autocomplete use case, and it's working as expected. The blur of the chip inside it when "Escape" is pressed (and actually, when any key except "ArrowLeft" and "ArrowRight" is pressed) is handled here: https://github.com/mui/material-ui/blob/next/packages/mui-base/src/useAutocomplete/useAutocomplete.js#L799-L802

I think we can move forward with this change. May I ask you to remove the if clause entirely, @shrilsharma, instead of commenting on the blur call? Thanks!

[shrilsharma]

@DiegoAndai, I have removed the if block as you suggested.

[DiegoAndai]

👏🏼

[DiegoAndai]

Hey @shrilsharma, thanks for updating the PR. May I ask you to update the failing tests as well? Thanks!

[DiegoAndai]

Thanks for updating the test @shrilsharma!

I think the last thing left to do is to add this to the v6 migration guide: https://github.com/mui/material-ui/blob/next/docs/data/material/migration/migration-v5/migration-v5.md

May I ask you to add a short explanation about the change, why it was needed (to make the chip consistent with buttons), and a short example on how to implement the previous behaviour (probably providing a custom onKeyUp handler)

We should also add a codemod, but that might take a little more time. Let me know if you're up to implementing the codemod, otherwise I can take it separately.

[shrilsharma]

Hi @DiegoAndai, I have updated the migration doc and I think we can add the codemod separately.

[DiegoAndai]

Thanks for updating the PR @shrilsharma

I've reconsidered and think we shouldn't add a codemod, as it would be an edge case and anticipating the user's component architecture would be very complicated.

I've updated the copy a bit, and asked @samuelsycamore for a review on the guide specifically.