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 listbox opens and closes on click when used with limitTags #42494

Merged
merged 31 commits into from
Sep 20, 2024

Conversation

appleSimple
Copy link
Contributor

@appleSimple appleSimple commented Jun 2, 2024

Fixes #42432

Input element is fall down because of hide tag was shown.
So event target not same in the code below.
I fixed it by add style property pointer-events: none; at autocomplete input. To attach mdn link for you.

// Autocomplete.js

onClick: (event) => {
  if (event.target === event.currentTarget) {
    handleInputMouseDown(event);
  }
},

See discussion below in the pull request.

Before: https://mui.com/material-ui/react-autocomplete/#limit-tags
After: https://deploy-preview-42494--material-ui.netlify.app/material-ui/react-autocomplete/#limit-tags

@mui-bot
Copy link

mui-bot commented Jun 2, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against cfb9bd5

@aarongarciah aarongarciah added package: material-ui Specific to @mui/material component: autocomplete This is the name of the generic UI component, not the React module! labels Jun 3, 2024
@aarongarciah aarongarciah requested a review from mj12albert June 3, 2024 07:24
@ZeeshanTamboli ZeeshanTamboli added the bug 🐛 Something doesn't work label Jun 13, 2024
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.

This works as shown in the documentation:

Before: https://mui.com/material-ui/react-autocomplete/#limit-tags
After: https://deploy-preview-42494--material-ui.netlify.app/material-ui/react-autocomplete/#limit-tags

However, the tests fail as shown in the CI.

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Autocomplete] openOnFocus prop doesn't work when used with limitTags (#42432) [material-ui][Autocomplete] Listbox flashes when used with limitTags Jun 13, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Autocomplete] Listbox flashes when used with limitTags [material-ui][Autocomplete] Listbox opens and closes on click when used with limitTags Jun 13, 2024
@appleSimple
Copy link
Contributor Author

Thank you for read. @ZeeshanTamboli .

I passed CI test done! 🎉 please check this result.

Additionally, I deleted previous fixes and pushed another. Because of cause was wrong I expected.
When I clicked Input, occured events that Input's mouse down and wrapper's click so the list box is open and close because of double events.

Input area

image

Wrapper area

image

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.

This seem to work well. But there is a slight delay in the tag getting rendered after the listbox is opened as shown in the video:

Autocomplete.42494.mp4

Could that be avoided?

@appleSimple
Copy link
Contributor Author

hi, @ZeeshanTamboli

I think the same way about slightly delay movement.
I tried several things and finally i fixed it done. The cause remains the same.

autocomplete-delay.mov

check result please 🙏

@ZeeshanTamboli ZeeshanTamboli added the regression A bug, but worse label Jun 26, 2024
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.

The current solution works but is not ideal. The bug is a regression from #36369. The following fix resolves the issue:

--- a/packages/mui-base/src/useAutocomplete/useAutocomplete.js
+++ b/packages/mui-base/src/useAutocomplete/useAutocomplete.js
@@ -1042,6 +1042,7 @@ export function useAutocomplete(props) {
   const handleInputMouseDown = (event) => {
     if (!disabledProp && (inputValue === '' || !open)) {
       handlePopupIndicator(event);
+      event.stopPropagation();
     }
   };

diff --git a/packages/mui-material/src/Autocomplete/Autocomplete.js b/packages/mui-material/src/Autocomplete/Autocomplete.js
index c8ba035f39..8dc63ca14e 100644
--- a/packages/mui-material/src/Autocomplete/Autocomplete.js
+++ b/packages/mui-material/src/Autocomplete/Autocomplete.js
@@ -724,11 +724,7 @@ const Autocomplete = React.forwardRef(function Autocomplete(inProps, ref) {
             ref: setAnchorEl,
             className: classes.inputRoot,
             startAdornment,
-            onClick: (event) => {
-              if (event.target === event.currentTarget) {
-                handleInputMouseDown(event);
-              }
-            },
+            onMouseDown: (event) => handleInputMouseDown(event),
             ...((hasClearIcon || hasPopupIcon) && {
               endAdornment: (
                 <AutocompleteEndAdornment className={classes.endAdornment} ownerState={ownerState}>
diff --git a/packages/mui-material/src/Autocomplete/Autocomplete.test.js b/packages/mui-material/src/Autocomplete/Autocomplete.test.js
index cc2550cb23..3a3e831e9a 100644
--- a/packages/mui-material/src/Autocomplete/Autocomplete.test.js
+++ b/packages/mui-material/src/Autocomplete/Autocomplete.test.js
@@ -1109,7 +1109,7 @@ describe('<Autocomplete />', () => {
         />,
       );

-      fireEvent.click(ref.current);
+      fireEvent.mouseDown(ref.current);
       expect(handleOpen.callCount).to.equal(1);
     });

Explanation:

The issue was that the mousedown event on the input (handleInputMouseDown handler) was being called twice, leading to the initial opening and then immediate closing of the listbox. The open state was toggled in handleInputMouseDown because it was executed twice. This happened because the mousedown event from the input's onMouseDown bubbled up to the root's onClick, causing handleInputMouseDown here to be executed again.

By the time the click event is fired, the event has already propagated through the mousedown and mouseup phases, potentially causing the target and currentTarget to be the same if the event bubbled up to the parent div.

The solution above stops the event from bubbling from the input's mousedown event and changes the root div's onClick to onMouseDown because we are handling the input's mousedown event. This approach also retains the fix from #36369.

Additionally, we need to add a test case for this with the limitTags prop to prevent future regressions.

@appleSimple
Copy link
Contributor Author

I didn't know if it was okay to modify test code.
Thanks for the explanation 😊

@ZeeshanTamboli
Copy link
Member

@appleSimple Could you add a test case for this using the limitTags prop? The listbox should open but not close immediately on click. The test should pass here but fail in the next branch, ensuring it tests the correct behavior.

@ZeeshanTamboli ZeeshanTamboli added the PR: needs test The pull request can't be merged label Jun 30, 2024
@appleSimple
Copy link
Contributor Author

@ZeeshanTamboli
My answer was too late, and I will try to make test case you said.
I guessed cause of fail in the next branch is "when clicked the arrow". (like below video)
This changed was occured same issue.

I will fix it, too.

open_close_when_click_arrow.mov

@appleSimple appleSimple reopened this Jul 28, 2024
@ZeeshanTamboli ZeeshanTamboli removed the PR: needs test The pull request can't be merged label Sep 20, 2024
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.

@appleSimple Thanks for the pull request.

@ZeeshanTamboli ZeeshanTamboli merged commit 1289e89 into mui:master Sep 20, 2024
20 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: material-ui Specific to @mui/material regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][Autocomplete] Listbox opens and closes on click when used with limitTags
4 participants