mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.74k stars 32.24k forks source link

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

Closed aisamu closed 4 years ago

aisamu commented 4 years ago

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 commented 4 years ago

It sounds like the same root issue as #19206

oliviertassinari commented 4 years ago

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 commented 4 years ago

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? :)

netochaves commented 4 years ago

Can I take this?

aisamu commented 4 years ago

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