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

Cursor jumps to end of input when inputValue is a controlled prop #217

Closed
robin-drexler opened this issue Oct 6, 2017 · 25 comments · Fixed by #230
Closed

Cursor jumps to end of input when inputValue is a controlled prop #217

robin-drexler opened this issue Oct 6, 2017 · 25 comments · Fixed by #230

Comments

@robin-drexler
Copy link
Contributor

  • downshift version: 1.11.0
  • node version: 6.9 (and codesadbox.io, could not find out, what they're using)
  • npm (or yarn) version: yarn 1.1.0 (and whatever codesandbox.io uses)

Problem description:

  • Visit this codesandbox
  • type text
  • move cursor to the left
  • type a letter
  • the letter will be added at the correct position, but the cursor will move to the end of the input instead of staying where you have typed

Relevant code or config

class Demo extends React.Component {
  state = {
    inputValue: ""
  };

  render() {
    return (
      <div>
        <Downshift
          onStateChange={data => {
            return (
              data.hasOwnProperty("inputValue") &&
              this.setState({ inputValue: data.inputValue })
            );
          }}
          inputValue={this.state.inputValue}
        >
          {({ getInputProps }) => (
            <div>
              <input {...getInputProps()} />
            </div>
          )}
        </Downshift>
      </div>
    );
  }
}
@kentcdodds
Copy link
Member

Thanks for the perfect reproduction issue!

I'm not sure why this is happening. It'll take a little digging. I'll try to get to it when I get the chance, but if you could give it a whirl that'd be great :)

@geoffdavis92
Copy link
Contributor

It seems @kentcdodds posted before I could share this 😆


As I try to debug this, it seems that the onStateChange update lags behind the native update of the caret/cursor. I used the function from this Stack Overflow answer to track the caret position in the input's onInput event handler, as well as in the Downshift wrapper's onStateChange event handler.

GIF of my test:

GIF of a test I did to track the onStateChange update versus the native JS caret position update

This looks like the setState call replaces the value of the text field after typing has finished, essentially overwriting what's in the field, and therefore moving the caret to the end of the text.

To be honest I'm not sure if Kent had intended for the input being "watched" by Downshift to be used as a controlled component in this way.


My solution

I made a test branch for this issue that seems to provide a solution to this issue.

There's a short description in the README, but basically I took that input and made it a class; the "controlled" part is contained within that input's class, while Downshift can still grab the input and use it internally.

Fixed caret issue, state lives in contained class

@kentcdodds
Copy link
Member

Hmmm... Interesting. I would expect that even when setState is called, react would make sure that cursor position would be preserved.... 🤔

I'd love to solve this in a way that folks controlling the inputValue don't need to worry about it...

@kentcdodds
Copy link
Member

Also, thanks so much for a thorough dig @geoffdavis92! That's really helpful!

@geoffdavis92
Copy link
Contributor

@kentcdodds happy to help.

From my testing, it seems this is the default behavior of any change of an HTML input's value attribute, as demonstrated with this Pen.

@robin-drexler
Copy link
Contributor Author

robin-drexler commented Oct 6, 2017

It smells like: https://stackoverflow.com/questions/28922275/in-reactjs-why-does-setstate-behave-differently-when-called-synchronously

The issue also vanishes if one moves the this.props.onStateChange call above the setState call in internalSetState. Just for testing of course, as quite some other things break in that case.

I tried to reproduce the same behavior in a smaller example without Downshift, but wasn't able yet.

Is there anything async I'm missing?
I've noticed the debounce, but removing it also does not seem to do the trick.

@robin-drexler
Copy link
Contributor Author

robin-drexler commented Oct 6, 2017

Reproduced without downshift: https://codesandbox.io/s/036r6xkr7w

The issue seems to be that upon setState children are re-rendered.
When this happens, the value property of the underlying input field dom node already has changed to the new value, while the result of getStateAndHelpers still contains the old value from the prop. Only after this.props.onStateChange is called, the prop also has the new value.
Since react already has rendered the input with a new value, it now has to touch/change the value again, leading to the cursor moving to the end.

A possible fix that came to mind could be to put the controlled props into the state of Downshift and sync them in componentWillReceiveProps instead of reading them from props later on. It could also be done for inputValue only and the rest kept as is.

@kentcdodds
Copy link
Member

I'd be open to look at a PR for that, but I'd reeeeally like to avoid it if there's any other way to do it...

Alternatively, here's an easy solution: https://codesandbox.io/s/pj5n3xx07

Will that work?

@robin-drexler
Copy link
Contributor Author

Also thought about something like this, however, we currently use:

if (data.hasOwnProperty('inputValue')) {
   this.props.onChange(data.inputValue);
}

in onStateChange to call a passed in onChange hander. This works for the input's onChange, but also when the user has selected something from the list. The input's onChange does not trigger in that case.

But I think this could be fixed by using the input's onChange in addition to onStateChange and by adding an additional check for the presence of selectedItem there to avoid invoking the passed in onChange twice.

