[Events] Nested discrete events across systems
If an event in the old system is dispatched synchronously within an event from the new system, or vice versa, and the inner event is a discrete update, React should not flush pending discrete updates before firing the inner event's handlers, even if the outer event is not discrete. Another way of saying this is that nested events should never force React to flush discrete updates. Arguably, if the outer event is not a discrete event, then the inner event _should_ flush the pending events. However, that would be a breaking change. I would argue this isn't so bad, however, given that nested events are pretty rare. They don't fit nicely into our event model regardless, since we don't support nested React renders. In the future we should consider warning when events are nested.
Andrew Clark committed
May 31, 2019 at 16:33 UTC
5763f1d4c458b9d9a769a28b9d92b698bc94533f
6 files changed
+156
-80
packages/events/ReactGenericBatching.js
+43
-12
@@ -9,6 +9,7 @@ import {
9
needsStateRestore,
10
restoreStateIfNeeded,
11
} from './ReactControlledComponent';
12
+import {enableEventAPI} from 'shared/ReactFeatureFlags';
13
14
// Used as a way to call batchedUpdates when we don't have a reference to
15
// the renderer. Such as when we're dispatching events or if third party
@@ -26,14 +27,13 @@ let discreteUpdatesImpl = function(fn, a, b, c) {
27
let flushDiscreteUpdatesImpl = function() {};
28
let batchedEventUpdatesImpl = batchedUpdatesImpl;
29
29
-let isBatching = false;
30
+let isInsideEventHandler = false;
31
31
-function batchedUpdatesFinally() {
32
+function finishEventHandler() {
33
// Here we wait until all updates have propagated, which is important
34
// when using controlled components within layers:
35
// https://github.com/facebook/react/issues/1698
36
// Then we restore state of any controlled component.
36
- isBatching = false;
37
const controlledComponentsHavePendingUpdates = needsStateRestore();
38
if (controlledComponentsHavePendingUpdates) {
39
// If a controlled event was fired, we may need to restore the state of
@@ -45,39 +45,70 @@ function batchedUpdatesFinally() {
45
}
46
47
export function batchedUpdates(fn, bookkeeping) {
48
- if (isBatching) {
48
+ if (isInsideEventHandler) {
49
// If we are currently inside another batch, we need to wait until it
50
// fully completes before restoring state.
51
return fn(bookkeeping);
52
}
53
- isBatching = true;
53
+ isInsideEventHandler = true;
54
try {
55
return batchedUpdatesImpl(fn, bookkeeping);
56
} finally {
57
- batchedUpdatesFinally();
57
+ isInsideEventHandler = false;
58
+ finishEventHandler();
59
}
60
}
61
62
export function batchedEventUpdates(fn, bookkeeping) {
62
- if (isBatching) {
63
+ if (isInsideEventHandler) {
64
// If we are currently inside another batch, we need to wait until it
65
// fully completes before restoring state.
66
return fn(bookkeeping);
67
}
67
- isBatching = true;
68
+ isInsideEventHandler = true;
69
try {
70
return batchedEventUpdatesImpl(fn, bookkeeping);
71
} finally {
71
- batchedUpdatesFinally();
72
+ isInsideEventHandler = false;
73
+ finishEventHandler();
74
}
75
}
76
77
export function discreteUpdates(fn, a, b, c) {
76
- return discreteUpdatesImpl(fn, a, b, c);
78
+ const prevIsInsideEventHandler = isInsideEventHandler;
79
+ isInsideEventHandler = true;
80
+ try {
81
+ return discreteUpdatesImpl(fn, a, b, c);
82
+ } finally {
83
+ isInsideEventHandler = prevIsInsideEventHandler;
84
+ if (!isInsideEventHandler) {
85
+ finishEventHandler();
86
+ }
87
+ }
88
}
89
79
-export function flushDiscreteUpdates() {
80
- return flushDiscreteUpdatesImpl();
90
+let lastFlushedEventTimeStamp = 0;
91
+export function flushDiscreteUpdatesIfNeeded(timeStamp: number) {
92
+ // event.timeStamp isn't overly reliable due to inconsistencies in
93
+ // how different browsers have historically provided the time stamp.
94
+ // Some browsers provide high-resolution time stamps for all events,
95
+ // some provide low-resoltion time stamps for all events. FF < 52
96
+ // even mixes both time stamps together. Some browsers even report
97
+ // negative time stamps or time stamps that are 0 (iOS9) in some cases.
98
+ // Given we are only comparing two time stamps with equality (!==),
99
+ // we are safe from the resolution differences. If the time stamp is 0
100
+ // we bail-out of preventing the flush, which can affect semantics,
101
+ // such as if an earlier flush removes or adds event listeners that
102
+ // are fired in the subsequent flush. However, this is the same
103
+ // behaviour as we had before this change, so the risks are low.
104
+ if (
105
+ !isInsideEventHandler &&
106
+ (!enableEventAPI ||
107
+ (timeStamp === 0 || lastFlushedEventTimeStamp !== timeStamp))
108
+ ) {
109
+ lastFlushedEventTimeStamp = timeStamp;
110
+ flushDiscreteUpdatesImpl();
111
+ }
112
}
113
114
export function setBatchingImplementation(
packages/react-dom/src/events/DOMEventResponderSystem.js
+2
-26
@@ -28,7 +28,7 @@ import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes';
28
import {
29
batchedEventUpdates,
30
discreteUpdates,
31
- flushDiscreteUpdates,
31
+ flushDiscreteUpdatesIfNeeded,
32
} from 'events/ReactGenericBatching';
33
import type {Fiber} from 'react-reconciler/src/ReactFiber';
34
import warning from 'shared/warning';
@@ -610,9 +610,7 @@ export function processEventQueue(): void {
610
return;
611
}
612
if (discrete) {
613
- if (shouldFlushDiscreteUpdates(currentTimeStamp)) {
614
- flushDiscreteUpdates();
615
- }
613
+ flushDiscreteUpdatesIfNeeded(currentTimeStamp);
614
discreteUpdates(() => {
615
batchedEventUpdates(processEvents, events);
616
});
@@ -1016,25 +1014,3 @@ export function generateListeningKey(
1014
const passiveKey = passive ? '_passive' : '_active';
1015
return `${topLevelType}${passiveKey}`;
1016
}
1019
-
1020
-let lastDiscreteEventTimeStamp = 0;
1021
-
1022
-export function shouldFlushDiscreteUpdates(timeStamp: number): boolean {
1023
- // event.timeStamp isn't overly reliable due to inconsistencies in
1024
- // how different browsers have historically provided the time stamp.
1025
- // Some browsers provide high-resolution time stamps for all events,
1026
- // some provide low-resoltion time stamps for all events. FF < 52
1027
- // even mixes both time stamps together. Some browsers even report
1028
- // negative time stamps or time stamps that are 0 (iOS9) in some cases.
1029
- // Given we are only comparing two time stamps with equality (!==),
1030
- // we are safe from the resolution differences. If the time stamp is 0
1031
- // we bail-out of preventing the flush, which can affect semantics,
1032
- // such as if an earlier flush removes or adds event listeners that
1033
- // are fired in the subsequent flush. However, this is the same
1034
- // behaviour as we had before this change, so the risks are low.
1035
- if (timeStamp === 0 || lastDiscreteEventTimeStamp !== timeStamp) {
1036
- lastDiscreteEventTimeStamp = timeStamp;
1037
- return true;
1038
- }
1039
- return false;
1040
-}
packages/react-dom/src/events/ReactDOMEventListener.js
+3
-8
@@ -14,13 +14,10 @@ import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes';
14
import {
15
batchedEventUpdates,
16
discreteUpdates,
17
- flushDiscreteUpdates,
17
+ flushDiscreteUpdatesIfNeeded,
18
} from 'events/ReactGenericBatching';
19
import {runExtractedPluginEventsInBatch} from 'events/EventPluginHub';
20
-import {
21
- dispatchEventForResponderEventSystem,
22
- shouldFlushDiscreteUpdates,
23
-} from '../events/DOMEventResponderSystem';
20
+import {dispatchEventForResponderEventSystem} from '../events/DOMEventResponderSystem';
21
import {isFiberMounted} from 'react-reconciler/reflection';
22
import {HostRoot} from 'shared/ReactWorkTags';
23
import {
@@ -229,9 +226,7 @@ function trapEventForPluginEventSystem(
226
}
227
228
function dispatchDiscreteEvent(topLevelType, eventSystemFlags, nativeEvent) {
232
- if (!enableEventAPI || shouldFlushDiscreteUpdates(nativeEvent.timeStamp)) {
233
- flushDiscreteUpdates();
234
- }
229
+ flushDiscreteUpdatesIfNeeded(nativeEvent.timeStamp);
230
discreteUpdates(dispatchEvent, topLevelType, eventSystemFlags, nativeEvent);
231
}
232
packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js
+4
-2
@@ -9,7 +9,7 @@
9
10
'use strict';
11
12
-const React = require('react');
12
+let React = require('react');
13
let ReactDOM = require('react-dom');
14
let ReactFeatureFlags;
15
let Scheduler;
@@ -479,14 +479,16 @@ describe('ChangeEventPlugin', () => {
479
}
480
});
481
482
- describe('async mode', () => {
482
+ describe('concurrent mode', () => {
483
beforeEach(() => {
484
jest.resetModules();
485
ReactFeatureFlags = require('shared/ReactFeatureFlags');
486
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
487
+ React = require('react');
488
ReactDOM = require('react-dom');
489
Scheduler = require('scheduler');
490
});
491
+
492
it('text input', () => {
493
const root = ReactDOM.unstable_createRoot(container);
494
let input;
packages/react-events/src/__tests__/Press-test.internal.js
+55
@@ -2583,6 +2583,61 @@ describe('Event responder: Press', () => {
2583
document.body.removeChild(newContainer);
2584
});
2585
2586
+ it(
2587
+ 'should only flush before outermost discrete event handler when mixing ' +
2588
+ 'event systems',
2589
+ async () => {
2590
+ const {useState} = React;
2591
+
2592
+ const button = React.createRef();
2593
+
2594
+ const ops = [];
2595
+
2596
+ function MyComponent() {
2597
+ const [pressesCount, updatePressesCount] = useState(0);
2598
+ const [clicksCount, updateClicksCount] = useState(0);
2599
+
2600
+ function handlePress() {
2601
+ // This dispatches a synchronous, discrete event in the legacy event
2602
+ // system. However, because it's nested inside the new event system,
2603
+ // its updates should not flush until the end of the outer handler.
2604
+ button.current.click();
2605
+ // Text context should not have changed
2606
+ ops.push(newContainer.textContent);
2607
+ updatePressesCount(pressesCount + 1);
2608
+ }
2609
+
2610
+ return (
2611
+ <div>
2612
+ <Press onPress={handlePress}>
2613
+ <button
2614
+ ref={button}
2615
+ onClick={() => updateClicksCount(clicksCount + 1)}>
2616
+ Presses: {pressesCount}, Clicks: {clicksCount}
2617
+ </button>
2618
+ </Press>
2619
+ </div>
2620
+ );
2621
+ }
2622
+
2623
+ const newContainer = document.createElement('div');
2624
+ document.body.appendChild(newContainer);
2625
+ const root = ReactDOM.unstable_createRoot(newContainer);
2626
+
2627
+ root.render(<MyComponent />);
2628
+ Scheduler.flushAll();
2629
+ expect(newContainer.textContent).toEqual('Presses: 0, Clicks: 0');
2630
+
2631
+ dispatchEventWithTimeStamp(button.current, 'pointerdown', 100);
2632
+ dispatchEventWithTimeStamp(button.current, 'pointerup', 100);
2633
+ dispatchEventWithTimeStamp(button.current, 'click', 100);
2634
+ Scheduler.flushAll();
2635
+ expect(newContainer.textContent).toEqual('Presses: 1, Clicks: 1');
2636
+
2637
+ expect(ops).toEqual(['Presses: 0, Clicks: 0']);
2638
+ },
2639
+ );
2640
+
2641
describe('onContextMenu', () => {
2642
it('is called after a right mouse click', () => {
2643
const onContextMenu = jest.fn();
packages/react-reconciler/src/ReactFiberWorkLoop.js
+49
-32
@@ -178,12 +178,13 @@ const {
178
179
type ExecutionContext = number;
180
181
-const NoContext = /* */ 0b00000;
182
-const BatchedContext = /* */ 0b00001;
183
-const EventContext = /* */ 0b00010;
184
-const LegacyUnbatchedContext = /* */ 0b00100;
185
-const RenderContext = /* */ 0b01000;
186
-const CommitContext = /* */ 0b10000;
181
+const NoContext = /* */ 0b000000;
182
+const BatchedContext = /* */ 0b000001;
183
+const EventContext = /* */ 0b000010;
184
+const DiscreteEventContext = /* */ 0b000100;
185
+const LegacyUnbatchedContext = /* */ 0b001000;
186
+const RenderContext = /* */ 0b010000;
187
+const CommitContext = /* */ 0b100000;
188
189
type RootExitStatus = 0 | 1 | 2 | 3 | 4;
190
const RootIncomplete = 0;
@@ -363,6 +364,10 @@ export function scheduleUpdateOnFiber(
364
checkForInterruption(fiber, expirationTime);
365
recordScheduleUpdate();
366
367
+ // TODO: computeExpirationForFiber also reads the priority. Pass the
368
+ // priority as an argument to that function and this one.
369
+ const priorityLevel = getCurrentPriorityLevel();
370
+
371
if (expirationTime === Sync) {
372
if (
373
// Check if we're inside unbatchedUpdates
@@ -392,25 +397,26 @@ export function scheduleUpdateOnFiber(
397
}
398
}
399
} else {
395
- // TODO: computeExpirationForFiber also reads the priority. Pass the
396
- // priority as an argument to that function and this one.
397
- const priorityLevel = getCurrentPriorityLevel();
398
- if (priorityLevel === UserBlockingPriority) {
399
- // This is the result of a discrete event. Track the lowest priority
400
- // discrete update per root so we can flush them early, if needed.
401
- if (rootsWithPendingDiscreteUpdates === null) {
402
- rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]);
403
- } else {
404
- const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
405
- if (
406
- lastDiscreteTime === undefined ||
407
- lastDiscreteTime > expirationTime
408
- ) {
409
- rootsWithPendingDiscreteUpdates.set(root, expirationTime);
410
- }
400
+ scheduleCallbackForRoot(root, priorityLevel, expirationTime);
401
+ }
402
+
403
+ if (
404
+ (executionContext & DiscreteEventContext) !== NoContext &&
405
+ // Only updates at user-blocking priority or greater are considered
406
+ // discrete, even inside a discrete event.
407
+ (priorityLevel === UserBlockingPriority ||
408
+ priorityLevel === ImmediatePriority)
409
+ ) {
410
+ // This is the result of a discrete event. Track the lowest priority
411
+ // discrete update per root so we can flush them early, if needed.
412
+ if (rootsWithPendingDiscreteUpdates === null) {
413
+ rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]);
414
+ } else {
415
+ const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
416
+ if (lastDiscreteTime === undefined || lastDiscreteTime > expirationTime) {
417
+ rootsWithPendingDiscreteUpdates.set(root, expirationTime);
418
}
419
}
413
- scheduleCallbackForRoot(root, priorityLevel, expirationTime);
420
}
421
}
422
export const scheduleWork = scheduleUpdateOnFiber;
@@ -623,15 +629,6 @@ export function deferredUpdates<A>(fn: () => A): A {
629
return runWithPriority(NormalPriority, fn);
630
}
631
626
-export function discreteUpdates<A, B, C, R>(
627
- fn: (A, B, C) => R,
628
- a: A,
629
- b: B,
630
- c: C,
631
-): R {
632
- return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c));
633
-}
634
-
632
export function syncUpdates<A, B, C, R>(
633
fn: (A, B, C) => R,
634
a: A,
@@ -683,6 +680,26 @@ export function batchedEventUpdates<A, R>(fn: A => R, a: A): R {
680
}
681
}
682
683
+export function discreteUpdates<A, B, C, R>(
684
+ fn: (A, B, C) => R,
685
+ a: A,
686
+ b: B,
687
+ c: C,
688
+): R {
689
+ const prevExecutionContext = executionContext;
690
+ executionContext |= DiscreteEventContext;
691
+ try {
692
+ // Should this
693
+ return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c));
694
+ } finally {
695
+ executionContext = prevExecutionContext;
696
+ if (executionContext === NoContext) {
697
+ // Flush the immediate callbacks that were scheduled during this batch
698
+ flushSyncCallbackQueue();
699
+ }
700
+ }
701
+}
702
+
703
export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
704
const prevExecutionContext = executionContext;
705
executionContext &= ~BatchedContext;