Inputs lose focus on parent re-render

[chrisui]

I'm currently experiencing issues (using 0.3.0-rc2) where if you are focused in a generated input and the parent state is changed (forcing an update) then the input will be blurred.

A brief bit of investigation (just watching DOM changes) showed me that the entire form tree is removed and replaced with an identical tree when the parent component is re-rendered.

Is this a known issue within this release?

Reproducing should be quite easy: just a simple form with a textbox and force an update on your parent component to see that you'll lose focus of your input if you were typing.

[srconklin]

I think I am witnessing this issue as well

[gcanti]

Hi, a quick workaround should be to add:

shouldComponentUpdate: function () {
  return false;
}

to the component containing the form, or to wrap the form with a component with shouldComponentUpdate defined.
Let me know if it works for you.

But I wonder if it would be a good thing to add shouldComponentUpdate to the top level component, here:

https://github.com/gcanti/tcomb-form/blob/master/lib/create.js#L25

That would be an easy fix, what do you think?

[chrisui]

Cool, so putting shouldComponentUpdate() => false in will fix the focus issue but then you lose the power of being able to use the form as a "controlled input". Ie. you can no longer pass new value prop in which updates values in the form from outside state.

Also, somewhat related, it seems when you create a form in the render method it will remount the entire new form everytime. Perhaps some internal React evaluation seeing a newly created class everytime? The reason I am attempting this pattern is because I have variable enums so just creating the model & form within render with current state makes most sense following the React pattern. You may have a better way of updating models though which I have probably missed!

[srconklin]

well, for my particular use case, i have a Login component that does that already..it just wraps a simple form with 2 fields on a big front login screen. I have a customized way of showing an error message when authentication fails from the server. If i add shouldComponentUpdate and return false.. that message never gets rendered.
see here
http://www.evernote.com/shard/s349/sh/552b8bb9-e574-436c-a3a5-1e52160622d8/01e55e911aea42d25e1f96314c9791c1

Note: i am not using the custom error as provided by tcomb-forms
http://gcanti.github.io/tcomb-form/guide/index.html#fieldset-error

because i noticed that when i repeatedly hit the sign in button with an intentionalyl incorrect username and password,, there was a noticeable "flashing" of the content as what appeared to be the entire form re-rendering itself.. it was noticeable enough that i went with the custom layout seen above, which was much smoother... ,This also brings me to the reason that i am so interested in autofocus.

I have an onChange handler on my form that removes the custom error div (when present) when the user attempts to enter the email again..this message disappears nicely but the whole form seems to be re-rendering as well and as soon as you type one character the focus is lost and you must re-enter the email field again and start typing fall over again..it is kind of quirky...

But, maybe i should just drop all this and go back to using the tcombs-form way of doing form level error messages? or do you have any other thoughts on this approach.

[chrisui]

Looked a little more into the React "Reconciliation" process and tried out some tests to confirm.

When a new React Component is discovered, regardless of whether the rendered output is identical to the existing DOM, if the class is a newly created one it will discard it and replace it with a new identical (only in appearance and structure) tree Ie. component will mount everytime parent is updated.

So the problem is how the render methods for forms use factory methods to create new classes rather than using a set of static classes and placing logic within them and passing data down through props for appropiate nesting situations.

I'm going to try and throw together a quick proof of concept around tcomb-form but it seems there will be a fair bit of refactoring required.

Quick test you can run which should simulate what's going on:

var MyTestComponent = React.createClass({
    componentDidMount: function {
        // Force some updating to simulate state changing and re-rendering
        setInterval(this.forceUpdate.bind(this), 500);
    },
    render: function() {
        var MySubComponent = React.createClass({
            componentDidMount: function() {
                console.log('Mounted sub component!');
            },
            render: function() {
                return (<input type="text" />);
            }
        });

        return (<div><MySubComponent /></div>);
    }
});
React.render(<MyTestComponent />, document.body);

[gcanti]

Wow it's complicated. When I wrote tcomb-form the main goal was to build tons of forms for internal backoffices (so static forms). You have to handle pretty dynamic forms. It's an interesting problem, I'll try to help as much as I can.

I wrote this experiment, let me know if something is missing:

var Person = t.struct({
  name: t.Str,
  tags: t.list(t.Str)
});

var Form = t.form.create(Person, {
  fields: {
    name: {
      // auto focus on first rendering
      autoFocus: true
    }
  }
});

