From 354fb4429978a8d77df72781ce1754c2ffcce9b3 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 28 Jun 2014 16:45:43 -0700 Subject: [PATCH] Use setImmediate to defer value restoration Depends on #1758. Fixes #1698. Previously, controlled components would update too soon when using something like ReactLayeredComponentMixin (i.e., before the layer's updates could propagate from the parent), causing the cursor to jump even when always updating the new model value to match the DOM state. With this change, we defer the update until after all nested updates have had a chance to finish, which prevents the cursor from misbehaving. Also cleaned up the logic around updating a bit -- the .value and .checked updates in ReactDOMInput weren't being relied on at all so I removed them and opted for a simple forceUpdate instead. I also got rid of _isChanging which hasn't been necessary since the introduction of update batching. Test Plan: Tested the example in http://jsfiddle.net/Bobris/ZZtXn/2/ and verified that the cursor didn't jump. Changed the code to filter out numbers and verified that the field prevents typing numbers (attempting to do so still causes the cursor to jump to the end). Also verified that controlled and uncontrolled radio buttons, textareas, and select boxes work. --- .../ui/dom/components/ReactDOMInput.js | 42 +++++++++---------- .../ui/dom/components/ReactDOMSelect.js | 23 ++++++---- .../ui/dom/components/ReactDOMTextarea.js | 17 ++++---- 3 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/browser/ui/dom/components/ReactDOMInput.js b/src/browser/ui/dom/components/ReactDOMInput.js index 0437f60513..1fd4351251 100644 --- a/src/browser/ui/dom/components/ReactDOMInput.js +++ b/src/browser/ui/dom/components/ReactDOMInput.js @@ -25,6 +25,7 @@ var ReactBrowserComponentMixin = require('ReactBrowserComponentMixin'); var ReactCompositeComponent = require('ReactCompositeComponent'); var ReactDOM = require('ReactDOM'); var ReactMount = require('ReactMount'); +var ReactUpdates = require('ReactUpdates'); var invariant = require('invariant'); var merge = require('merge'); @@ -34,6 +35,13 @@ var input = ReactDOM.input; var instancesByReactID = {}; +function forceUpdateIfMounted() { + /*jshint validthis:true */ + if (this.isMounted()) { + this.forceUpdate(); + } +} + /** * Implements an native component that allows setting these optional * props: `checked`, `value`, `defaultChecked`, and `defaultValue`. @@ -58,16 +66,11 @@ var ReactDOMInput = ReactCompositeComponent.createClass({ getInitialState: function() { var defaultValue = this.props.defaultValue; return { - checked: this.props.defaultChecked || false, - value: defaultValue != null ? defaultValue : null + initialChecked: this.props.defaultChecked || false, + initialValue: defaultValue != null ? defaultValue : null }; }, - shouldComponentUpdate: function() { - // Defer any updates to this component during the `onChange` handler. - return !this._isChanging; - }, - render: function() { // Clone `this.props` so we don't mutate the input. var props = merge(this.props); @@ -76,10 +79,10 @@ var ReactDOMInput = ReactCompositeComponent.createClass({ props.defaultValue = null; var value = LinkedValueUtils.getValue(this); - props.value = value != null ? value : this.state.value; + props.value = value != null ? value : this.state.initialValue; var checked = LinkedValueUtils.getChecked(this); - props.checked = checked != null ? checked : this.state.checked; + props.checked = checked != null ? checked : this.state.initialChecked; props.onChange = this._handleChange; @@ -119,14 +122,12 @@ var ReactDOMInput = ReactCompositeComponent.createClass({ var returnValue; var onChange = LinkedValueUtils.getOnChange(this); if (onChange) { - this._isChanging = true; returnValue = onChange.call(this, event); - this._isChanging = false; } - this.setState({ - checked: event.target.checked, - value: event.target.value - }); + // Here we use setImmediate to wait until all updates have propagated, which + // is important when using controlled components within layers: + // https://github.com/facebook/react/issues/1698 + ReactUpdates.setImmediate(forceUpdateIfMounted, this); var name = this.props.name; if (this.props.type === 'radio' && name != null) { @@ -164,13 +165,10 @@ var ReactDOMInput = ReactCompositeComponent.createClass({ 'ReactDOMInput: Unknown radio button ID %s.', otherID ); - // In some cases, this will actually change the `checked` state value. - // In other cases, there's no change but this forces a reconcile upon - // which componentDidUpdate will reset the DOM property to whatever it - // should be. - otherInstance.setState({ - checked: false - }); + // If this is a controlled radio button group, forcing the input that + // was previously checked to update will cause it to be come re-checked + // as appropriate. + ReactUpdates.setImmediate(forceUpdateIfMounted, otherInstance); } } diff --git a/src/browser/ui/dom/components/ReactDOMSelect.js b/src/browser/ui/dom/components/ReactDOMSelect.js index 9a3a3e0818..3b965c60ba 100644 --- a/src/browser/ui/dom/components/ReactDOMSelect.js +++ b/src/browser/ui/dom/components/ReactDOMSelect.js @@ -23,12 +23,21 @@ var LinkedValueUtils = require('LinkedValueUtils'); var ReactBrowserComponentMixin = require('ReactBrowserComponentMixin'); var ReactCompositeComponent = require('ReactCompositeComponent'); var ReactDOM = require('ReactDOM'); +var ReactUpdates = require('ReactUpdates'); var merge = require('merge'); // Store a reference to the