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.
Sebastian Markbåge committed
Nov 8, 2016 at 17:42 UTC
84b8bbdfc9d47c82f3facc4e37d58463b5b54078
5 files changed
+103
-13
scripts/fiber/tests-failing.txt
+1
@@ -253,6 +253,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMIframe-test.js
253
254
src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
255
* should properly control a value even if no event listener exists
256
+* should control a value in reentrant events
257
* should display `defaultValue` of number 0
258
* should display "true" for `defaultValue` of `true`
259
* should display "false" for `defaultValue` of `false`
src/renderers/dom/shared/ReactEventListener.js
+5
-9
@@ -14,7 +14,6 @@
14
var EventListener = require('EventListener');
15
var ExecutionEnvironment = require('ExecutionEnvironment');
16
var PooledClass = require('PooledClass');
17
-var ReactControlledComponent = require('ReactControlledComponent');
17
var ReactDOMComponentTree = require('ReactDOMComponentTree');
18
var ReactGenericBatching = require('ReactGenericBatching');
19
@@ -172,14 +171,11 @@ var ReactEventListener = {
171
try {
172
// Event queue being processed in the same cycle allows
173
// `preventDefault`.
175
- ReactGenericBatching.batchedUpdates(handleTopLevelImpl, bookKeeping);
176
- if (bookKeeping.targetInst) {
177
- // Here we wait until all updates have propagated, which is important
178
- // when using controlled components within layers:
179
- // https://github.com/facebook/react/issues/1698
180
- // Then we restore state of any controlled component.
181
- ReactControlledComponent.restoreStateIfNeeded(bookKeeping.targetInst);
182
- }
174
+ ReactGenericBatching.batchedUpdatesWithControlledTarget(
175
+ handleTopLevelImpl,
176
+ bookKeeping,
177
+ bookKeeping.targetInst
178
+ );
179
} finally {
180
TopLevelCallbackBookKeeping.release(bookKeeping);
181
}
src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js
+5
-4
@@ -101,10 +101,11 @@ function manualDispatchChangeEvent(nativeEvent) {
101
// components don't work properly in conjunction with event bubbling because
102
// the component is rerendered and the value reverted before all the event
103
// handlers can run. See https://github.com/facebook/react/issues/708.
104
- ReactGenericBatching.batchedUpdates(runEventInBatch, event);
105
- if (activeElementInst) {
106
- ReactControlledComponent.restoreStateIfNeeded(activeElementInst);
107
- }
104
+ ReactGenericBatching.batchedUpdatesWithControlledTarget(
105
+ runEventInBatch,
106
+ event,
107
+ activeElementInst
108
+ );
109
}
110
111
function runEventInBatch(event) {
src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
+66
@@ -66,6 +66,72 @@ describe('ReactDOMInput', () => {
66
document.body.removeChild(container);
67
});
68
69
+ it('should control a value in reentrant events', () => {
70
+ // This must use the native event dispatching. If we simulate, we will
71
+ // bypass the lazy event attachment system so we won't actually test this.
72
+ var inputEvent = document.createEvent('Event');
73
+ inputEvent.initEvent('input', true, true);
74
+ // This must use the native event dispatching. If we simulate, we will
75
+ // bypass the lazy event attachment system so we won't actually test this.
76
+ var changeEvent = document.createEvent('Event');
77
+ changeEvent.initEvent('change', true, true);
78
+
79
+ class ControlledInputs extends React.Component {
80
+ state = { value: 'lion' };
81
+ a = null;
82
+ b = null;
83
+ switchedFocus = false;
84
+ change(newValue) {
85
+ this.setState({ value: newValue });
86
+ // Calling focus here will blur the text box which causes a native
87
+ // change event. Ideally we shouldn't have to fire this ourselves.
88
+ // I don't know how to simulate a change event on a text box.
89
+ this.a.dispatchEvent(changeEvent);
90
+ this.b.focus();
91
+ }
92
+ blur(currentValue) {
93
+ this.switchedFocus = true;
94
+ // currentValue should be 'giraffe' here because we should not have
95
+ // restored it on the target yet.
96
+ this.setState({ value: currentValue });
97
+ }
98
+ render() {
99
+ return (
100
+ <div>
101
+ <input
102
+ type="text"
103
+ ref={n => this.a = n}
104
+ value={this.state.value}
105
+ onChange={e => this.change(e.target.value)}
106
+ onBlur={e => this.blur(e.target.value)}
107
+ />
108
+ <input
109
+ type="text"
110
+ ref={n => this.b = n}
111
+ />
112
+ </div>
113
+ );
114
+ }
115
+ }
116
+
117
+ var container = document.createElement('div');
118
+ var instance = ReactDOM.render(<ControlledInputs />, container);
119
+
120
+ // We need it to be in the body to test native event dispatching.
121
+ document.body.appendChild(container);
122
+
123
+ instance.a.focus();
124
+ // Simulate a native keyup event
125
+ setUntrackedValue(instance.a, 'giraffe');
126
+
127
+ instance.a.dispatchEvent(inputEvent);
128
+
129
+ expect(instance.a.value).toBe('giraffe');
130
+ expect(instance.switchedFocus).toBe(true);
131
+
132
+ document.body.removeChild(container);
133
+ });
134
+
135
it('should display `defaultValue` of number 0', () => {
136
var stub = <input type="text" defaultValue={0} />;
137
stub = ReactTestUtils.renderIntoDocument(stub);
src/renderers/shared/shared/event/ReactGenericBatching.js
+26
@@ -11,6 +11,8 @@
11
12
'use strict';
13
14
+var ReactControlledComponent = require('ReactControlledComponent');
15
+
16
// Used as a way to call batchedUpdates when we don't know if we're in a Fiber
17
// or Stack context. Such as when we're dispatching events or if third party
18
// libraries need to call batchedUpdates. Eventually, this API will go away when
@@ -36,6 +38,29 @@ function batchedUpdates(fn, bookkeeping) {
38
stackBatchedUpdates(performFiberBatchedUpdates, fn, bookkeeping);
39
}
40
41
+var isBatching = false;
42
+function batchedUpdatesWithControlledTarget(fn, bookkeeping, target) {
43
+ if (isBatching) {
44
+ // TODO: If this target is not the same as the one currently batched,
45
+ // we'll drop it.
46
+ batchedUpdates(fn, bookkeeping);
47
+ return;
48
+ }
49
+ isBatching = true;
50
+ try {
51
+ batchedUpdates(fn, bookkeeping);
52
+ } finally {
53
+ isBatching = false;
54
+ }
55
+ if (target) {
56
+ // Here we wait until all updates have propagated, which is important
57
+ // when using controlled components within layers:
58
+ // https://github.com/facebook/react/issues/1698
59
+ // Then we restore state of any controlled component.
60
+ ReactControlledComponent.restoreStateIfNeeded(target);
61
+ }
62
+}
63
+
64
var ReactGenericBatchingInjection = {
65
injectStackBatchedUpdates: function(_batchedUpdates) {
66
stackBatchedUpdates = _batchedUpdates;
@@ -47,6 +72,7 @@ var ReactGenericBatchingInjection = {
72
73
var ReactGenericBatching = {
74
batchedUpdates,
75
+ batchedUpdatesWithControlledTarget,
76
injection: ReactGenericBatchingInjection,
77
};
78