Connected text input cursor position reset on input

[thefloweringash]

I have an input field connected to a part of a redux store. When I type in the text field the cursor position is reset to the end of the text field. For example filling in "123", then attempting to insert "abc" at the beginning results in "a123bc". I've been unable to isolate the exact circumstances that cause this, but have a small reproduction at https://jsfiddle.net/1fm2cczq/. Occurs on 5.0.0-beta.3. Does not occur on 4.4.5.

[jimbolla]

Hmmm yeah good find. I can reproduce this in my own app. This seems related to facebook/react#955, and my first hunch is that changing the subscriptions from firing bottom-up to top-down reveals this behavior. Will need to investigate further.

[jimbolla]

As an experiment, I wrapped an <input> with this component to see what happens, and this does correct the behavior. So it seems to me that the issue is related to controlled inputs needing to be updated earlier or else React sorta goes off the rails with cursor position.

class Input extends React.Component {
  constructor(props, ...args) {
    super(props, ...args);
    this.state = { value: props.value };
  }

  componentWillReceiveProps(nextProps) {
    if (this.state.value !== nextProps.value) {
      this.setState({ value: nextProps.value });
    }
  }

  onChange(evt) {
    this.setState({ value: evt.target.value }, () => this.props.onChange(evt));
  }

  render() {
    return (<input {...this.props} {...this.state} onChange={this.onChange} />);
  }
}

[jimbolla]

I created a custom build of react-redux v5 that disables the top-down ordering so that subscriptions behave like v4. The cursor issue went away. So I think I know what the problem is, but I don't yet have a great solution. So far, the first idea I have is to provide another option for connect that tells it to prioritize its store subscription before those of its parents. This option would have to be enabled for controlled inputs and textareas (I'm assuming) as a workaround for facebook/react#955 until that's fixed (if ever.)

[markerikson]

Ew. This... sounds nasty.

[jimbolla]

@markerikson Indeed it is.

As another experiment, I just tried switching React for Preact in my own project and the bug goes away.

[markerikson]

Hmm. Pinging @gaearon and @timdorr for thoughts...

[jimbolla]

Is there a build of react master available somewhere? I'd like to test this against that because I know there are other bugfixes related to controlled inputs in master that are supposedly gonna land in react v16.

[jimbolla]

@johnnycopperstone From what I gather from facebook/react#955, there are (at least) 2 scenarios in which this bug manifests itself, one being not updating the input's value prop soon enough (which is what I think is happening with react-redux v5) and one related to updating the prop with a value different than what's in the textbox, typically because of data altering (input masking or whitespace stripping, for example). Are you doing anything like that?

[jonathancopperstone]

Hey @jimbolla Sorry didn't see your reply - I just removed my comment as I realised it was a different issue and didn't want to pollute this issue. I was still using 15.0.0 and the cursor fix was in 15.0.1

[timdorr]

As another experiment, I just tried switching React for Preact in my own project and the bug goes away.

Well, Preact is pretty awesome, so I would suspect it's not going to see issues like this 😄 (Also, it doesn't have the huge event model that React does. Simpler code == less bugs)

Is there a build of react master available somewhere? I'd like to test this against that because I know there are other bugfixes related to controlled inputs in master that are supposedly gonna land in react v16.

I'm not sure what's in it, but react@next is currently on 15.4.0-rc.4, so it might have some of that in there? It's from 10 days ago.

[jimbolla]

@timdorr True, but our answer can't just be "use Preact instead" unfortunately. What I'm thinking...

