material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Autocomplete] Not working properly with repeated options values

Open bcarusodev opened this issue 4 years ago • 33 comments

Current Behavior 😯

Autocomplete when receiving repeated options does not filter and does not work correctly

Expected Behavior 🤔

This only happens in version 5, in version 4 this works correctly.

Steps to Reproduce 🕹

Enter this CS https://codesandbox.io/embed/combobox-material-demo-forked-hyslp?fontsize=14&hidenavigation=1&theme=dark

Steps:

  1. Find a unique value, the repeated value will be shown and will add more values when you delete letters

bcarusodev avatar May 28 '21 14:05 bcarusodev

Thanks for the report.

Autocomplete when receiving repeated options does not filter and does not work correctly

Could you be a bit more specific what you mean with "correctly"? What are the concrete differences between v4 and v5? A screen recording might help here.

eps1lon avatar May 31 '21 08:05 eps1lon

Thanks for the report.

Autocomplete when receiving repeated options does not filter and does not work correctly

Could you be a bit more specific about what you mean by "correctly"? What are the concrete differences between v4 and v5? A screen recording might help here.

Sure!

V5 In the next video, I show V5 version on Autocomplete works incorrectly, as you can see when you search for Unique Value option, the autocomplete shows the Repeated Value options, and when you hover the cursor over the options the hover effect was incorrect, selecting randomly options, another bug is when you delete characters the Repeated Value option duplicates indefinitely

https://www.loom.com/share/5f1db4ea81634e45a8b2078d08bcc15e


V4 In this other video, I show the V4 version on Autocomplete works correctly, as you can see I search for Unique Value and it not repeat and not produces a bad hover effect.

https://www.loom.com/share/04bf71a1cc744a438d0c246ef135269e

If you need more info, please reply and request me, sorry for the bad english.

bcarusodev avatar May 31 '21 11:05 bcarusodev

@silvergraphs Thanks! That clarified the issue to me. I can't make any statement regarding the timeframe to fix this issue since I'm not sure if duplicate options are even useful.

eps1lon avatar May 31 '21 18:05 eps1lon

Just to add here, I believe there is a use case for 'duplicate' options. The scenario is where we want distinct options to have non-unique option labels.

In my use case the options are dictionaries and I use getOptionLabel to construct labels that the user sees. My func for getOptionLabel created duplicate labels for some options; this led to the odd filtering glitches @silvergraphs mentioned. e.g. getOptionLabel={(option) => option.name + ' ' + option.num}

As a workaround, we changed the option labels to be unique (I had to expose an id that should be hidden from the user). e.g getOptionLabel={(option) => option.name + ' ' + option.num + ' ' + option.id}

To hide this id from the user, I passed in a function to renderOptions. renderOption={(props, option) => <Box component="li" {...props}>{option.name + ' ' + option.num}</Box>}

This works until a user actually selects an option, then the search field is populated with the option label which includes the id that we would prefer to stay hidden

ramspeedy avatar Jun 24 '21 07:06 ramspeedy

Here is why the behavior is different between v4 and v5: #24213. I would recommend we go back to the initial solution proposed: https://github.com/mui-org/material-ui/issues/24198#issuecomment-752797748

oliviertassinari avatar Jun 27 '21 23:06 oliviertassinari

I am also having this issue here with repeated option values. We've had to downgrade in order to keep our ux intact. Along with the solution from @ramspeedy above, is it possible to separate between key and label values?

jyang619 avatar Jun 29 '21 19:06 jyang619

@jyang619 What's your use case for two options with the same label? @ramspeedy Do you have a visual example of the UI/UX generated? It's not clear to me.

oliviertassinari avatar Jun 29 '21 19:06 oliviertassinari

