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

Bug: Virtualized Autocomplete scrolling issue on Internet Explorer 11 (IE) #22705

Closed
2 tasks done
jakub-astrahit opened this issue Sep 23, 2020 · 11 comments · Fixed by #22940
Closed
2 tasks done

Bug: Virtualized Autocomplete scrolling issue on Internet Explorer 11 (IE) #22705

jakub-astrahit opened this issue Sep 23, 2020 · 11 comments · Fixed by #22940
Assignees
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

@jakub-astrahit
Copy link

I am unable to click the scrollbar on the virtualized autocomplete.

  • 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 😯

Open https://material-ui.com/components/autocomplete/#virtualization in Internet Explorer 11 and open the dropdown. Then click the scrollbar and the dropdown will disappear.

Expected Behavior 🤔

The scrollbar should NOT disappear. I should be able to click and hold the scroll through the options.

Steps to Reproduce 🕹

Steps:

  1. Start Internet Explorer 11.
  2. Open https://material-ui.com/components/autocomplete/#virtualization
  3. Click the autocomplete to open the dropdown.
  4. Click on the scrollbar.
  5. The dropdown disappears.

Context 🔦

It's broken in IE 11 only. It works fine in Chrome, for example.

Your Environment 🌎

Internet Explorer 11

Tech Version
Material-UI Core 4.11.0
Material-UI Lab 4.0.0-alpha.56
React 16.13.1
Browser Internet Explorer 11
TypeScript 3.7.5
@jakub-astrahit jakub-astrahit added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 23, 2020
@jakub-astrahit jakub-astrahit changed the title Bug: Virtualized Autocomplete scrolling issue on Internet Explorer (IE) Bug: Virtualized Autocomplete scrolling issue on Internet Explorer 11 (IE) Sep 23, 2020
@mnajdova
Copy link
Member

mnajdova commented Sep 23, 2020

I've spend some time on this, but don't have a final fix to be honest. I believe the issue is that, clicking on the scrollbar acts like click away, and that is the reason why it is closed. But why does it happens only in IE11? 😕

When inspecting the elements I noticed that ul element actually ends right before the scrollbar

image

so I applied this change

diff --git a/docs/src/pages/components/autocomplete/Virtualize.js b/docs/src/pages/components/autocomplete/Virtualize.jsindex 4eaa53df7f..42c2ad7e0e 100644
--- a/docs/src/pages/components/autocomplete/Virtualize.js
+++ b/docs/src/pages/components/autocomplete/Virtualize.js
@@ -112,6 +112,7 @@ const useStyles = makeStyles({
     '& ul': {
       padding: 0,
       margin: 0,
+      width: 'calc(100% + 20px) !important',
     },
   },
 });

Which fixed the width

image

But the issue is still there. All other examples work as expected in IE 11 (scrolling is not closing the listbox)

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 23, 2020

@mnajdova I suspect that the logic of #19969 isn't resilient to the different DOM structure that react-window impose.

@jakub-astrahit
Copy link
Author

As a workaround, I added this style to hide the scrollbar on IE 11 but still be able to scroll:

listbox: {
    '-ms-overflow-style': 'none'
}

It doesn't fix the issue though.

@mnajdova
Copy link
Member

@oliviertassinari thanks for pointing that out, wasn't aware of it. I'll spend some time on it and will provide my findings tomorrow morning.

@mnajdova
Copy link
Member

So Olivier was right. Basically the difference between the virtualized and the other examples is that in the virtualized we need to check if the document.activeElement is the firstChild of the listbox, not the parentElement. With this I see two options, first one, we could add a prop for this specific scenario (not sure about the best name for it to be honest).. It would look something like this:

diff --git a/docs/src/pages/components/autocomplete/Virtualize.js b/docs/src/pages/components/autocomplete/Virtualize.jsindex 4eaa53df7f..d615712509 100644
--- a/docs/src/pages/components/autocomplete/Virtualize.js
+++ b/docs/src/pages/components/autocomplete/Virtualize.js
@@ -137,6 +137,7 @@ export default function Virtualize() {
       disableListWrap
       classes={classes}
       ListboxComponent={ListboxComponent}
+      ie11CheckChild
       renderGroup={renderGroup}
       options={OPTIONS}
       groupBy={(option) => option[0].toUpperCase()}
diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index b0edc3a225..6c5a418f2b 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -303,6 +303,7 @@ const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {
     selectOnFocus = !props.freeSolo,
     size = 'medium',
     value: valueProp,
+    ie11CheckChild,
     ...other
   } = props;
   /* eslint-enable @typescript-eslint/no-unused-vars */
diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 75b3a2093a..ca46cb9b71 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -93,6 +93,7 @@ export default function useAutocomplete(props) {
     includeInputInList = false,
     inputValue: inputValueProp,
     multiple = false,
+    ie11CheckChild,
     onChange,
     onClose,
     onHighlightChange,
@@ -783,13 +784,23 @@ export default function useAutocomplete(props) {
   const handleBlur = (event) => {
     // Ignore the event when using the scrollbar with IE 11
     if (
-      listboxRef.current !== null &&
-      document.activeElement === listboxRef.current.parentElement
+      (!ie11CheckChild &&
+        listboxRef.current !== null &&
+        document.activeElement === listboxRef.current.parentElement) ||
+      (listboxRef.current !== null && document.activeElement === listboxRef.current.firstChild)
     ) {
       inputRef.current.focus();
       return;
     }
(END)                                                                                                                 

Or if we have custom ListboxComponent we could suppose it needs to handle a ref that we send, that will be set on the correct DOM element, something like ie11ScrollElementRef

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 25, 2020

@mnajdova We have an alternative. We can lessen the check. I don't think we need to check that the document.activeElement is exactly a specific node as long as it's a descendant of the listbox. This would solve the issue:

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 2544c436ad..5514200566 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
     // Ignore the event when using the scrollbar with IE 11
     if (
       listboxRef.current !== null &&
-      document.activeElement === listboxRef.current.parentElement
+      listboxRef.current.parentElement.contains(document.activeElement)
     ) {
       inputRef.current.focus();
       return;

While we are at it, I have noticed that we could edge for: https://material-ui.com/components/menus/#limitations

diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index b0edc3a225..0b16375ae8 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -168,7 +168,7 @@ export const styles = (theme) => ({
   /* Styles applied to the `Paper` component. */
   paper: {
     ...theme.typography.body1,
-    overflow: 'hidden',
+    overflow: 'auto',
     margin: '4px 0',
   },
   /* Styles applied to the `listbox` component. */
@@ -193,6 +193,7 @@ export const styles = (theme) => ({
   option: {
     minHeight: 48,
     display: 'flex',
+    overflow: 'hidden',
     justifyContent: 'flex-start',
     alignItems: 'center',
     cursor: 'pointer',

@oliviertassinari oliviertassinari added 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. bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 25, 2020
@jakub-astrahit
Copy link
Author

Is there any way I can help? @oliviertassinari @mnajdova
Never contributed to a repo on github so maybe this could be my first time?

@mnajdova
Copy link
Member

@jakub-astrahit sure feel free to :) Make sure to follow https://github.com/mui-org/material-ui/blob/next/CONTRIBUTING.md I would say changes proposed from @oliviertassinari should be the starting point :)

@mbruhler
Copy link
Contributor

mbruhler commented Oct 7, 2020

is it still available @oliviertassinari @mnajdova ? If yes, please assign it to me :)

I'm new to the codebase, giving a try to open-source projects

@oliviertassinari
Copy link
Member

@bearfromtheabyss Feel free to work on it :)

@mbruhler
Copy link
Contributor

mbruhler commented Oct 8, 2020

I've implemented the solution from @oliviertassinari

Created a PR here

I checked it and works fine now in Chrome and IE11

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