From 8a09a2fc538e98523f67022095de9120e5dc2c32 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 29 Jan 2018 23:49:10 -0800 Subject: [PATCH] Interactive updates (#12100) * Updates inside controlled events (onChange) are sync even in async mode This guarantees the DOM is in a consistent state before we yield back to the browser. We'll need to figure out a separate strategy for other interactive events. * Don't rely on flushing behavior of public batchedUpdates implementation Flush work as an explicit step at the end of the event, right before restoring controlled state. * Interactive updates At the beginning of an interactive browser event (events that fire as the result of a user interaction, like a click), check for pending updates that were scheduled in a previous interactive event. Flush the pending updates synchronously so that the event handlers are up-to-date before responding to the current event. We now have three classes of events: - Controlled events. Updates are always flushed synchronously. - Interactive events. Updates are async, unless another a subsequent event is fired before it can complete, as described above. They are also slightly higher priority than a normal async update. - Non-interactive events. These are treated as normal, low-priority async updates. * Flush lowest pending interactive update time Accounts for case when multiple interactive updates are scheduled at different priorities. This can happen when an interactive event is dispatched inside an async subtree, and there's an event handler on an ancestor that is outside the subtree. * Update comment about restoring controlled components --- packages/events/ReactControlledComponent.js | 4 + packages/events/ReactGenericBatching.js | 53 ++- packages/events/ReactSyntheticEventType.js | 1 + packages/react-dom/src/client/ReactDOM.js | 6 +- .../src/events/ReactDOMEventListener.js | 32 +- .../react-dom/src/events/SimpleEventPlugin.js | 92 ++-- ....js => ChangeEventPlugin-test.internal.js} | 281 +++++++++++- .../SimpleEventPlugin-test.internal.js | 412 ++++++++++++++++++ .../__tests__/SimpleEventPlugin-test.js | 196 --------- .../src/test-utils/ReactTestUtils.js | 1 + .../react-native-renderer/src/ReactFabric.js | 4 +- .../src/ReactFabricRenderer.js | 5 - .../src/ReactNativeRenderer.js | 4 +- packages/react-noop-renderer/src/ReactNoop.js | 2 + .../src/ReactFiberReconciler.js | 12 +- .../src/ReactFiberScheduler.js | 196 ++++++--- 16 files changed, 966 insertions(+), 335 deletions(-) rename packages/react-dom/src/events/__tests__/{ChangeEventPlugin-test.js => ChangeEventPlugin-test.internal.js} (61%) create mode 100644 packages/react-dom/src/events/__tests__/SimpleEventPlugin-test.internal.js delete mode 100644 packages/react-dom/src/events/__tests__/SimpleEventPlugin-test.js diff --git a/packages/events/ReactControlledComponent.js b/packages/events/ReactControlledComponent.js index c03233cc21..499fb04927 100644 --- a/packages/events/ReactControlledComponent.js +++ b/packages/events/ReactControlledComponent.js @@ -63,6 +63,10 @@ export function enqueueStateRestore(target) { } } +export function needsStateRestore(): boolean { + return restoreTarget !== null || restoreQueue !== null; +} + export function restoreStateIfNeeded() { if (!restoreTarget) { return; diff --git a/packages/events/ReactGenericBatching.js b/packages/events/ReactGenericBatching.js index 3e22cd54ce..96a9bd9a51 100644 --- a/packages/events/ReactGenericBatching.js +++ b/packages/events/ReactGenericBatching.js @@ -5,7 +5,10 @@ * LICENSE file in the root directory of this source tree. */ -import {restoreStateIfNeeded} from './ReactControlledComponent'; +import { + needsStateRestore, + restoreStateIfNeeded, +} from './ReactControlledComponent'; // Used as a way to call batchedUpdates when we don't have a reference to // the renderer. Such as when we're dispatching events or if third party @@ -14,35 +17,53 @@ import {restoreStateIfNeeded} from './ReactControlledComponent'; // scheduled work and instead do synchronous work. // Defaults -let fiberBatchedUpdates = function(fn, bookkeeping) { +let _batchedUpdates = function(fn, bookkeeping) { return fn(bookkeeping); }; +let _interactiveUpdates = function(fn, a, b) { + return fn(a, b); +}; +let _flushInteractiveUpdates = function() {}; -let isNestingBatched = false; +let isBatching = false; export function batchedUpdates(fn, bookkeeping) { - if (isNestingBatched) { + if (isBatching) { // If we are currently inside another batch, we need to wait until it - // fully completes before restoring state. Therefore, we add the target to - // a queue of work. - return fiberBatchedUpdates(fn, bookkeeping); + // fully completes before restoring state. + return fn(bookkeeping); } - isNestingBatched = true; + isBatching = true; try { - return fiberBatchedUpdates(fn, bookkeeping); + return _batchedUpdates(fn, bookkeeping); } finally { // 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. - isNestingBatched = false; - restoreStateIfNeeded(); + isBatching = false; + const controlledComponentsHavePendingUpdates = needsStateRestore(); + if (controlledComponentsHavePendingUpdates) { + // If a controlled event was fired, we may need to restore the state of + // the DOM node back to the controlled value. This is necessary when React + // bails out of the update without touching the DOM. + _flushInteractiveUpdates(); + restoreStateIfNeeded(); + } } } -const ReactGenericBatchingInjection = { - injectFiberBatchedUpdates: function(_batchedUpdates) { - fiberBatchedUpdates = _batchedUpdates; +export function interactiveUpdates(fn, a, b) { + return _interactiveUpdates(fn, a, b); +} + +export function flushInteractiveUpdates() { + return _flushInteractiveUpdates(); +} + +export const injection = { + injectRenderer(renderer) { + _batchedUpdates = renderer.batchedUpdates; + _interactiveUpdates = renderer.interactiveUpdates; + _flushInteractiveUpdates = renderer.flushInteractiveUpdates; }, }; - -export const injection = ReactGenericBatchingInjection; diff --git a/packages/events/ReactSyntheticEventType.js b/packages/events/ReactSyntheticEventType.js index 9f68f83bc1..b2b0b688fc 100644 --- a/packages/events/ReactSyntheticEventType.js +++ b/packages/events/ReactSyntheticEventType.js @@ -17,6 +17,7 @@ export type DispatchConfig = { captured: string, }, registrationName?: string, + isInteractive?: boolean, }; export type ReactSyntheticEvent = { diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 1a0c9fd430..a39bafdf24 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -989,9 +989,7 @@ const DOMRenderer = ReactFiberReconciler({ cancelDeferredCallback: ReactDOMFrameScheduling.cIC, }); -ReactGenericBatching.injection.injectFiberBatchedUpdates( - DOMRenderer.batchedUpdates, -); +ReactGenericBatching.injection.injectRenderer(DOMRenderer); let warnedAboutHydrateAPI = false; @@ -1282,7 +1280,7 @@ const ReactDOM: Object = { return createPortal(...args); }, - unstable_batchedUpdates: ReactGenericBatching.batchedUpdates, + unstable_batchedUpdates: DOMRenderer.batchedUpdates, unstable_deferredUpdates: DOMRenderer.deferredUpdates, diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index 515a083664..83a7247ebc 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -5,7 +5,11 @@ * LICENSE file in the root directory of this source tree. */ -import {batchedUpdates} from 'events/ReactGenericBatching'; +import { + batchedUpdates, + flushInteractiveUpdates, + interactiveUpdates, +} from 'events/ReactGenericBatching'; import {runExtractedEventsInBatch} from 'events/EventPluginHub'; import {isFiberMounted} from 'react-reconciler/reflection'; import {HostRoot} from 'shared/ReactTypeOfWork'; @@ -13,6 +17,9 @@ import {HostRoot} from 'shared/ReactTypeOfWork'; import {addEventBubbleListener, addEventCaptureListener} from './EventListener'; import getEventTarget from './getEventTarget'; import {getClosestInstanceFromNode} from '../client/ReactDOMComponentTree'; +import SimpleEventPlugin from './SimpleEventPlugin'; + +const {isInteractiveTopLevelEventType} = SimpleEventPlugin; const CALLBACK_BOOKKEEPING_POOL_SIZE = 10; const callbackBookkeepingPool = []; @@ -120,10 +127,15 @@ export function trapBubbledEvent(topLevelType, handlerBaseName, element) { if (!element) { return null; } + const dispatch = isInteractiveTopLevelEventType(topLevelType) + ? dispatchInteractiveEvent + : dispatchEvent; + addEventBubbleListener( element, handlerBaseName, - dispatchEvent.bind(null, topLevelType), + // Check if interactive and wrap in interactiveUpdates + dispatch.bind(null, topLevelType), ); } @@ -141,13 +153,27 @@ export function trapCapturedEvent(topLevelType, handlerBaseName, element) { if (!element) { return null; } + const dispatch = isInteractiveTopLevelEventType(topLevelType) + ? dispatchInteractiveEvent + : dispatchEvent; + addEventCaptureListener( element, handlerBaseName, - dispatchEvent.bind(null, topLevelType), + // Check if interactive and wrap in interactiveUpdates + dispatch.bind(null, topLevelType), ); } +function dispatchInteractiveEvent(topLevelType, nativeEvent) { + // If there are any pending interactive updates, synchronously flush them. + // This needs to happen before we read any handlers, because the effect of the + // previous event may affect which handlers are called during this event. + flushInteractiveUpdates(); + // Increase the priority of updates inside this event. + interactiveUpdates(dispatchEvent, topLevelType, nativeEvent); +} + export function dispatchEvent(topLevelType, nativeEvent) { if (!_enabled) { return; diff --git a/packages/react-dom/src/events/SimpleEventPlugin.js b/packages/react-dom/src/events/SimpleEventPlugin.js index a6db694d0a..602624dd31 100644 --- a/packages/react-dom/src/events/SimpleEventPlugin.js +++ b/packages/react-dom/src/events/SimpleEventPlugin.js @@ -49,77 +49,82 @@ import getEventCharCode from './getEventCharCode'; * 'topAbort': { sameConfig } * }; */ -const eventTypes: EventTypes = {}; -const topLevelEventsToDispatchConfig: { - [key: TopLevelTypes]: DispatchConfig, -} = {}; -[ - 'abort', - 'animationEnd', - 'animationIteration', - 'animationStart', +const interactiveEventTypeNames: Array = [ 'blur', 'cancel', - 'canPlay', - 'canPlayThrough', 'click', 'close', 'contextMenu', 'copy', 'cut', 'doubleClick', - 'drag', 'dragEnd', - 'dragEnter', - 'dragExit', - 'dragLeave', - 'dragOver', 'dragStart', 'drop', - 'durationChange', - 'emptied', - 'encrypted', - 'ended', - 'error', 'focus', 'input', 'invalid', 'keyDown', 'keyPress', 'keyUp', - 'load', - 'loadedData', - 'loadedMetadata', - 'loadStart', 'mouseDown', - 'mouseMove', - 'mouseOut', - 'mouseOver', 'mouseUp', 'paste', 'pause', 'play', - 'playing', - 'progress', 'rateChange', 'reset', - 'scroll', 'seeked', + 'submit', + 'touchCancel', + 'touchEnd', + 'touchStart', + 'volumeChange', +]; +const nonInteractiveEventTypeNames: Array = [ + 'abort', + 'animationEnd', + 'animationIteration', + 'animationStart', + 'canPlay', + 'canPlayThrough', + 'drag', + 'dragEnter', + 'dragExit', + 'dragLeave', + 'dragOver', + 'durationChange', + 'emptied', + 'encrypted', + 'ended', + 'error', + 'load', + 'loadedData', + 'loadedMetadata', + 'loadStart', + 'mouseMove', + 'mouseOut', + 'mouseOver', + 'playing', + 'progress', + 'scroll', 'seeking', 'stalled', - 'submit', 'suspend', 'timeUpdate', 'toggle', - 'touchCancel', - 'touchEnd', 'touchMove', - 'touchStart', 'transitionEnd', - 'volumeChange', 'waiting', 'wheel', -].forEach(event => { +]; + +const eventTypes: EventTypes = {}; +const topLevelEventsToDispatchConfig: { + [key: TopLevelTypes]: DispatchConfig, +} = {}; + +function addEventTypeNameToConfig(event: string, isInteractive: boolean) { const capitalizedEvent = event[0].toUpperCase() + event.slice(1); const onEvent = 'on' + capitalizedEvent; const topEvent = 'top' + capitalizedEvent; @@ -130,9 +135,17 @@ const topLevelEventsToDispatchConfig: { captured: onEvent + 'Capture', }, dependencies: [topEvent], + isInteractive, }; eventTypes[event] = type; topLevelEventsToDispatchConfig[topEvent] = type; +} + +interactiveEventTypeNames.forEach(eventTypeName => { + addEventTypeNameToConfig(eventTypeName, true); +}); +nonInteractiveEventTypeNames.forEach(eventTypeName => { + addEventTypeNameToConfig(eventTypeName, false); }); // Only used in DEV for exhaustiveness validation. @@ -173,6 +186,11 @@ const knownHTMLTopLevelTypes = [ const SimpleEventPlugin: PluginModule = { eventTypes: eventTypes, + isInteractiveTopLevelEventType(topLevelType: TopLevelTypes): boolean { + const config = topLevelEventsToDispatchConfig[topLevelType]; + return config !== undefined && config.isInteractive === true; + }, + extractEvents: function( topLevelType: TopLevelTypes, targetInst: Fiber, diff --git a/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js b/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js similarity index 61% rename from packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js rename to packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js index aab3acc176..1b5d690da9 100644 --- a/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js +++ b/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js @@ -10,7 +10,8 @@ 'use strict'; const React = require('react'); -const ReactDOM = require('react-dom'); +let ReactDOM = require('react-dom'); +let ReactFeatureFlags; const setUntrackedChecked = Object.getOwnPropertyDescriptor( HTMLInputElement.prototype, @@ -22,6 +23,11 @@ const setUntrackedValue = Object.getOwnPropertyDescriptor( 'value', ).set; +const setUntrackedTextareaValue = Object.getOwnPropertyDescriptor( + HTMLTextAreaElement.prototype, + 'value', +).set; + describe('ChangeEventPlugin', () => { let container; @@ -439,4 +445,277 @@ describe('ChangeEventPlugin', () => { document.createElement = originalCreateElement; } }); + + describe('async mode', () => { + beforeEach(() => { + jest.resetModules(); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.enableAsyncSubtreeAPI = true; + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.enableCreateRoot = true; + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactDOM = require('react-dom'); + }); + it('text input', () => { + const root = ReactDOM.createRoot(container); + let input; + + let ops = []; + + class ControlledInput extends React.Component { + state = {value: 'initial'}; + onChange = event => this.setState({value: event.target.value}); + render() { + ops.push(`render: ${this.state.value}`); + const controlledValue = + this.state.value === 'changed' ? 'changed [!]' : this.state.value; + return ( + (input = el)} + type="text" + value={controlledValue} + onChange={this.onChange} + /> + ); + } + } + + // Initial mount. Test that this is async. + root.render(); + // Should not have flushed yet. + expect(ops).toEqual([]); + expect(input).toBe(undefined); + // Flush callbacks. + jest.runAllTimers(); + expect(ops).toEqual(['render: initial']); + expect(input.value).toBe('initial'); + + ops = []; + + // Trigger a change event. + setUntrackedValue.call(input, 'changed'); + input.dispatchEvent( + new Event('input', {bubbles: true, cancelable: true}), + ); + // Change should synchronously flush + expect(ops).toEqual(['render: changed']); + // Value should be the controlled value, not the original one + expect(input.value).toBe('changed [!]'); + }); + + it('checkbox input', () => { + const root = ReactDOM.createRoot(container); + let input; + + let ops = []; + + class ControlledInput extends React.Component { + state = {checked: false}; + onChange = event => { + this.setState({checked: event.target.checked}); + }; + render() { + ops.push(`render: ${this.state.checked}`); + const controlledValue = this.props.reverse + ? !this.state.checked + : this.state.checked; + return ( + (input = el)} + type="checkbox" + checked={controlledValue} + onChange={this.onChange} + /> + ); + } + } + + // Initial mount. Test that this is async. + root.render(); + // Should not have flushed yet. + expect(ops).toEqual([]); + expect(input).toBe(undefined); + // Flush callbacks. + jest.runAllTimers(); + expect(ops).toEqual(['render: false']); + expect(input.checked).toBe(false); + + ops = []; + + // Trigger a change event. + input.dispatchEvent( + new MouseEvent('click', {bubbles: true, cancelable: true}), + ); + // Change should synchronously flush + expect(ops).toEqual(['render: true']); + expect(input.checked).toBe(true); + + // Now let's make sure we're using the controlled value. + root.render(); + jest.runAllTimers(); + + ops = []; + + // Trigger another change event. + input.dispatchEvent( + new MouseEvent('click', {bubbles: true, cancelable: true}), + ); + // Change should synchronously flush + expect(ops).toEqual(['render: true']); + expect(input.checked).toBe(false); + }); + + it('textarea', () => { + const root = ReactDOM.createRoot(container); + let textarea; + + let ops = []; + + class ControlledTextarea extends React.Component { + state = {value: 'initial'}; + onChange = event => this.setState({value: event.target.value}); + render() { + ops.push(`render: ${this.state.value}`); + const controlledValue = + this.state.value === 'changed' ? 'changed [!]' : this.state.value; + return ( +