• IMO, the new top-down ordering is the "right" way to do it, and much of the perf gains and many of the bug fixes are because of it. We could remove it, but that undoes a lot of the benefits of v5.
• This does seem like a bug in React that the new subscription ordering is exposing, but one that doesn't seem easy to fix, and even if it was, that fix wouldn't make it to release in a timely fashion, such that we can ignore it. I did pull and build react master, and the bug still exists.
• I'm guessing this bug might manifest itself in react-redux v4 if redux were to fire subscriptions in reverse order because of how it relates to the timing of which components receive new props first. I might try test this just for awareness of the issue.
• If we keep top-down as the default behavior, we need to make it as painless as possible to work around this issue, and make devs aware of it.
  • While it's certainly solvable in userland using the code provided above, I'd hate to force that on everyone, lest we be accused of creating javascript fatigue. 👅
  • The best solution I have right now is for connect (actually connectAdvanced) to offer an opt-in option that would allow that component to skip the subscription tree and subscribe directly to the store, making it work like v4. Devs that are using controlled text inputs would have to set this option to true. I don't know what to name this option. Libraries like redux-form would have to handle this as well. Docs would have to stress this as it's a bug that isn't discovered easily, because I doubt many devs test entering values in the middle of an input.

[timdorr]

Oh no, I'm not suggesting everyone use Preact, just that it's awesome in its own right. That's ancillary to this issue.

[gaearon]

Devs that are using controlled text inputs would have to set this option to true.

IMO this will be extremely confusing and hurt the ecosystem. There are enough gotchas already, we should fix this in the library rather than add options.

[gaearon]

one being not updating the input's value prop soon enough (which is what I think is happening with react-redux v5)

I think that if you update it while event is being handled, React should understand it. If not, it's a bug and I'm happy to look into it, given a pure React (no RR) reproducing case.

[jimbolla]

I think I can produce a vanilla React repro.

[jimbolla]

Here's a repro of the issue using redux + react-redux. I'm going to inline those to distill it down to vanilla react.

import React, { Component } from 'react';
import { createStore } from 'redux';
import { Provider, connect } from 'react-redux';

const store = createStore((state = {}, action) => {
  return action.payload
    ? action.payload
    : state;
});

const Parent = connect(state => state)(props => props.children);

class BlockUpdates extends Component {
  shouldComponentUpdate() { return false; }
  render() { return this.props.children; }
}

const TextBox = connect(
  state => ({ value: state.value }),
  { 
    onChange: evt => ({
      type: 'CHANGE',
      payload: { value: evt.target.value }
    })
  }
)(props => (<input type="text" id="demo" {...props} />));

const App = () =>(
  <Provider store={store}>
    <Parent>
      <BlockUpdates>
        <form>
          <TextBox />
        </form>
      </BlockUpdates>
    </Parent>
  </Provider>
);

export default App;

[jimbolla]

And here's the vanilla React version, that reduces (no pun intended) the issue down to its core:

import React, { Component } from 'react';

let currentState = { value: '' };

class BlockUpdates extends Component {
  shouldComponentUpdate() { return false; }
  render() { return this.props.children; }
}

class TextBox extends Component {
  render() {
    return (
      <input
        type="text"
        id="demo"
        onChange={evt => this.props.setState({ value: evt.target.value })}
        value={currentState.value}
      />
    );
  }
}

class App extends Component {
  render() {
    return (
      <form>
        <BlockUpdates>
          <TextBox
            ref={c => this.textbox = c}
            setState={newState => { 
              const makeTheCursorJump = true;

              currentState = newState;
              if (makeTheCursorJump) {
                // my guess is that the code that affects the cursor position executes before
                // setState's callback, meaning the callback code doesn't get a chance to be
                // a part of that process.
                this.setState({}, () => this.textbox.setState({}));
              } else {
                // if the textbox updates first 
                this.textbox.setState({});
                this.setState({});
              }
            }}
          />
        </BlockUpdates>
      </form>
    );
  }
}

export default App;

[Guria]

@gaearon
repro with react only:

import React from 'react';

const format = value => {
  // any transform here
  return value.replace(/\d/g, 'a')
};

class HelloWorld extends React.Component {
  constructor(props) {
    super(props)
    this.state = {
      value: '0000000'
    }
  }

  render () {
    return <input value={this.state.value} onChange={e => this.setState({
        value: format(e.target.value)
      })} />
  }
}

export default HelloWorld;

[gaearon]

@Guria If you change the value right after input, the cursor jump is expected. React can't guess where to put the cursor. So that is not a bug.

[gaearon]

@jimbolla This does not look like a bug to me. It is documented that this.state contains the rendered value of the state. There is no guarantee that calling setState() will update this.state synchronously. Therefore, by reading from this.state you won't get the just-updated value.