var Wrapper = React.createClass({
  shouldComponentUpdate: function () {
    // this prevents to lose focus
    return false;
  },
  onChange: function (value) {
    // this keeps the state in sync with the form
    this.setState(value);
  },
  change: function (state) {
    // this allows to change the form value from the outside
    this.setState(state);
    this.forceUpdate();
  },
  render: function () {
    console.log('wrapper rendered', this.state);
    return <Form ref="form" value={this.state} onChange={this.onChange} />
  }
});

var App = React.createClass({

  componentDidMount: function () {
    // force re-rendering
    setInterval(this.forceUpdate.bind(this), 1000);
  },

  changeValue: function () {
    this.refs.wrapper.change({name: 'changed'});
  },

  render: function() {
    console.log('app rendered');
    return (
      <div>
        <Wrapper ref="wrapper" />
        <button onClick={this.changeValue}>force change</button>
      </div>
    );
  }
});

React.render(<App />, document.body);

[chrisui]

Cheers for the help! The tcomb libraries are all great and if this issue can be resolved it really will be a perfect solution for easily creating drop in forms for react.

I don't think the above code would solve the issue for two reasons:

1. When your outside state changes and we call change() an update is forced which will blur the input we had focused. Imagine you had your username field focused as you were typing and then we had to call change() to set some options from an async operation which was happening in the background.
2. With React your data is usually stateful so usually you just know "something has changed, re-render this block and let the render diff handle what has really changed" rather than creating explicit data bindings which is what React quite importantly helps you avoid.

To get things working in a way which would handle the everyday react/stateful application I'd see there being a single root <Form /> component which would take a model and the usual options object you'd usually pass to t.form.create(opts)

So the public api would look more like:

React.createClass({
  render() {
    let options = {
        fields: {username: {}}
        ...
    };

    return (
        <Form model={UserModel} opts={options} />
    );
  }
});

This root component would hold the state of the entire form (I think like it currently is) so if any part of your models or options change they are still stored and passed down.

Internally you'd have static components for each type which knew how to render itself with passed props. Then we use the normal react method of rendering these rather than having a factory create new classes every re-render which cause the DOM destruction.

At a glance I think this essentially involves moving all the logic of handling passed 'context' and 'options' from factory functions into the react classes (render methods???) themselves.

var Components = {};

function getComponent(model) {
    return Components[utils.getType(model)];
}

function generateContext(model, options) {
    return {}; // Some object which describes to a component how to render itself
};

var Form = React.createClass({
    render() {
        var Component = getComponent(this.props.model);
        var context = generateContext(this.props.model, this.props.opts);

        return (<Component context={context} />);
    }
});

Components.Struct = React.createClass({
    render() {
        var props = this.props.contexts.props;
        var renderFields = [];

        for (var propName in props) {
            var Component = getComponent(props[propName]);
            var context = generateContext(props[propName], this.props.context.opts);

            renderFields.push(<Component context={context} key={propName} />);
        }

        return (
            <div>{renderFields}</div>
        );
    }
});

Components.Textbox = React.createClass({
    render() {
        var Template = getTemplate(this.props.context.model, this.props.context.opts);
        var templateProps = {
            placeholder: ...
        };

        return (<Template {...templateProps} />);
    }
});

...

Hopefully this isn't too psuedo and sparse to be confusing! :P But basically propagating 'context' the library does now but not factory creating new classes on the fly (giving render the power to decide what to do dependant on 'current state') would solve all the issues and allow these forms to be used just like any other react component.

[gcanti]

