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.

[gaearon]

Can you describe a high level overview of how you use this callback to solve this problem?

[jimbolla]

Using the callback prevents the child components from receiving notifications that the store has updated, which triggers their update process, until after their parents have guaranteed to be giving them updated props.

In cases where there isn't an intermediate component that is blocking updates by overriding shouldComponentUpdate, the child components first see the new state and new props simultaneously during their componentWillReceiveProps as a result of the parent's render call. In this case, the cursor bug doesn't show up because react reconciled everything in a single pass, and the store notification ends up being a NOOP for the child component.

In cases where there is a blocker component (like <BlockUpdates> above), the children won't receive new props, but they'll be informed of new state from their subscribe callback, during the parent component's setState callback.

[gaearon]

Can we use ReactDOM.unstable_batchedUpdates instead?

[jimbolla]

I'm having trouble finding docs or source code for that. Is that in ReactDOM? Wouldn't that mean adding a dependency to react-dom to react-redux?

[jimbolla]

If I modify the above code like so:

ReactDOM.unstable_batchedUpdates(() => {
  this.setState({}, () => this.textbox.setState({}));
});

the cursor bug still exists. If I modify it like this:

ReactDOM.unstable_batchedUpdates(() => {
  this.setState({});
  this.textbox.setState({});
});

or like this:

ReactDOM.unstable_batchedUpdates(() => { this.setState({}); });
ReactDOM.unstable_batchedUpdates(() => { this.textbox.setState({}); });

the cursor bug goes away.

If I make the same change to Connect, the "should pass state consistently to mapState" test fails due to an increase in calls to mapState, but does not fail because of the wrong value being passed. Here's a diff of the relevant test to get it to pass:

diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js
index 2b63bdb..60f818f 100644
--- a/test/components/connect.spec.js
+++ b/test/components/connect.spec.js
@@ -1717,16 +1723,16 @@ describe('React', () => {
       ReactDOM.unstable_batchedUpdates(() => {
         store.dispatch({ type: 'APPEND', body: 'c' })
       })
-      expect(childMapStateInvokes).toBe(2)
+      expect(childMapStateInvokes).toBe(3)

       // setState calls DOM handlers are batched
       const container = TestUtils.findRenderedComponentWithType(tree, Container)
       const node = container.getWrappedInstance().refs.button
       TestUtils.Simulate.click(node)
-      expect(childMapStateInvokes).toBe(3)
+      expect(childMapStateInvokes).toBe(5)

       store.dispatch({ type: 'APPEND', body: 'd' })
-      expect(childMapStateInvokes).toBe(4)
+      expect(childMapStateInvokes).toBe(6)
     })

     it('should not render the wrapped component when mapState does not produce change', () => {

when I do the update like this:

  ReactDOM.unstable_batchedUpdates(() => { this.setState(dummyState) })
  ReactDOM.unstable_batchedUpdates(() => { notifyNestedSubs() })

I was suspect of this, so I added logging like so:

      @connect((state, parentProps) => {
        console.log(JSON.stringify(state))
        console.log(JSON.stringify(parentProps))
        console.log()

        childMapStateInvokes++
        // The state from parent props should always be consistent with the current state
        expect(state).toEqual(parentProps.parentState)
        return {}
      })

and the output was this:

"a"
{"parentState":"a"}

"ac"
{"parentState":"a"}

"ac"
{"parentState":"ac"}

"acb"
{"parentState":"ac"}

"acb"
{"parentState":"acb"}

"acbd"
{"parentState":"acbd"}

Notice the 2nd and 4th outputs should have actually caused the test to fail. But the failed assertions are swallowed inside batchedUpdates.

[jimbolla]

Actually it looks like the exceptions are getting swallowed because Connect wraps the selector functions in a try/catch and then rethrows in its render, but since it that particular render doesn't happen because it immediately gets recomputed with good values.

[gaearon]

My point with unstable_batchedUpdates() was that it's the API designed for what you're trying to do (batching updates).

setState callbacks were designed for a different purpose (same as lifecycle hooks). So they're not the right choice here, and any direction forward will likely involve using unstable_batchedUpdates().

It's unstable because in the future, React will batch updates by default. There are also some caveats. For example it depends on ReactDOM but we also need RN support. Relay solves it by having two different files (presumably RN chooses .native.js although I don't know for sure).

[jimbolla]

batchedUpdates doesn't solve the problem though. The problem is child component updates need to be deferred until parent updates have completed and propagated new props to the children.

[jimbolla]

batchedUpdates seems to solve the issue of batching render calls, which isn't the problem here. The problem is that the subscribe listener is being fired before it has received updated props from its parent, unless you put it in the setState callback. In order to take advantage of batchedUpdates, the mapStateToProps calculation would have to take place later, in one of the React lifecycle methods that's in the purview of batchedUpdates. But moving that would require each listener to fire setState on every state change, as it is in v4, which has been shown to cause significant performance hit, such as in #398.

[gaearon]

If setState inside batchedUpdates is slow, can you help me figure out why? I wouldn't expect it to be slow in this case, since it doesn't actually do the synchronous work and instead delays actual reconciliation until batchedUpdates exits. If it is slow, maybe we can fix this in React?

[jimbolla]

I'd like to, but I have limited free time right now so I'm not sure when I can put time towards this.

[jimbolla]

So I saw there were some changes (facebook/react/pull/8204, facebook/react/pull/8207) in react master related to setState callbacks, so I decided to give this another test. The bug is fixed in react master branch right now.

[gaearon]

Wow, that's cool 😮 Let's wait for React 16 with this change then?

[gaearon]

@jimbolla Can you submit a PR to React with a test case for this?

[jimbolla]

I confirmed commit https://github.com/facebook/react/commit/5f49b63bde09567fb3d52a13e3e264dfcda5cac6 (PR https://github.com/facebook/react/pull/8204) is the commit that fixes the problem. Bug exists when the above demo is run against its parent, but doesn't when run against it.

I'll see what I can do about turning that code above into a complete test, but admittedly, this will be pushing the limits of my JS/testing abilities. Assuming I can turn it into a test, do I submit it as a PR to react master?

I still need to test react-redux@next against react master to confirm the fix. I'm probably like 85% sure that it will work though.

@gaearon Do you have any insight into when React v16 might launch? Are we sure 8204 will be a part of that and not a 15.x minor release?

Is our plan to not release react-redux v5 to stable until we can bump our React dependency to whatever version includes the fix? If so, do we want to offer an interim solution for the users that want to use the beta? Something like:

connect(..., ..., ..., { temporaryReact15textInputCursorFix: true })

This would be a temporary feature that bypasses the subscription ordering.

[gaearon]

I'll see what I can do about turning that code above into a complete test, but admittedly, this will be pushing the limits of my JS/testing abilities. Assuming I can turn it into a test, do I submit it as a PR to react master?

Yes.

Do you have any insight into when React v16 might launch?

Likely within a few months but hard to say for sure.

Are we sure 8204 will be a part of that and not a 15.x minor release?

It seems dangerous enough that it warrants a major release.

Is our plan to not release react-redux v5 to stable until we can bump our React dependency to whatever version includes the fix?

Yes, we can't release a stable version of React Redux that is incompatible with the stable version of React.

If so, do we want to offer an interim solution for the users that want to use the beta?

If you feel strongly about it, sure.

[timdorr]

Yes, we can't release a stable version of React Redux that is incompatible with the stable version of React.

We'll do it within a set of 5.0 and 5.1 releases. 5.0 goes with React 15 and has this extra option for those that need it; 5.1 goes with React 16 and doesn't need the option (basically, it switches default to enabled). Hence the 5.1 milestone on this issue :)