[jimbolla]

@gaearon But It's happening as part of the callback of setState(), which should have the new state. But even still, if I store the state in a global variable and read from there instead of component state, it behaves the same way. This seems more related to when React reconciles the current value of the input's value prop with what's actually in the DOM element. Basically, by the time the callback to setState fires, it's too late. I can update the code to make this more clear.

[gaearon]

Oh okay. setState() callback fires after the DOM has been updated. Think of it as componentDidUpdate(). So this also seems expected unless I'm missing something. Generally I don't recommend using setState() callbacks at all precisely because lifecycles do the same but better.

[gaearon]

It would help my understanding if you showed a snippet with a global variable. The smaller example the better.

[Guria]

Ouch, forgot about that render caused by setState is not synced with event. So my example definetely invalid.

If you change the value right after input, the cursor jump is expected. React can't guess where to put the cursor. So that is not a bug.

Then is there a right way to make masked value with controlled input? Looks like an overkill to make class component here when it is just about transorming a value.

[jimbolla]

@gaearon I updated my above example to use global variable. You can toggle the makeTheCursorJump variable to see the 2 different behaviors. This is the core difference between react-redux master and next in its simplest form. A lot of the perf gains and the bugfix related to props/state being out of sync boiled down to this change.

[gaearon]

It's a bit hard to tell what's going on but I can look deeper into it. My intuition is that you should almost never use setState callback, it's just a legacy API that happens to stick around. It has other weird edge cases too (e.g. it won't get called if you setState inside componentWillMount on the server). Unless I'm mistaken, it also happens after the changes have been flushed to the DOM, so setState inside setState callback is a cascading render and generally not very good.

Could you explain why setState callback is useful to you, and why you'd rather wait for it than update the value immediately. Is this callback the thing you rely on to make setStates come in the parent-to-child order?

[jimbolla]

Using the callback fixes #292, #368, and the many related issues by ensuring children components never recalculate props and rerender with stale props their parents.

[timdorr]

Fixed via #540. Will be changed to default off when React 16 is released and we can push a 5.1 version.

[istarkov]

Hi guys,

I've read this thread and the source of connectAdvanced.js and I have one question,
why not just move notifyNestedSubs from this line this.setState(dummyState, notifyNestedSubs) into componentDidUpdate

So you will get something like

subscription.onStateChange = function onStateChange() {
...
  if (!this.selector.shouldComponentUpdate) {
    subscription.notifyNestedSubs()
  } else {
    this.callOnDidUpdate = true;
    this.setState(dummyState)
  }
}
....
  componentDidUpdate() {
    if (this.callOnDidUpdate) {
      notifyNestedSubs()
      this.callOnDidUpdate = false
    }      
  }

I've created jsbin example based on vanila React example and it has no cursor bug.

What the real need to call nested subs notification in setState callback vs in componentDidUpdate ?

PS: I wanna say that setState callback in React@16 will be somehow similar to didUpdate,
so if didUpdate solution above has some problems with current React version, looks like setState solution will have the same problems in React@16

[jimbolla]

Hmmmm. I could swear this did not work when I was testing it. But here it is. Well, I'll have to give this another go. Assuming this doesn't have significant perf impact or any other problems, this would eliminate the need for that stop-gap setting.

[jimbolla]

@spicyj I was just wondering if you were able to make an automated test from this. If so, I'd like to see it for my own education.

[earlsioson]

Hello, before posting a separate issue, can anyone here confirm that the behavior demonstrated in the following demo site is related to this issue...

To replicate the behavior, insert cursor after 42.00 in the input field. Hit the delete key twice and notice how the cursor jumps to the beginning of the input field.

demo site: https://earlsioson.github.io/react-redux-input
source: https://github.com/earlsioson/react-redux-input

[derwaldgeist]

I just stumbled upon this problem and am quite confused after having read most of these comments here. Has this problem been fixed in the core somehow, and if yes, in which version? Or do I still need a kind of hack to prevent the cursor from jumping to the end of the input field?

[markerikson]

