Skip to content

Commit

Permalink
[Events] Nested discrete events across systems
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@acdlite
acdlite committed Jun 1, 2019
1 parent 7aa35ce commit 5763f1d
Show file tree
Hide file tree
Showing 6 changed files with 156 additions and 80 deletions.
55 changes: 43 additions & 12 deletions packages/events/ReactGenericBatching.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
needsStateRestore,
restoreStateIfNeeded,
} from './ReactControlledComponent';
import {enableEventAPI} from 'shared/ReactFeatureFlags';

// 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
Expand All @@ -26,14 +27,13 @@ let discreteUpdatesImpl = function(fn, a, b, c) {
let flushDiscreteUpdatesImpl = function() {};
let batchedEventUpdatesImpl = batchedUpdatesImpl;

let isBatching = false;
let isInsideEventHandler = false;

function batchedUpdatesFinally() {
function finishEventHandler() {
// 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.
isBatching = false;
const controlledComponentsHavePendingUpdates = needsStateRestore();
if (controlledComponentsHavePendingUpdates) {
// If a controlled event was fired, we may need to restore the state of
Expand All @@ -45,39 +45,70 @@ function batchedUpdatesFinally() {
}

export function batchedUpdates(fn, bookkeeping) {
if (isBatching) {
if (isInsideEventHandler) {
// If we are currently inside another batch, we need to wait until it
// fully completes before restoring state.
return fn(bookkeeping);
}
isBatching = true;
isInsideEventHandler = true;
try {
return batchedUpdatesImpl(fn, bookkeeping);
} finally {
batchedUpdatesFinally();
isInsideEventHandler = false;
finishEventHandler();
}
}

export function batchedEventUpdates(fn, bookkeeping) {
if (isBatching) {
if (isInsideEventHandler) {
// If we are currently inside another batch, we need to wait until it
// fully completes before restoring state.
return fn(bookkeeping);
}
isBatching = true;
isInsideEventHandler = true;
try {
return batchedEventUpdatesImpl(fn, bookkeeping);
} finally {
batchedUpdatesFinally();
isInsideEventHandler = false;
finishEventHandler();
}
}

export function discreteUpdates(fn, a, b, c) {
return discreteUpdatesImpl(fn, a, b, c);
const prevIsInsideEventHandler = isInsideEventHandler;
isInsideEventHandler = true;
try {
return discreteUpdatesImpl(fn, a, b, c);
} finally {
isInsideEventHandler = prevIsInsideEventHandler;
if (!isInsideEventHandler) {
finishEventHandler();
}
}
}

export function flushDiscreteUpdates() {
return flushDiscreteUpdatesImpl();
let lastFlushedEventTimeStamp = 0;
export function flushDiscreteUpdatesIfNeeded(timeStamp: number) {
// event.timeStamp isn't overly reliable due to inconsistencies in
// how different browsers have historically provided the time stamp.
// Some browsers provide high-resolution time stamps for all events,
// some provide low-resoltion time stamps for all events. FF < 52
// even mixes both time stamps together. Some browsers even report
// negative time stamps or time stamps that are 0 (iOS9) in some cases.
// Given we are only comparing two time stamps with equality (!==),
// we are safe from the resolution differences. If the time stamp is 0
// we bail-out of preventing the flush, which can affect semantics,
// such as if an earlier flush removes or adds event listeners that
// are fired in the subsequent flush. However, this is the same
// behaviour as we had before this change, so the risks are low.
if (
!isInsideEventHandler &&
(!enableEventAPI ||
(timeStamp === 0 || lastFlushedEventTimeStamp !== timeStamp))
) {
lastFlushedEventTimeStamp = timeStamp;
flushDiscreteUpdatesImpl();
}
}

export function setBatchingImplementation(
Expand Down
28 changes: 2 additions & 26 deletions packages/react-dom/src/events/DOMEventResponderSystem.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes';
import {
batchedEventUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushDiscreteUpdatesIfNeeded,
} from 'events/ReactGenericBatching';
import type {Fiber} from 'react-reconciler/src/ReactFiber';
import warning from 'shared/warning';
Expand Down Expand Up @@ -610,9 +610,7 @@ export function processEventQueue(): void {
return;
}
if (discrete) {
if (shouldFlushDiscreteUpdates(currentTimeStamp)) {
flushDiscreteUpdates();
}
flushDiscreteUpdatesIfNeeded(currentTimeStamp);
discreteUpdates(() => {
batchedEventUpdates(processEvents, events);
});
Expand Down Expand Up @@ -1016,25 +1014,3 @@ export function generateListeningKey(
const passiveKey = passive ? '_passive' : '_active';
return `${topLevelType}${passiveKey}`;
}

let lastDiscreteEventTimeStamp = 0;

export function shouldFlushDiscreteUpdates(timeStamp: number): boolean {
// event.timeStamp isn't overly reliable due to inconsistencies in
// how different browsers have historically provided the time stamp.
// Some browsers provide high-resolution time stamps for all events,
// some provide low-resoltion time stamps for all events. FF < 52
// even mixes both time stamps together. Some browsers even report
// negative time stamps or time stamps that are 0 (iOS9) in some cases.
// Given we are only comparing two time stamps with equality (!==),
// we are safe from the resolution differences. If the time stamp is 0
// we bail-out of preventing the flush, which can affect semantics,
// such as if an earlier flush removes or adds event listeners that
// are fired in the subsequent flush. However, this is the same
// behaviour as we had before this change, so the risks are low.
if (timeStamp === 0 || lastDiscreteEventTimeStamp !== timeStamp) {
lastDiscreteEventTimeStamp = timeStamp;
return true;
}
return false;
}
11 changes: 3 additions & 8 deletions packages/react-dom/src/events/ReactDOMEventListener.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,10 @@ import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes';
import {
batchedEventUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushDiscreteUpdatesIfNeeded,
} from 'events/ReactGenericBatching';
import {runExtractedPluginEventsInBatch} from 'events/EventPluginHub';
import {
dispatchEventForResponderEventSystem,
shouldFlushDiscreteUpdates,
} from '../events/DOMEventResponderSystem';
import {dispatchEventForResponderEventSystem} from '../events/DOMEventResponderSystem';
import {isFiberMounted} from 'react-reconciler/reflection';
import {HostRoot} from 'shared/ReactWorkTags';
import {
Expand Down Expand Up @@ -229,9 +226,7 @@ function trapEventForPluginEventSystem(
}

function dispatchDiscreteEvent(topLevelType, eventSystemFlags, nativeEvent) {
if (!enableEventAPI || shouldFlushDiscreteUpdates(nativeEvent.timeStamp)) {
flushDiscreteUpdates();
}
flushDiscreteUpdatesIfNeeded(nativeEvent.timeStamp);
discreteUpdates(dispatchEvent, topLevelType, eventSystemFlags, nativeEvent);
}

Expand Down
6 changes: 4 additions & 2 deletions packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

'use strict';

const React = require('react');
let React = require('react');
let ReactDOM = require('react-dom');
let ReactFeatureFlags;
let Scheduler;
Expand Down Expand Up @@ -479,14 +479,16 @@ describe('ChangeEventPlugin', () => {
}
});

describe('async mode', () => {
describe('concurrent mode', () => {
beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');
});

it('text input', () => {
const root = ReactDOM.unstable_createRoot(container);
let input;
Expand Down
55 changes: 55 additions & 0 deletions packages/react-events/src/__tests__/Press-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2583,6 +2583,61 @@ describe('Event responder: Press', () => {
document.body.removeChild(newContainer);
});

it(
'should only flush before outermost discrete event handler when mixing ' +
'event systems',
async () => {
const {useState} = React;

const button = React.createRef();

const ops = [];

function MyComponent() {
const [pressesCount, updatePressesCount] = useState(0);
const [clicksCount, updateClicksCount] = useState(0);

function handlePress() {
// This dispatches a synchronous, discrete event in the legacy event
// system. However, because it's nested inside the new event system,
// its updates should not flush until the end of the outer handler.
button.current.click();
// Text context should not have changed
ops.push(newContainer.textContent);
updatePressesCount(pressesCount + 1);
}

return (
<div>
<Press onPress={handlePress}>
<button
ref={button}
onClick={() => updateClicksCount(clicksCount + 1)}>
Presses: {pressesCount}, Clicks: {clicksCount}
</button>
</Press>
</div>
);
}

const newContainer = document.createElement('div');
document.body.appendChild(newContainer);
const root = ReactDOM.unstable_createRoot(newContainer);

root.render(<MyComponent />);
Scheduler.flushAll();
expect(newContainer.textContent).toEqual('Presses: 0, Clicks: 0');

dispatchEventWithTimeStamp(button.current, 'pointerdown', 100);
dispatchEventWithTimeStamp(button.current, 'pointerup', 100);
dispatchEventWithTimeStamp(button.current, 'click', 100);
Scheduler.flushAll();
expect(newContainer.textContent).toEqual('Presses: 1, Clicks: 1');

expect(ops).toEqual(['Presses: 0, Clicks: 0']);
},
);

describe('onContextMenu', () => {
it('is called after a right mouse click', () => {
const onContextMenu = jest.fn();
Expand Down
81 changes: 49 additions & 32 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,12 +178,13 @@ const {

type ExecutionContext = number;

const NoContext = /* */ 0b00000;
const BatchedContext = /* */ 0b00001;
const EventContext = /* */ 0b00010;
const LegacyUnbatchedContext = /* */ 0b00100;
const RenderContext = /* */ 0b01000;
const CommitContext = /* */ 0b10000;
const NoContext = /* */ 0b000000;
const BatchedContext = /* */ 0b000001;
const EventContext = /* */ 0b000010;
const DiscreteEventContext = /* */ 0b000100;
const LegacyUnbatchedContext = /* */ 0b001000;
const RenderContext = /* */ 0b010000;
const CommitContext = /* */ 0b100000;

type RootExitStatus = 0 | 1 | 2 | 3 | 4;
const RootIncomplete = 0;
Expand Down Expand Up @@ -363,6 +364,10 @@ export function scheduleUpdateOnFiber(
checkForInterruption(fiber, expirationTime);
recordScheduleUpdate();

// TODO: computeExpirationForFiber also reads the priority. Pass the
// priority as an argument to that function and this one.
const priorityLevel = getCurrentPriorityLevel();

if (expirationTime === Sync) {
if (
// Check if we're inside unbatchedUpdates
Expand Down Expand Up @@ -392,25 +397,26 @@ export function scheduleUpdateOnFiber(
}
}
} else {
// TODO: computeExpirationForFiber also reads the priority. Pass the
// priority as an argument to that function and this one.
const priorityLevel = getCurrentPriorityLevel();
if (priorityLevel === UserBlockingPriority) {
// This is the result of a discrete event. Track the lowest priority
// discrete update per root so we can flush them early, if needed.
if (rootsWithPendingDiscreteUpdates === null) {
rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]);
} else {
const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
if (
lastDiscreteTime === undefined ||
lastDiscreteTime > expirationTime
) {
rootsWithPendingDiscreteUpdates.set(root, expirationTime);
}
scheduleCallbackForRoot(root, priorityLevel, expirationTime);
}

if (
(executionContext & DiscreteEventContext) !== NoContext &&
// Only updates at user-blocking priority or greater are considered
// discrete, even inside a discrete event.
(priorityLevel === UserBlockingPriority ||
priorityLevel === ImmediatePriority)
) {
// This is the result of a discrete event. Track the lowest priority
// discrete update per root so we can flush them early, if needed.
if (rootsWithPendingDiscreteUpdates === null) {
rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]);
} else {
const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
if (lastDiscreteTime === undefined || lastDiscreteTime > expirationTime) {
rootsWithPendingDiscreteUpdates.set(root, expirationTime);
}
}
scheduleCallbackForRoot(root, priorityLevel, expirationTime);
}
}
export const scheduleWork = scheduleUpdateOnFiber;
Expand Down Expand Up @@ -623,15 +629,6 @@ export function deferredUpdates<A>(fn: () => A): A {
return runWithPriority(NormalPriority, fn);
}

export function discreteUpdates<A, B, C, R>(
fn: (A, B, C) => R,
a: A,
b: B,
c: C,
): R {
return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c));
}

export function syncUpdates<A, B, C, R>(
fn: (A, B, C) => R,
a: A,
Expand Down Expand Up @@ -683,6 +680,26 @@ export function batchedEventUpdates<A, R>(fn: A => R, a: A): R {
}
}

export function discreteUpdates<A, B, C, R>(
fn: (A, B, C) => R,
a: A,
b: B,
c: C,
): R {
const prevExecutionContext = executionContext;
executionContext |= DiscreteEventContext;
try {
// Should this
return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c));
} finally {
executionContext = prevExecutionContext;
if (executionContext === NoContext) {
// Flush the immediate callbacks that were scheduled during this batch
flushSyncCallbackQueue();
}
}
}

export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
const prevExecutionContext = executionContext;
executionContext &= ~BatchedContext;
Expand Down

0 comments on commit 5763f1d

Please sign in to comment.