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

[material-ui][Autocomplete] Fix bug with child chip button events propagating to parent #43982

Merged
merged 8 commits into from
Oct 16, 2024

Conversation

snapwich
Copy link
Contributor

@snapwich snapwich commented Oct 3, 2024

#42494 introduced a bug where the mouse down events in chips (such as clicking the delete button) was being handled in the autocomplete causing it to open/close as you deleted chips. i put the check back for if (event.target === event.currentTarget) { and added a test and it seems to fix the issue.

@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Oct 4, 2024
@snapwich
Copy link
Contributor Author

snapwich commented Oct 5, 2024

i just added one more update. i found another bug introduced by #42494 where if you clicked the autocomplete root component (and not the input) it wouldn't open and close the menu which was a regression as it used to do that. i added another fix to add that behavior back. i'm assuming #42494's use case is still good as well as after applying this new fix (which undid some of their changes) their test they added still passes.

it all looks good to me now, but tbh i didn't totally understand what bug they were fixing.

@DiegoAndai DiegoAndai requested review from DiegoAndai and removed request for mj12albert October 11, 2024 12:27
@mui-bot
Copy link

mui-bot commented Oct 11, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against dc6fdf8

@DiegoAndai
Copy link
Member

Thanks for working on this @snapwich, I'm picking this one up, see #43976 (comment).

@DiegoAndai
Copy link
Member

Tests added for:

Waiting for #43976 (comment) to see if we can come up with a way to test #43976.

@DiegoAndai DiegoAndai self-assigned this Oct 15, 2024
@DiegoAndai DiegoAndai requested review from aarongarciah and removed request for DiegoAndai October 15, 2024 19:36
@DiegoAndai
Copy link
Member

@aarongarciah may I ask you to review this? (I don't review it myself as I contributed heavily to it)

This PR reverts #42494, but keeps the onClick -> onMouseDown change so #42432 is not reintroduced.

I added tests for the issue reported in this PR, #44053, and #42432.

@cherniavskii if you consider it necessary, we could add a regression test for #43976 that simulates that issue's actions in the mui-x repo. I haven't opened a PR because the regression test setup in that repo is a bit different than Material UI's, and I'm not very familiar with it.

Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Nice! Tested manually and all of the fixes seem to work as expected. The tests also look better now.

@DiegoAndai DiegoAndai merged commit 064fa67 into mui:master Oct 16, 2024
19 checks passed
@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work package: material-ui Specific to @mui/material regression A bug, but worse labels Oct 24, 2024
@ZeeshanTamboli
Copy link
Member

@DiegoAndai @snapwich Thanks for fixing this and other related issues.

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: material-ui Specific to @mui/material regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants