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

Open
wants to merge 25 commits into
base: next
Choose a base branch
from

Conversation

appleSimple
Copy link
Contributor

@appleSimple appleSimple commented Jun 2, 2024

Preview: https://deploy-preview-42494--material-ui.netlify.app/material-ui/react-autocomplete/#limit-tags

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);
  }
},

@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 73ae5d9

@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
@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
Copy link
Contributor Author

appleSimple commented Jul 14, 2024

I have added the test code. Could you please check if it was added as you intended?

Due to a recent PR that removed dependencies, the files that need to be updated have changed. Therefore, I have added the necessary code to those files. (#42907)

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Autocomplete] Listbox opens and closes on click when used with limitTags [material-ui][Autocomplete] Fix listbox opens and closes on click when used with limitTags Jul 15, 2024
@@ -413,6 +413,29 @@ describe('<Autocomplete />', () => {
expect(getAllByRole('button', { hidden: false })).to.have.lengthOf(5);
}
});

it('listbox stays open after opening', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test passes in the next HEAD branch, so it isn't testing correctly what's needed. It should pass in this branch but fail in the next branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it, I'll modify it, soon!!

appleSimple and others added 2 commits July 16, 2024 22:08
Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Signed-off-by: JiYeon <73057489+appleSimple@users.noreply.github.com>
@appleSimple
Copy link
Contributor Author

@ZeeshanTamboli
I clearly included the cause of the bug in the test title. I wrote the test code forgetting this. Sorry.
Please check this change.

@@ -413,6 +413,40 @@ describe('<Autocomplete />', () => {
expect(getAllByRole('button', { hidden: false })).to.have.lengthOf(5);
}
});

it('When the input box needs to expand downward, the listbox should remain open.', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This is also passing on the next branch. It should fail there. handleClose should be called once there.

@ZeeshanTamboli ZeeshanTamboli added PR: needs test The pull request can't be merged and removed PR: needs test The pull request can't be merged labels Jul 17, 2024
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 PR: needs test The pull request can't be merged 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