I tried to build v0.3 as you showed but I failed (I don't remember why though). I must find a few hours, sit down and reconsider the whole thing (maybe tomorrow or in the week end). Thanks for your insights, may I share my thoughts with you when I'll have something concrete?

[chrisui]

Sounds good.

I'm going to try out a rudimentary version tomorrow for work. While it won't be as flexible for different templates etc. it should hopefully prove the concept!

Thanks for everything so far though!

[gcanti]

@chrisui Some thoughts:

1. (leaf) components should be stateful: they must store the current value in their state in order to validate it when someone calls their getValue() method. Why? Because in general only the component knows how to validate a value and how to display an error. Besides, when you consider deeply nested structures, it would be hard for a top level component to handle the state / errors of children and children of children, etc...
2. because of 1. the components won't react to a value change from outside (via props change). The only solution I found is to implement componentWillReceiveProps

Experimental branch (v0.4):

https://github.com/gcanti/tcomb-form/tree/v0.4

It contains a simple prototype (only Form, Struct, List, Textbox components, not skinnable, no context, etc..)

This is a setup example:

var React = require('react');
var t = require('../.');
var debug = require('debug');
debug.enable('*');

var type = t.struct({
  name: t.Str
});
var options = {};
var value = {name: 'a'};

var Form = t.form.Form;

var App = React.createClass({

  componentDidMount: function () {
    // force re-rendering
    //setInterval(this.force, 1000);
  },

  force: function () {
  },

  getInitialState: function () {
    return {value: value};
  },

  onChange: function (value) {

  },

  getValue: function () {
    console.log(this.refs.form.getValue());
  },

  render: function() {
    return (
      <div>
        <Form ref="form" type={type} options={options} onChange={this.onChange} value={this.state.value} />
        <button className="btn btn-primary" onClick={this.getValue}>Ok</button>
        <button className="btn btn-primary" onClick={this.force}>Force update</button>
      </div>
    );
  }
});

React.render(<App />, document.body);

May I ask you to do some test with your use cases? Just to see if it's the right direction

[chrisui]

I'll give it a go shortly! thanks!

Regarding the state I agree that each leaf should be responsible for it's own validation and storing local value. However I think that it should be allowed to work as a controlled input too.

So if you pass a value prop to the root Form component each leaf should take it's value from that (null if not present). This allows the state to be stored and updated easily from outside. Default initial values can be set with the defaultValue prop. If you omit the value prop you can use the component normally as you describe where state is local to each leaf.

[gcanti]

So if you pass a value prop to the root Form component each leaf should take it's value from that (null if not present). This allows the state to be stored and updated easily from outside

The experimental branch should allow that behaviour. The key point is this check:

https://github.com/gcanti/tcomb-form/blob/v0.4/lib/components/Textbox.js#L27

In my tests I was able to update the value of a textbox from outside without losing focus.

The exception is the List component (now I remember why I failed...lists are very, very tricky :)

https://github.com/gcanti/tcomb-form/blob/v0.4/lib/components/List.js#L28

[chrisui]

Great seems to be working as expected using v0.4 branch!

What is the issue with lists? Might be able to help!

[gcanti]

The main problem with lists is here (uploaded a new version):

https://github.com/gcanti/tcomb-form/blob/v0.4/lib/components/List.js#L107

If I use the counter i, React will try to reutilize the DOM elements when I swap two elements
(Up, Down button) resulting in weird behaviors

Example (with the previous setup):

var Person = t.struct({
  name: t.Str,
  surname: t.Str
});

var type = t.list(Person);
var value = null;
var options = null;

On the other hand if I use uuids (keys array) helping React to do the right thing I lose the possibility of having a controlled component since values and keys go out of sync if componentWillReceiveProps is called.

I'm working hard on this, hope to find a solution soon.

[gcanti]

@chrisui I got it! Now there is a (painful) refactoring (code, tests and docs..) but it should be relatively
straightforward. I also spotted some possible optimizations.

Unfortunately there will be some breaking changes in the APIs.

Thanks so much for your help. Stay tuned!

[gcanti]

@srconklin in the new version the re-renderings will be drastically reduced. I think it will solve your flickering issues

[srconklin]

great. I have been following the thread and can't wait for the new version.

[chrisui]

Sounds great! I'll test it all out with my use cases when you think it's ready for it. :)

Sorry I'm only just replying! Needed to switch off for the weekend ;)

[gcanti]

I'm writing unit tests right now: it ended up with a complete rewrite...

I'll test it all out with my use cases when you think it's ready for it.

Great! Thx

[gcanti]

@chrisui @srconklin

Ok, beta version uploaded to v0.4 branch. There are 261 tests at the moment, if you agree on the implementation and there are no problems I'll proceed to add more tests and final documentation / README.

Important note

Now form renderings are optimized, i.e. I implemented an aggressive shouldComponentUpdate.

https://github.com/gcanti/tcomb-form/blob/v0.4/lib/components/shouldComponentUpdate.js

In order to force an actual re-rendering you must pass in a different reference (=== is used) for at least one of:

• type
• options
• onChange
• value

Here's a bare setup to play with:

var React = require('react');
var t = require('.'); // <- here require the top level index.js file
var debug = require('debug');

debug.enable('*'); // shows when re-redenderings happen in the console