@derwaldgeist : It should be fixed with the current "latest" versions of React and React-Redux.

Are you seeing this behavior? If so, what versions are you using now?

[markerikson]

I take that back. If I'm reading this correctly, the change on the React side that resolves this is merged in, but won't be released until 16.0?

[jimbolla]

Pretty sure this is totally fixed. Not sure what the exact version though.

[derwaldgeist]

Thanks for the info. Will check if I am on the latest versions of React and Redux, then.

[linde12]

This worked for me when using [email protected] but broke when i upgraded to [email protected] where the initial fix was removed, and replaced by some componentWillUpdate fix. After upgrading to 5.0.0-rc.2 or anything higher, e.g. 5.0.5, i get the same behavior as before with the jumping cursor.

I'm not sure if this has anything to do with other libraries that interfere somehow. I'm using redux-form to update my field values, so maybe it has to do with that.

I'm having a hard time seeing how that would change anything, but it might. I will try to isolate the problem.

[jimbolla]

@linde12 Can you provide a repro? I'm also using redux-form and it works fine.

[linde12]

@jimbolla I've narrowed down the problem and slimmed down the code and it seems to be in combination with the UX library Grommet, it doesn't happen with "vanilla" <input> elements or other components that wrap inputs.

I'm doing something like this:

import {TextInput} from 'grommet';
const renderText = ({input}) => {
  // Try commenting out this block and un-commenting the other block
  return (
    <TextInput onDOMChange={e => input.onChange(e)} value={input.value} />
  );
  //return (
    //<input onChange={e => input.onChange(e)} value={input.value} />
  //);
}

const TestForm = ({handleSubmit}) =>
  <div>
    <form onSubmit={handleSubmit}>
      <Field
        name='Name'
        component={renderText}
      />
      <button type="submit">Ok</button>
    </form>
  </div>;

export default reduxForm({
  form: 'TestForm'
})(TestForm);

I'm using it with a empty reducer(apart from the form reducer provided from redux-form) and just a <Provider store={store}></Provider> wrawpped around the TestForm. I will try to dig deeper to see what's causing the problem, it might not have anything to do with react-redux. I will report back if i ever find out!

[linde12]

@jimbolla I think i've found the culprit. In Grommet's onChange proxy they do a this.setState({...}) to update various things, which seems to trigger a re-render with the "old" input value because the new value wasn't quick enough to arrive from props. So when the new prop arrives there is a difference between the actual value in the <input> and the "new" value coming from props.

However, the fix in 5.0.0-rc.1 seemed to fix this. Any idea what to do next?

[linde12]

@jimbolla (and anyone else interested)

I made a fiddle demonstrating the problem. You can try typing anything(e.g. "aaaa") and put your cursor in the middle("aa|aa") and type something else(e.g. "z") which results in "aazaa|".

Steps to reproduce:

1. Type "aaaa"
2. Click in the middle of the text, like so "aa|aa"
3. Type "z"
4. See that with [email protected] the cursor placement now is at the end("aazaa|")
5. See that with [email protected] the cursor placement stays in place("aaz|aa")

So the fix that was reverted in [email protected] (and replaced by another fix) worked.

Here are the two fiddles:
[email protected] (bug shows) - https://jsfiddle.net/hqoohq4b/
[email protected] (works) - https://jsfiddle.net/hqoohq4b/1/

[derwaldgeist]

I am not using Grommet and have the same problem with a plain vanilla <input>

[linde12]

@derwaldgeist Are you manipulating state(with setState) in someway? Or are you formatting your input(because that would probably require you to keep track of the cursor yourself somehow)?

Is there any reason why the fix in [email protected] was replaced? Is it something we can take in today? If yes, i'd be happy to help as we are currently considering what options we have to fix this.

[derwaldgeist]

@linde12 Nope, I am just using Redux to update the store based on user input (i.e. in an onChange handler). I am currently using [email protected]. My app is running inside Electron, if that matters (it shouldn't, if you ask me).

[linde12]

@derwaldgeist I don't think it should either, do you have some code where you can reproduce the problem? Maybe i can verify if it's similar to the problem i am experiencing.