@kentcdodds
Copy link
Member

Feel free to open up a pull request and we can talk about it. But know that I probably wont accept a PR that attempts to synchronize state with props. I'm sure we can do this another way. Maybe an onInputValueChange prop?

robin-drexler added a commit to robin-drexler/downshift that referenced this issue Oct 17, 2017
provide an onInputValueChange callback to allow to update controlled prop
before its old value is used to re-render the children
causing the cursor in the input to jump to the end in the next render

downshift-js#217
@slavab89
Copy link
Contributor

Is this usable when using "multi selection"?
I'm asking this because i've tried to remove my onChange handler of input and use onInputValueChange to get updates on the inputValue changes.
The issue is that once an item is selected and added to the array of the controlled selectedItem prop, it triggers onInputValueChange with the whole contents of the array basically (here).
I can try and filter the updates i receive in onInputValueChange to only actually input changes but dont think its the right way to go.

This function also triggers for me when i'm selecting an item (when isOpen changes to false) but i might be missing something on my side on this..?

@kentcdodds
Copy link
Member

Hi! I'm afraid I don't have too much time right now, but I made this last night and it might help you: https://github.com/kentcdodds/advanced-downshift

Notice that I just set the selectedItem to null and instead of controlling the input value, I use reset to clear the input.

@slavab89
Copy link
Contributor

WOW!! That looks awesome! Going to have to change some code on my side 😆
A lot of code in the render functions but still nice :) Does it have some performance impact when you do all those render inside render dynamic stuff?

In any case I understand your point and its a very nice approach 😄

@kentcdodds
Copy link
Member

Does it have some performance impact when you do all those render inside render dynamic stuff?

Maybe? But not enough to be concerned about.

Good luck!

@franklixuefei
Copy link
Collaborator

Hi @kentcdodds
Sorry, but I haven't figured out how you fixed the cursor jumping problem in your advance downshift demo without using onInputValueChange()? And also, if I want to pass in default selected recipients, how would you do that? I tried defaultSelectedItem prop but it doesn't work in your demo...

Thanks!

@austintackaberry
Copy link
Collaborator

I used the codesandbox from advanced downshift and did not have the cursor jumping issue:

undefined

@franklixuefei
Copy link
Collaborator

Thanks @austintackaberry but I still don't know how this was achieved without onInputValueChange().
@kentcdodds
Sorry, but I haven't figured out how you fixed the cursor jumping problem in your advance downshift demo without using onInputValueChange()? And also, if I want to pass in default selected recipients, how would you do that? I tried defaultSelectedItem prop but it doesn't work in your demo...

Thanks!

@franklixuefei
Copy link
Collaborator

franklixuefei commented Mar 7, 2018

NVM, I figured it out.

  1. @kentcdodds 's demo is working because in his demo inputValue is not a controlled (by us) prop, so that the cursor problem will not happen at all.
  2. In his demo, selectedItem is assigned null so that this API is disabled, thus defaultSelectedItem is not working either, as expected.

If I understand correctly, the defaultXXX props will only be useful when the corresponding control props are uncontrolled. Please correct me if I'm wrong @kentcdodds

Thanks!

@austintackaberry
Copy link
Collaborator

Below is a demo that has inputValue and defaultSelectedItem as control props. The defaultSelectedItem prop works as expected and there is no jumping cursor issue.

https://codesandbox.io/s/5konk5137n

Let me know if this isn't what you're looking for! 😄

@franklixuefei
Copy link
Collaborator

@austintackaberry Thanks! This is what I'm looking for. I think you can also use the onInputValueChange() prop which is already provided. But either works.

@rishith-p
Copy link

If you are using a value from the redux store in the textfield and updating the value of the redux store onChange. The problem is observed from react-redux version 6.0.1 and higher. Change your react-redux version to 6.0.0 and the cursor jumping problem should be solved.

@slorber
Copy link

slorber commented May 12, 2020

I also had this problem, with the useCombobox hook.

The only solution I found so far is to let combobox controls the input state, and forward this state to parent components in useEffect.

  useEffect(() => {
    if (combobox.inputValue !== value) {
      onChange(combobox.inputValue);
    }
  }, [combobox.inputValue, value]);

@kentcdodds
Copy link
Member

kentcdodds commented May 12, 2020

That sounds like a workaround for an unrelated bug @slorber. Could you open a new issue for that to be tracked?

@john-rodewald
Copy link

john-rodewald commented Jan 9, 2023

I am still running into this problem when:

  • inputValue is controlled from outside (useState)
  • onStateChange is set in the useCombobox call

I'm encountering this on v6.1.7 and on v7.1.12.

@slorber's workaround fixes the issue.

@rodogir
Copy link

rodogir commented Jan 17, 2023

@john-rodewald and anybody else who is running into the same issue with hooks

this worked for us and doesn't require useEffect: #1108 (comment)

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