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

Controlled input + reselecting of item issue #163

Closed
slavab89 opened this issue Aug 31, 2017 · 26 comments
Closed

Controlled input + reselecting of item issue #163

slavab89 opened this issue Aug 31, 2017 · 26 comments

Comments

@slavab89
Copy link
Contributor

  • downshift version: 1.3.0

I've taken control over the inputValue to be able to render the wanted menu items (Somewhat related to my other opened issue).
Please open the linked sandbox code and do the following steps:

  1. Click on the select box and choose some value
  2. Select all the text in the input and rewrite it - write the letter l for example
  3. Choose one of the values that show

You'll see that the input and state returns to the initial selected option (instead of the new one).

Now, uncomment both lines that say Try to remove. Includes the onMouseDown onClick events and do the above steps once again.
You'll notice that the state now changes correctly.

I think it has something to do with the onBlur event + react state change that overshadows and cancels the item click event.

Reproduction repository:
https://codesandbox.io/s/xy66ml554

Please let me know if you're able to reproduce that.

@tansongyang
Copy link
Collaborator

I tried reproducing your issue and couldn't. My steps:

  1. Click on select box.
  2. Click on 4-LOM.
  3. Select all text and write l.
  4. Click on Biggs Darklighter.

I see Biggs Darklighter is now selected, which is what I would expect.

Perhaps you could try posting the exact options you chose? Or a screen capture?

@slavab89
Copy link
Contributor Author

slavab89 commented Sep 7, 2017

Thanks a lot for looking into that!
Apparently the sandbox that i've posted is the one with the workaround already 😄 . To make the issue appear, comment the lines with the Try to remove (Those should not be there naturally, they do the workaround)
Here is a screen capture of the issue (Hope it helps)

downshiftbug mov

@tansongyang
Copy link
Collaborator

tansongyang commented Sep 7, 2017

Thanks for clearing that up. I see the issue happening now.

There are a lot of other things going on here, and I see a lot of other dependencies. Would you please see if you can create a minimal example? If we take out all other dependencies, and we're just working with raw React and downshift, does this problem still happen? This would make it easier to track down the issue.

@slavab89
Copy link
Contributor Author

slavab89 commented Sep 7, 2017

Ok so i've stripped it as much as i can
https://codesandbox.io/s/wy3jv4lqvk

Key points to notice:

  • selectedItem must be controlled
  • items needs to be "filterable"

Further weird behavior of the issue:

  1. Click on selectbox and select "First"
  2. Select the whole text and press "t"
  3. Select "Third" - Issue happens

Now refresh the page and do the following

  1. Click on selectbox and select "First"
  2. Select the whole text and press "s"
  3. Select "Second" - NO isssue

Between the different value changes on the above steps you can clearly see a small "flicker" of the value in the input. If you uncommend both event handles for onMouseDown and onClick you wont see this flicker or issue anymore.

This is a very strange behavior, i'm not sure what to point at.

Same as previously, he's a recording showing this (Sorry about the weird overlay that you see there)
downshiftbug2 mov

@tansongyang
Copy link
Collaborator

tansongyang commented Sep 7, 2017

Thanks a lot for doing that. @kentcdodds Can you shed any light on this? I took a stab at looking through the downshift source and it's not immediately clear to me what the issue might be.

@slavab89
Copy link
Contributor Author

slavab89 commented Sep 7, 2017

This may be somehow related to facebook/react#4210 or facebook/react#2291
Based on another search i reached the info about onMouseDown vs onClick when using onBlur
https://stackoverflow.com/questions/17769005/onclick-and-onblur-ordering-issue
It might be related to the ordering of events since downshift does listen to onClick when clicking on an item...?

@tansongyang
Copy link
Collaborator

Actually, I'm starting to doubt that the issue here is with downshift. If you just type t in the beginning, and try to select Third, it doesn't work. You could try comparing your implementation with the canonical autocomplete implementation.

@kentcdodds
Copy link
Member

Hmmm... Looking at the reproduction it appears to me that you're trying to control the inputValue without actually controlling the inputValue which leads to behaviors which (I'm afraid) I don't have time to dig deeper and understand. I suggest you do something like the typeahead example instead.

@tansongyang
Copy link
Collaborator

@kentcdodds @slavab89 Since this doesn't appear to be an issue with downshift itself, can we close this issue? We would reopen if there is more conclusive evidence that the issue is with downshift.

@kentcdodds
Copy link
Member

Agreed 👍

@kentcdodds
Copy link
Member

We can continue to chat in here though, if you have more questions about this @slavab89 :) We'll help when we can.

@slavab89
Copy link
Contributor Author

