From d454d866b0484eda69cbfcba11ba0b7a57f16620 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 8 Nov 2016 15:21:11 -0800 Subject: [PATCH] Fixed batching reentrant controlled events If a controlled target fires within another controlled event, we should not restore it until we've fully completed the event. Otherwise, if someone reads from it afterwards, they'll get the restored value. Not super happy with this particular solution. --- scripts/fiber/tests-failing.txt | 1 + .../dom/shared/ReactEventListener.js | 14 ++-- .../shared/eventPlugins/ChangeEventPlugin.js | 9 +-- .../wrappers/__tests__/ReactDOMInput-test.js | 66 +++++++++++++++++++ .../shared/event/ReactGenericBatching.js | 26 ++++++++ 5 files changed, 103 insertions(+), 13 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 7955d43a1417a..bdb71eeb05d73 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -250,6 +250,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMIframe-test.js src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js * should properly control a value even if no event listener exists +* should control a value in reentrant events * should display `defaultValue` of number 0 * should display "true" for `defaultValue` of `true` * should display "false" for `defaultValue` of `false` diff --git a/src/renderers/dom/shared/ReactEventListener.js b/src/renderers/dom/shared/ReactEventListener.js index 74dc16ddd13a0..44177f69ae928 100644 --- a/src/renderers/dom/shared/ReactEventListener.js +++ b/src/renderers/dom/shared/ReactEventListener.js @@ -14,7 +14,6 @@ var EventListener = require('EventListener'); var ExecutionEnvironment = require('ExecutionEnvironment'); var PooledClass = require('PooledClass'); -var ReactControlledComponent = require('ReactControlledComponent'); var ReactDOMComponentTree = require('ReactDOMComponentTree'); var ReactGenericBatching = require('ReactGenericBatching'); @@ -172,14 +171,11 @@ var ReactEventListener = { try { // Event queue being processed in the same cycle allows // `preventDefault`. - ReactGenericBatching.batchedUpdates(handleTopLevelImpl, bookKeeping); - if (bookKeeping.targetInst) { - // Here we wait until all updates have propagated, which is important - // when using controlled components within layers: - // https://github.com/facebook/react/issues/1698 - // Then we restore state of any controlled component. - ReactControlledComponent.restoreStateIfNeeded(bookKeeping.targetInst); - } + ReactGenericBatching.batchedUpdatesWithControlledTarget( + handleTopLevelImpl, + bookKeeping, + bookKeeping.targetInst + ); } finally { TopLevelCallbackBookKeeping.release(bookKeeping); } diff --git a/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js b/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js index f84e2545bb44c..2bab859aa8b3a 100644 --- a/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js +++ b/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js @@ -101,10 +101,11 @@ function manualDispatchChangeEvent(nativeEvent) { // components don't work properly in conjunction with event bubbling because // the component is rerendered and the value reverted before all the event // handlers can run. See https://github.com/facebook/react/issues/708. - ReactGenericBatching.batchedUpdates(runEventInBatch, event); - if (activeElementInst) { - ReactControlledComponent.restoreStateIfNeeded(activeElementInst); - } + ReactGenericBatching.batchedUpdatesWithControlledTarget( + runEventInBatch, + event, + activeElementInst + ); } function runEventInBatch(event) { diff --git a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js index d61b5cca2be0d..5c8610126657a 100644 --- a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js @@ -66,6 +66,72 @@ describe('ReactDOMInput', () => { document.body.removeChild(container); }); + it('should control a value in reentrant events', () => { + // This must use the native event dispatching. If we simulate, we will + // bypass the lazy event attachment system so we won't actually test this. + var inputEvent = document.createEvent('Event'); + inputEvent.initEvent('input', true, true); + // This must use the native event dispatching. If we simulate, we will + // bypass the lazy event attachment system so we won't actually test this. + var changeEvent = document.createEvent('Event'); + changeEvent.initEvent('change', true, true); + + class ControlledInputs extends React.Component { + state = { value: 'lion' }; + a = null; + b = null; + switchedFocus = false; + change(newValue) { + this.setState({ value: newValue }); + // Calling focus here will blur the text box which causes a native + // change event. Ideally we shouldn't have to fire this ourselves. + // I don't know how to simulate a change event on a text box. + this.a.dispatchEvent(changeEvent); + this.b.focus(); + } + blur(currentValue) { + this.switchedFocus = true; + // currentValue should be 'giraffe' here because we should not have + // restored it on the target yet. + this.setState({ value: currentValue }); + } + render() { + return ( +
+ this.a = n} + value={this.state.value} + onChange={e => this.change(e.target.value)} + onBlur={e => this.blur(e.target.value)} + /> + this.b = n} + /> +
+ ); + } + } + + var container = document.createElement('div'); + var instance = ReactDOM.render(, container); + + // We need it to be in the body to test native event dispatching. + document.body.appendChild(container); + + instance.a.focus(); + // Simulate a native keyup event + setUntrackedValue(instance.a, 'giraffe'); + + instance.a.dispatchEvent(inputEvent); + + expect(instance.a.value).toBe('giraffe'); + expect(instance.switchedFocus).toBe(true); + + document.body.removeChild(container); + }); + it('should display `defaultValue` of number 0', () => { var stub = ; stub = ReactTestUtils.renderIntoDocument(stub); diff --git a/src/renderers/shared/shared/event/ReactGenericBatching.js b/src/renderers/shared/shared/event/ReactGenericBatching.js index 6461ae973dbb3..9fdab98daa497 100644 --- a/src/renderers/shared/shared/event/ReactGenericBatching.js +++ b/src/renderers/shared/shared/event/ReactGenericBatching.js @@ -11,6 +11,8 @@ 'use strict'; +var ReactControlledComponent = require('ReactControlledComponent'); + // Used as a way to call batchedUpdates when we don't know if we're in a Fiber // or Stack context. Such as when we're dispatching events or if third party // libraries need to call batchedUpdates. Eventually, this API will go away when @@ -36,6 +38,29 @@ function batchedUpdates(fn, bookkeeping) { stackBatchedUpdates(performFiberBatchedUpdates, fn, bookkeeping); } +var isBatching = false; +function batchedUpdatesWithControlledTarget(fn, bookkeeping, target) { + if (isBatching) { + // TODO: If this target is not the same as the one currently batched, + // we'll drop it. + batchedUpdates(fn, bookkeeping); + return; + } + isBatching = true; + try { + batchedUpdates(fn, bookkeeping); + } finally { + isBatching = false; + } + if (target) { + // Here we wait until all updates have propagated, which is important + // when using controlled components within layers: + // https://github.com/facebook/react/issues/1698 + // Then we restore state of any controlled component. + ReactControlledComponent.restoreStateIfNeeded(target); + } +} + var ReactGenericBatchingInjection = { injectStackBatchedUpdates: function(_batchedUpdates) { stackBatchedUpdates = _batchedUpdates; @@ -47,6 +72,7 @@ var ReactGenericBatchingInjection = { var ReactGenericBatching = { batchedUpdates, + batchedUpdatesWithControlledTarget, injection: ReactGenericBatchingInjection, };