@jimbolla Can you prep a PR to add the option to reorder the subscriptions? Maybe call it reorderSubscriptions? It doesn't need to be lengthy and scary. It would be ignored in 5.1.

[jimbolla]

If we did a two phase. with the second one bumping the React dependency version, that'd be a breaking change and would require bumping up to 6.

I verified that react master branch does fix the issue in react-redux@next. I'm having trouble turning the repro code above into an automated test; react doesn't seem to respond to the keypress events i'm firing from js.

I'll be submitting a PR shortly that adds react15CompatibilityMode option. There's the global setting set via <Provider react15CompatibilityMode={true|false}>, which defaults to true. Whatever setting is set at the Provider level can be overridden at the component level either as a prop passed to the component or as an options argument to connect. This way we can release 5.0 without waiting for React 16, and anyone that wants to opt-in to the changes now can do so with minimal effort.

[timdorr]

The React dep is a peer dep and 15 was added in in a patch (4.4.1). We can expand to support React 16 when it's out at any time and stay within our lane on semver. React 15 and below would be buggy for this particular use case, but they still work. And besides, it's not our bug to fix.

All we would be doing in a minor release is changing the default from true to false. Even then, I believe that would count as a patch release (as nothing "breaks" as far as the API is concerned). The minor bump would be a convenience thing for users so they don't have to fight with the version string in their package.json too much.

[sophiebits]

@jimbolla If you're able to make a simple test case depending only on React that exhibits the issue in a browser but you're struggling with jsdom, that would be very helpful and I can look at turning it into an automated test.

[jimbolla]

@spicyj https://github.com/reactjs/react-redux/issues/525#issuecomment-256181787 is basically that. I just used create-react-app and then wrote that in App.js

[sophiebits]

Oops. I missed that. I'll take a look.

[gaearon]

@jimbolla With the compat option on, does the perf regress over 4.x? What is the main benefit of 5.x for the people while it's enabled?