slavab89 commented Sep 8, 2017

I've simplified it even further to show you what i mean.
https://codesandbox.io/s/34nypnlpkm
So the inputValue is controlled normally through state, simple strings (no nulls etc) as you can see.
The thing that causes the "issue" to appear is attaching onBlur + setState inside it on the input. I am doing that because onBlur i want to clear the value of the input to an empty string when its not focused.
Remove the onBlur and it works as usual.

I believe that what you did to fix it inside downshift is exactly what i need. You prevent the setState in onBlur while the mouse is still down.
If i could get access to the isMouseDown that you're using i would be able to do the same logic (Just so that i wont have to register the events once more and do the same that you do).

The other "fix" or workaround that i did was listen to the onMouseDown of an item and do preventDefault which seems to do the trick

I agree that it is not a bug in downshift but somewhat expected react + browser behavior however strange it may be.
Just to clarify this once again: onBlur + setState overshadows and cancels onClick events. To overcome that, you should either stop not doing setState (at all or while the mouse is clicked) OR register a mouseDown event and do preventDefault which seems to work.

@kentcdodds
Copy link
Member

Ah, I think that I understand what you're trying to accomplish. You just want the input to be cleared whenever the menu is not open right? How's this?

@slavab89
Copy link
Contributor Author

slavab89 commented Sep 9, 2017

Interesting!! didnt think about using onStateChange for that. Thanks!!

BTW i didnt say this before but this project is great!! allows for a lot of flexibility and helped me to get the thing i've been trying to do for quite some time: Email input same as gmail (Autocomplete + multi select + editable selected values / tags - the part that's missing in all other select boxes that i've tried)

@kentcdodds
Copy link
Member

That's great! Do you think you could make an example and add it to the list of examples in the README? That'd be awesome.

@slavab89
Copy link
Contributor Author

So i've started to build something, its kinda a WIP but wanted your input on this. I didnt invest in the UI much so it looks kinda weird but the concept is similar to what react-select is doing.
I've used react-input-autosize from react-select because of the autosize feature which is nice
https://codesandbox.io/s/o4yp9vmm8z

The general concept is to use something similar to the multiple example that you already have but have each of the items as a editable tag + also listening to the key stokes of the main input for chars like , <tab> <backspace> etc. and act accordingly.
Not 100% sure why it gives me an uncontrolled Input once i press backspace to remove some item.

The whole thing contains the most basic functionality so in order to have more features like focusing and overall more UX friendly behavior one would need to add more code. I can add it but i'm not sure if its in the scope.
Also, please let me know what to fix or change there =]

@kentcdodds
Copy link
Member

This is great! Thanks for your work on it! This is exactly the kind of experience I was hoping to make easier with downshift. I don't think you could build this kind of thing as easily with other autocomplete solutions...

Not 100% sure why it gives me an uncontrolled Input once i press backspace to remove some item.

I'm not sure I understand. Everything seems to be functioning fine to me.

I don't see anything wrong with this functionally. Could use some clean up from a design perspective, and some accessibility things, but I think it's a great direction!

@slavab89
Copy link
Contributor Author

Nice! 😄 I'll be able to work on it a bit more close to the weekend so i'll update.

@slavab89
Copy link
Contributor Author

I've updated the example. Made it a bit more pretty to the eyes and more convenient to use.

Myself, i'm using material-ui library so you can easily swap the TagValue render with a Chip element (At least in 0.19). You would also have to play a bit with CSS to make it look more materialized and add an underline.
In version 1.0, its somewhat harder when using FormControl and Input so when i have time and get to it i'll try to make it work (Without having to write the whole CSS myself)

@kentcdodds
Copy link
Member

It's looking better and better!

@slavab89
Copy link
Contributor Author

Could you please direct me in what additional things should be done in terms of this example?

A material-ui example of this would be great but it will take in a while to get to it i believe + it will be a different example probably.

@kentcdodds
Copy link
Member

I think it looks great! You might just remove the console logs and then make a PR to add it to the list in the README :)

@kentcdodds
Copy link
Member

Oh, also, I'd suggest using match-sorter rather than just the built-in filter. See how to use that in the base example: http://kcd.im/ds-example

@zxxc
Copy link

zxxc commented Sep 26, 2017

I'm facing same problem, and we already have a lot of code and validation onBlur with setState. Do we have a workaround or fix for this? as i understand onMouseDown will cause a11y issues

@slavab89
Copy link
Contributor Author

@zxxc its a pretty long read :) but you can refer to this comment. Basically, you can make your changes and validation in onStateChange

@zxxc
Copy link

zxxc commented Sep 26, 2017

i'm not using downshift, simply react

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants