From 84b8bbdfc9d47c82f3facc4e37d58463b5b54078 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 8 Nov 2016 17:42:53 -0800 Subject: [PATCH] Fixed batching reentrant controlled events (#8240) 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 ab6ebb9e4c..385e8aa904 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -253,6 +253,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 74dc16ddd1..44177f69ae 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 f84e2545bb..2bab859aa8 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 d61b5cca2b..5c86101266 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 6461ae973d..9fdab98daa 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, };