I also have problems with label duplications in v5 (I didn't tested it with v4). It shows me a warning in a console

index.js:1 Warning: Encountered two children with the same key, `My lovely label`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.

I expected to have some API to tell Autocomplete how to distinguish the options. For instance,

<Autocomplete
  options=[{ value: 1, label: 'Text', value: 2, label: 'Text' }]
  setCustomKey={option => option.value)
/>

@jyang619 What's your use case for two options with the same label?

I also asked myself what is a reason to have such case... I didn't find any logical answer for that. It seems like unrealistic example, at least for me.

In my case I removed duplications because I created options without validation on BE. After fixing it issue seems like disappeared.

enheit avatar Jul 05 '21 16:07 enheit

@oliviertassinari I have 2 search bars like this. Screen Shot The first one helps choose a filter value. And the second one allows users to search through either all of the items or a list of filtered items (assuming a filter value is selected).

ramspeedy avatar Jul 11 '21 06:07 ramspeedy

I have the same issue. My use case is that I use <Autocomplete /> for the a list of clients names. If 2 or more people with the same name, they will have the same label, and it will cause duplicate exactly like the CodeSandbox from the original post. Got this error "Encountered two children with the same key, Client Name".

tpmh31292 avatar Jul 16 '21 01:07 tpmh31292

Any updates on this? I also have the same issue..

pantelispanayiotou avatar Aug 06 '21 11:08 pantelispanayiotou

A workaround in case someone needs it:

<Autocomplete
  options={options}
  renderOption={(props, option) => (
    <li {...props} key={option.id}>
      {option.yourLabel}
    </li>
  )}
  ...moreProps
/>

Preserves the style and everything but makes sure every option has their own key

JohnnyCrazy avatar Aug 18 '21 12:08 JohnnyCrazy

@JohnnyCrazy solution solves the UI issue. But make sure that in <li> you add the unique key={option.id} after the {...props}.

sipian avatar Sep 06 '21 13:09 sipian

Same issue here: We show in the <Autocomplete> content that can be managed by the user (a.o. user profile names). Each item has a unique ID but the name might be the same. We ended up with same solution as @JohnnyCrazy : change props.key to use the item ID instead of the item name.

ronnyroeller avatar Oct 12 '21 07:10 ronnyroeller

Here is why the behavior is different between v4 and v5: #24213. I would recommend we go back to the initial solution proposed: #24198 (comment)

To solve the problem, I suggest adding a getOptionKey prop as mentioned in https://github.com/mui-org/material-ui/issues/29225#issue-1033735721 with a default value as you recommended in https://github.com/mui-org/material-ui/issues/24198#issuecomment-752797748

@akomm thank you for relating these two issues.

I have a same use case as https://github.com/mui-org/material-ui/issues/26492#issuecomment-881115298, here is the codesandbox demo. (Type "john doe" in the input, then try to select "john doe" by pressing arrow key down).

I'm going to use @JohnnyCrazy 's solution. Thank you for the workaround

doiali avatar Jan 28 '22 22:01 doiali

From the conversation thread, I agree that there could be duplicate options like in a list of names. However, what I don't get is how will the user know which correct option to select from among the duplicated ones. So is there a point of showing duplicated options?

ZeeshanTamboli avatar Feb 02 '22 14:02 ZeeshanTamboli

So is there a point of showing duplicated options?

It might happen unintentionally when we're getting options from an api call. Anyway in this case it shouldn't lead to a bug.

doiali avatar Feb 02 '22 15:02 doiali

So is there a point of showing duplicated options?

It might happen unintentionally when we're getting options from an api call. Anyway in this case it shouldn't lead to a bug.

Then I think that it will actually hide the bug if we fix it, Autocomplete will work correctly, but the bug is that there are duplicated options in an API call response which shouldn't happen in the first place since the user will get confused as to which option to select out of the duplicated ones.

ZeeshanTamboli avatar Feb 02 '22 18:02 ZeeshanTamboli

So is there a point of showing duplicated options?

It might happen unintentionally when we're getting options from an api call. Anyway in this case it shouldn't lead to a bug.

Then I think that it will actually hide the bug if we fix it, Autocomplete will work correctly, but the bug is that there are duplicated options in an API call response which shouldn't happen in the first place since the user will get confused as to which option to select out of the duplicated ones.

@ZeeshanTamboli not really, we can have a custom renderOption that show the difference, especially when we don't render simple strings but objects. In the user listing example could be adding an avatar image, and the userId could be the key.

The problem is that even with a custom renderOption, still the getOptionLabel is used as key. If I put my own key in the return of the renderOption the whole thing stops working.

While typing this... maybe it could work when still providing a getOptionLabel that returns the actual id of the option... will try and report back.

JanMisker avatar Feb 02 '22 23:02 JanMisker

we can have a custom renderOption that show the difference, especially when we don't render simple strings but objects. In the user listing example could be adding an avatar image, and the userId could be the key.

The problem is that even with a custom renderOption, still the getOptionLabel is used as key.

@JanMisker Got it ! That cleared it for me. Suppose in the user example, there could be more than one person with the same name but the avatars are distinct for the persons. But the key used in the getOptionLabel is the label (person's name).

Edit: But then if there is a renderOption prop used to distinguish, then this won't be considered as a "workaround" but a thing you must do if you want to support duplicate options, right? How would autocomplete know there are duplicate labels but the user is distinguishing them with an Avatar (in renderOption)? And when not using renderOption there shouldn't be duplicate options in the autocomplete for the user to not get confused, in case we support custom key. We don't provide because there shouldn't be duplicate options when not using renderOption prop.

ZeeshanTamboli avatar Feb 03 '22 04:02 ZeeshanTamboli

@ZeeshanTamboli I did some tests. The thing is, when I indeed do put my own key on the returned element, I get a visually blank list. So this doesn't work:

renderOption={(props, opt, state) => return <li {...props} key={opt.id}><Avatar user={opt} />{opt.name}<li>

I don't really understand why, but I did find that in useAutocomplete the result of getOptionLabel is assigned to a key parameter. It seems that the value returned from getOptionLabel is used also as the value of the autoComplete. Which makes some sense but this is actually more like a getOptionId functionality. https://github.com/mui-org/material-ui/blob/171942ce6e9f242900928620610a794daf8e559c/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js#L1064

Now my workaround is to do

getOptionLabel={(opt)=>opt.id}

but this means that I also have to adjust how the Chips are rendered (for multiple select).

Anyways the bottom line, I can work around it, but what put me off-guard is that the getOptionLabel function is used not only for rendering the option (which I expect to override with renderOption), but I still have to return a valid (unique) value to get it to work properly.

JanMisker avatar Feb 03 '22 21:02 JanMisker

The thing is, when I indeed do put my own key on the returned element, I get a visually blank list.

@JanMisker Can you provide a CodeSandbox?

ZeeshanTamboli avatar Feb 04 '22 04:02 ZeeshanTamboli

@ZeeshanTamboli ok there you have it... for some reason I can't get my CodeSandbox to work. Or better said, I can't get it to break. This is super annoying, in my own code in the custom renderOption I get back a node with children: [undefined, false, undefined] but only if I add a key. Will have to do some deep debugging to see why that happens.

For future reference, the codesandbox showing a working example https://codesandbox.io/s/affectionate-roman-9yy5d?file=/src/App.tsx

JanMisker avatar Feb 04 '22 12:02 JanMisker

Yeah, with the key it should work.

ZeeshanTamboli avatar Feb 07 '22 04:02 ZeeshanTamboli

@doiali it was the first thing I'v tried, however it was not the solution to the problem. I don't have the time now to pick it up again why it was the case.

akomm avatar Feb 07 '22 09:02 akomm

I still believe https://github.com/mui/material-ui/issues/24198#issuecomment-752797748 should fix the issue, no need to add new API for it.

mnajdova avatar Jun 08 '22 12:06 mnajdova

@mnajdova Agree

From what I understand, two options with the same labels mean that end-users have to way to know which option is which once selected. Proof:

https://github.com/mui/material-ui/blob/91905ce59b76455f77229f5d513ddd7b0cd08c30/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js#L169-L177

So bad UX. I can think of two possible options:

  1. To fail sooner, to have an explicit warning that forbids doing two options with the same label.
  2. To be a bit more flexible: https://github.com/mui/material-ui/issues/26492#issuecomment-1149828622. We could imagine cases where developers customize the renderOption to make it possible to differentiate the options. And for the label, to render some side visual clue on the side of the component.

Option 2 can make more sense, as there are customization ways to solve the options display uniqueness issue. In either case, we should revert #32910 as it seems to go after the symptoms, not the root problem (edit: reverted in #33142).

oliviertassinari avatar Jun 13 '22 13:06 oliviertassinari

It happens because of the Same key of options so you just need to give the different keys for all options after that it searches properly Even if the options label are the same

<Autocomplete
  options={options}
  renderOption={(props, option) => (
    <li {...props} key={option.id}>
      {option.yourLabel}
    </li>
  )}
  ...rest
/>

Note :- key(option.id) should be different for all Options

webdevlapani avatar Dec 20 '22 11:12 webdevlapani

A workaround in case someone needs it:

<Autocomplete
  options={options}
  renderOption={(props, option) => (
    <li {...props} key={option.id}>
      {option.yourLabel}
    </li>
  )}
  ...moreProps
/>

Preserves the style and everything but makes sure every option has their own key

thanks, i got it

emrahztrkx avatar Jan 23 '23 15:01 emrahztrkx

The propsed fix of ensuring you key the renderOption doesn't entirely solve the problem. There's two ways you get the non-unique console errors - non-unique outputs of renderOptions, and non-unique outputs of getOptionsLabel.

I appreciate there's a GIGO / bad UX argument to be made about resultsets that, while differing in ID, have identical data, but in systems that allow for user generated content undesirable data is going to happen. I could include the record ID in getOptionsLabel to solve my uniqueness problem for the key, but now my users are having to look at an ID, which is just a different flavour of bad UX. Single responsiblity principal appears to be rearing it's head again here.

I quite liked the idea of the getOptionKey solution https://github.com/mui/material-ui/pull/32910 but can see that's since been removed https://github.com/mui/material-ui/pull/33142.

I can't think of another solution at the moment, possibly because my mind is anchored on getOptionKey, but I do hope someone can resolve this issue, cos the currently recommended workaround is not suitable for all circumstances.

P.S. I'm quickly becoming a MUI convert. Striking a balance between configurability and simplicity of implementation is tough, and so far you've all done a great job on that front.

JizzyBob avatar Jan 27 '23 17:01 JizzyBob