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.

Author: sebmarkbage

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`

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);
     }

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) {

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 (
+          <div>
+            <input
+              type="text"
+              ref={n => this.a = n}
+              value={this.state.value}
+              onChange={e => this.change(e.target.value)}
+              onBlur={e => this.blur(e.target.value)}
+            />
+            <input
+              type="text"
+              ref={n => this.b = n}
+            />
+          </div>
+        );
+      }
+    }
+
+    var container = document.createElement('div');
+    var instance = ReactDOM.render(<ControlledInputs />, 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 = <input type="text" defaultValue={0} />;
     stub = ReactTestUtils.renderIntoDocument(stub);

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,
 };