// showcase with all the relevant types
var Country = t.enums({
  IT: 'Italy',
  US: 'United States'
});

var Gender = t.enums({
  M: 'Male',
  F: 'Female'
});

var Person = t.struct({
  name: t.Str,
  surname: t.maybe(t.Str),
  gender: t.maybe(Gender),
  country: t.maybe(Country),
  rememberMe: t.Bool,
  tags: t.list(t.Str)
});

var type = Person;
var value = null;
var options = {
  fields: {
    gender: {
      factory: t.form.radio // this API fortunately is not changed
    }
  }
};

/*
  public API changed! t.form.create is gone. Now use t.form.Form

  <Form
    type="" required
    options="" optional
    value="" optional
    onChange="" optional

*/
var Form = t.form.Form;

// example of "uncontrolled" form (i.e. you can't change the form contents, focus is not lost)
var App = React.createClass({
  getValue: function () {
    var value = this.refs.form.getValue();
    if (value) {
      console.log(value);
    }
  },
  render: function() {
    return (
      <div>
        <Form ref="form" type={type} options={options} value={value} />
        <div className="form-group">
          <button className="btn btn-primary" onClick={this.getValue}>getValue</button>
        </div>
      </div>
    );
  }
});

// example of "controlled" form
// (i.e. you can change the form contents changin one of type, options, onChange, value and force a re-rendering)
App = React.createClass({
  getInitialState: function () {
    return {value: value, options: options};
  },
  componentDidMount: function () {
    setInterval(this.force, 1000);
  },
  force: function () {
    console.log('forcing update...');
    this.forceUpdate();
  },
  onChange: function (value) {
    this.setState({value: value});
  },
  getValue: function () {
    var value = this.refs.form.getValue();
    if (value) {
      console.log(value);
    }
  },
  render: function() {
    return (
      <div>
        <Form ref="form" type={type} options={this.state.options} onChange={this.onChange} value={this.state.value} />
        <div className="form-group">
          <button className="btn btn-primary" onClick={this.getValue}>getValue</button>
        </div>
      </div>
    );
  }
});

Hope it's all clear, let me know if you need more explanations.

[chrisui]

Seems to be working perfectly for my use cases so far! I guess it's a good thing I have nothing more really to add ;)

Good work and thanks so much for spending the time on this. I'll be using it pretty intensively over the coming weeks so I'll let you know if I find any further issues...

Thanks again!

[gcanti]

Awesome. I'll publish v0.4 on master, write more tests, update the docs, etc.. in the next few days.

Thanks again!

Thanks to you for pushing me towards the right direction.

Oh.. and before the next official release I expect from you a Pull Request regarding the "Contributions" section of the new README.md file ;-)

[gcanti]

0.4.0-beta landed on master. All tests green with React 0.13.0-beta.1 :)

[gcanti]

@chrisui it's amazing what you can do now, try this one with the current version on master!

// change the form configuration on the fly based on form values

var Person = t.struct({
  name: t.Str,
  surname: t.Str
});

var Person2 = t.struct({
  name: t.Str,
  surname: t.Str,
  age: t.Num
});

var App = React.createClass({
  getInitialState: function () {
    return {
      type: Person,
      options: {
        fields: {
          name: {
            help: 'Type "a" to change the form configuration on the fly'
          }
        }
      },
      value: {}
    };
  },
  getValue: function () {
    var value = this.refs.form.getValue();
    if (value) {
      console.log(value);
    }
  },
  onChange: function (value) {
    // changes the form configuration on the fly!
    if (value.name === 'a') {
      // you can change even the type
      this.state.type = Person2;
      // change options
      this.state.options.fields.surname = {disabled: true};
    }
    this.state.value = value;
    this.forceUpdate();
  },
  render: function() {
    return (
      <div>
        <Form ref="form"
          type={this.state.type}
          options={this.state.options}
          value={this.state.value}
          onChange={this.onChange} />
        <button className="btn btn-primary" onClick={this.getValue}>getValue</button>
      </div>
    );
  }
});

React.render(<App />, document.body);

[chrisui]

Yup it's basically exactly the type of thing I was trying to do! The power of stateful UI's!

After a week of developing everything seems to be working great!

[gcanti]

Great. I'm going to release v0.4 soon. May I add you in the Contributions section?

[chrisui]

Sounds great :)

[gcanti]

ok, feel free to change what I wrote in the README.md file