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] Input cleared unexpectedly with freeSolo, autoHighlight and autoSelect #18646

Closed
2 tasks done
reiv opened this issue Dec 1, 2019 · 11 comments · Fixed by #19511 or #23025
Closed
2 tasks done

[Autocomplete] Input cleared unexpectedly with freeSolo, autoHighlight and autoSelect #18646

reiv opened this issue Dec 1, 2019 · 11 comments · Fixed by #19511 or #23025
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. hacktoberfest https://hacktoberfest.digitalocean.com/

Comments

@reiv
Copy link

reiv commented Dec 1, 2019

An Autocomplete with freeSolo, autoHighlight and autoSelect props set to true clears its input when it loses focus and the input value is not an exact or partial match for any of the provided options.

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

See above. This only occurs once; going back and entering the same free string causes it to persist until the input is cleared again (either manually or by clicking the clear button), which seems to put it back into the problematic state. Note that the string must contain letters that appear in any of the option labels for this to occur.

Expected Behavior 🤔

The input value should not be cleared on blur as long as freeSolo is true.

Steps to Reproduce 🕹

Codesandbox: https://codesandbox.io/s/create-react-app-76xd5

Steps:

  1. Enter a string into the Autocomplete input which is not a substring of any of the option labels, but contains letters that appear in at least one of them (e.g. "aaa").
  2. Tab to the next field.
  3. Tab back to the Autocomplete and enter the same string.
  4. Tab to the next field.

To reset, click the clear button or backspace over the input field.

Context 🔦

I am trying to build a simple autocomplete field which permits free values but optimizes entry of provided options first (hence this combination of props).

Your Environment 🌎

Tech Version
Material-UI latest
Material-UI Lab 4.0.0-alpha.34
React latest
Browser Firefox 70.0.1, Chrome 78.0.3904.108
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! labels Dec 1, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 1, 2019

@reiv Thanks for the bug report, I can confirm two issues:

  1. type a x2, type Backspace => observe that the first option is not highlighted.
  2. type a x2, blur => observe that a undefined value is selected.

What do you think of the following diff?

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index a8fd29e1a..df72a0980 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -114,7 +114,6 @@ export default function useAutocomplete(props) {
   const [focusedTag, setFocusedTag] = React.useState(-1);
   const defaultHighlighted = autoHighlight ? 0 : -1;
   const highlightedIndexRef = React.useRef(defaultHighlighted);
-  const selectedIndexRef = React.useRef(-1);

   function setHighlightedIndex(index, mouse = false) {
     highlightedIndexRef.current = index;
@@ -350,7 +349,6 @@ export default function useAutocomplete(props) {

     const nextIndex = validOptionIndex(getNextIndex(), direction);
     setHighlightedIndex(nextIndex);
-    selectedIndexRef.current = nextIndex;

     if (autoComplete && diff !== 'reset') {
       if (nextIndex === -1) {
@@ -429,8 +427,6 @@ export default function useAutocomplete(props) {
     }

     resetInputValue(event, newValue);
-
-    selectedIndexRef.current = -1;
   };

   function validTagIndex(index, direction) {
@@ -615,8 +611,8 @@ export default function useAutocomplete(props) {
       return;
     }

-    if (autoSelect && selectedIndexRef.current !== -1) {
-      handleValue(event, filteredOptions[selectedIndexRef.current]);
+    if (autoSelect && highlightedIndexRef.current !== -1 && popupOpen) {
+      handleValue(event, filteredOptions[highlightedIndexRef.current]);
     } else if (!freeSolo) {
       resetInputValue(event, value);
     }
@@ -673,8 +669,13 @@ export default function useAutocomplete(props) {
       return;
     }

-    // Restore the focus to the correct option.
-    setHighlightedIndex(highlightedIndexRef.current);
+    // Automatically select the first option as the listbox become visible.
+    if (highlightedIndexRef.current === -1 && autoHighlight) {
+      changeHighlightedIndex('reset', 'next');
+    } else {
+      // Restore the focus to the correct option.
+      setHighlightedIndex(highlightedIndexRef.current);
+    }
   });

   const handlePopupIndicator = event => {

Basically:

  • use the same variable for highlighting and auto selection
  • only auto select if a highlighted option is visible
  • if a listbox become visible, give it an option to auto highlight

Do you want to submit a pull request? :)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 1, 2019
@reiv
Copy link
Author

reiv commented Dec 2, 2019

@oliviertassinari I tried out your suggested changes and while it does fix the original issue I'm not sure if treating "highlighted" and "selected" option as the same is the way to go. For instance, an option could be inadvertently selected by simply mousing over it, which feels weird.

Also, when clearing the input, the first available option is autoselected on blur, unless the listbox is dismissed first. autoSelect should not affect the selection when no input value is given, should it? So:

// Automatically select the first option as the listbox becomes visible.
- if (highlightedIndexRef.current === -1 && autoHighlight) {
+ if (inputValue && highlightedIndexRef.current === -1 && autoHighlight) {
    changeHighlightedIndex('reset', 'next');

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 2, 2019

@reiv Thanks for the feedback:

  • I think that it's important to keep the selected and highlighted state in sync. If we don't, we would open the door to unpredictable option selection (with no visual preliminary feedback).
  • I think that the highlighted option should be selected, even if the field is empty on blur. If you don't want to behavior, remove autoHighlight or autoSelect.

@captain-yossarian
Copy link
Contributor

@reiv @oliviertassinari Can I pick it up?

@alexpermiakov
Copy link

Are you still working on it @SerhiiBilyk?

@abbasyadollahi
Copy link

@oliviertassinari @captain-yossarian
This issue still pertains to Autocomplete when the multiple prop is set to true, alongside freeSolo, autoHighlight.

I modified the original sandbox to demonstrate this: https://codesandbox.io/s/create-react-app-forked-ff1h7?file=/src/App.js

Basically it's the same bug mentioned before:

  • type a x2, type Backspace => observe that the first option is not highlighted.

Considering v5 is currently being worked on, I'm assuming a fix for this would have to wait for the next release instead of being merged into v4?

@captain-yossarian
Copy link
Contributor

@alexpermiakov You can pick it up
@theGirrafish @oliviertassinari if @alexpermiakov will not pick it up, I'll start work on it in 2 days

@oliviertassinari
Copy link
Member

@theGirrafish Do you want to open a new issue for it? Regarding a fix, I believe we could do:

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index a9d8d48de3..5e47e637ca 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -430,7 +430,7 @@ export default function useAutocomplete(props) {
     }

     // Synchronize the value with the highlighted index
-    if (!filterSelectedOptions && valueItem != null) {
+    if (valueItem != null) {
       const currentOption = filteredOptions[highlightedIndexRef.current];

       // Keep the current highlighted index if possible

It would fail a test case @captain-yossarian you added but I believe the asserted behavior from #19923 was only one of the two options mentioned in #19637. I think that it's fair to reset the index when filterSelectedOptions, it's disruptive, in the first place.

@abbasyadollahi
Copy link

@oliviertassinari Sure, I can create another ticket for this.
I can start looking into fixing it in a few days if @captain-yossarian hasn't already picked this up.

@captain-yossarian
Copy link
Contributor

@theGirrafish please take it, I have no time (

@enoh-barbu
Copy link

what's the status on this one?

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. hacktoberfest https://hacktoberfest.digitalocean.com/
Projects
None yet
7 participants