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

[Autocomplete] Group labels hide the selection when navigating with keyboard #19217

Closed
2 tasks done
aisamu opened this issue Jan 13, 2020 · 5 comments · Fixed by #19305
Closed
2 tasks done

[Autocomplete] Group labels hide the selection when navigating with keyboard #19217

aisamu opened this issue Jan 13, 2020 · 5 comments · Fixed by #19305
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

@aisamu
Copy link
Contributor

aisamu commented Jan 13, 2020

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

Once we reach the top while scrolling up the selection's no longer visible (and appears to be behind the group label):

Expected Behavior 🤔

Selected/hovered item should be visible

Steps to Reproduce 🕹

The issue is present on the official AutoComplete's demo page

Steps:

  1. Visit https://material-ui.com/components/autocomplete/#grouped
  2. Click on the component to open the drop-down
  3. Scroll down using the keyboard (⬇️), until the group category changes from 0-9 to A
  4. Scroll up using the keyboard (⬆️) and try to select the first item from the whole list ("12 Angry Men")

Your Environment 🌎

Whatever's being currently used for the demo page!
We've double checked that it also happens on our environment:

Tech Version
@material-ui/core 4.7.2
@material-ui/lab 4.0.0-alpha.38
React 16.12.0
Browser Safari 13 (❌), Chrome 79 (❌) , Firefox 72(❌)
@oliviertassinari
Copy link
Member

It sounds like the same root issue as #19206

@oliviertassinari oliviertassinari added the component: autocomplete This is the name of the generic UI component, not the React module! label Jan 13, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 17, 2020

They seem to have found a viable solution, but I'm not sure exactly how they do it. It seems to be:

let count = this.getIndexByValue(activeElement.getAttribute('data-value')) - 1;
let height = parseInt(getComputedStyle(this.liCollections[0], null).getPropertyValue('height'), 10);
if (!isNaN(height) && this.getModuleName() !== 'autocomplete') {
    this.removeFocus();
    let fixedHead = this.fields.groupBy ? this.liCollections[0].offsetHeight : 0;
    this.list.scrollTop = count * height + fixedHead;
    addClass([activeElement], dropDownListClasses.focus);
}

source

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Jan 17, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 17, 2020

This is probably not the best approach, but it does relatively well the job (quick and dirty)

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index a0dcae57a..c54d010ce 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -178,11 +178,14 @@ export default function useAutocomplete(props) {
       const element = option;

       const scrollBottom = listboxNode.clientHeight + listboxNode.scrollTop;
-      const elementBottom = element.offsetTop + element.offsetHeight;
+      const elementBottom = element.offsetTop + element.offsetHeight * 2;
       if (elementBottom > scrollBottom) {
         listboxNode.scrollTop = elementBottom - listboxNode.clientHeight;
-      } else if (element.offsetTop < listboxNode.scrollTop) {
-        listboxNode.scrollTop = element.offsetTop;
+      } else if (
+        element.offsetTop - element.offsetHeight * (groupBy ? 2 : 1) <
+        listboxNode.scrollTop
+      ) {
+        listboxNode.scrollTop = element.offsetTop - element.offsetHeight * (groupBy ? 2 : 1);
       }
     }
   }

@aisamu Unless somebody can come up with a better approach, I think that we can move forward with it. What do you think? Do you want to take on this task? :)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Jan 17, 2020
@netochaves
Copy link
Contributor

Can I take this?

@aisamu
Copy link
Contributor Author

aisamu commented Jan 19, 2020

@netochaves I'm currently tackling this one!
Would you mind giving a hand on #19251 or confirming that the proposed fix for #19213 works?

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.

3 participants