Skip to content

Commit

Permalink
Remove Set bookkeeping for root events (facebook#19990)
Browse files Browse the repository at this point in the history 
* Remove dead code branch

This function is only called when initializing roots/containers (where we skip non-delegated events) and in the createEventHandle path for non-DOM nodes (where we never hit this path because targetElement is null).

* Move related functions close to each other

* Fork listenToNativeEvent for createEventHandle

It doesn't need all of the logic that's needed for normal event path.

And the normal codepath doesn't use the last two arguments.

* Expand test coverage for non-delegated events

This changes a test to fail if we removed the event handler Sets. Previously, we didn't cover that.

* Add DEV-level check that top-level events and non-delegated events do not overlap

This makes us confident that they're mutually exclusive and there is no duplication between them.

* Add a test verifying selectionchange deduplication

This is why we still need the Set bookkeeping. Adding a test for it.

* Remove Set bookkeeping for root events

Root events don't intersect with non-delegated bubbled events (so no need to deduplicate there). They also don't intersect with createEventHandle non-managed events (because those don't go on the DOM elements). So we can remove the bookeeping because we already have code ensuring the eager subscriptions only run once per element.

I've moved the selectionchange special case outside, and added document-level deduplication for it alone.

Technically this might change the behavior of createEventHandle with selectionchange on the document, but we're not using that, and I'm not sure that behavior makes sense anyway.

* Flow
  • Loading branch information
@gaearon @koto
gaearon authored and koto committed Jun 15, 2021
1 parent ce264b0 commit cddd747
Show file tree
Hide file tree
Showing 3 changed files with 173 additions and 81 deletions.
107 changes: 101 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -820,19 +820,21 @@ describe('ReactDOMEventListener', () => {
</div>,
container,
);

// Update to attach.
ReactDOM.render(
<div
className="grand"
onScroll={onScroll}
onScrollCapture={onScrollCapture}>
onScroll={e => onScroll(e)}
onScrollCapture={e => onScrollCapture(e)}>
<div
className="parent"
onScroll={onScroll}
onScrollCapture={onScrollCapture}>
onScroll={e => onScroll(e)}
onScrollCapture={e => onScrollCapture(e)}>
<div
className="child"
onScroll={onScroll}
onScrollCapture={onScrollCapture}
onScroll={e => onScroll(e)}
onScrollCapture={e => onScrollCapture(e)}
ref={ref}
/>
</div>
Expand All @@ -850,6 +852,58 @@ describe('ReactDOMEventListener', () => {
['capture', 'child'],
['bubble', 'child'],
]);

// Update to verify deduplication.
log.length = 0;
ReactDOM.render(
<div
className="grand"
// Note: these are intentionally inline functions so that
// we hit the reattachment codepath instead of bailing out.
onScroll={e => onScroll(e)}
onScrollCapture={e => onScrollCapture(e)}>
<div
className="parent"
onScroll={e => onScroll(e)}
onScrollCapture={e => onScrollCapture(e)}>
<div
className="child"
onScroll={e => onScroll(e)}
onScrollCapture={e => onScrollCapture(e)}
ref={ref}
/>
</div>
</div>,
container,
);
ref.current.dispatchEvent(
new Event('scroll', {
bubbles: false,
}),
);
expect(log).toEqual([
['capture', 'grand'],
['capture', 'parent'],
['capture', 'child'],
['bubble', 'child'],
]);

// Update to detach.
log.length = 0;
ReactDOM.render(
<div>
<div>
<div ref={ref} />
</div>
</div>,
container,
);
ref.current.dispatchEvent(
new Event('scroll', {
bubbles: false,
}),
);
expect(log).toEqual([]);
} finally {
document.body.removeChild(container);
}
Expand Down Expand Up @@ -899,8 +953,49 @@ describe('ReactDOMEventListener', () => {
['capture', 'child'],
['bubble', 'child'],
]);

log.length = 0;
ReactDOM.render(
<div>
<div>
<div ref={ref} />
</div>
</div>,
container,
);
ref.current.dispatchEvent(
new Event('scroll', {
bubbles: false,
}),
);
expect(log).toEqual([]);
} finally {
document.body.removeChild(container);
}
});

it('should not subscribe to selectionchange twice', () => {
const log = [];

const originalDocAddEventListener = document.addEventListener;
document.addEventListener = function(type, fn, options) {
switch (type) {
case 'selectionchange':
log.push(options);
break;
default:
throw new Error(
`Did not expect to add a document-level listener for the "${type}" event.`,
);
}
};
try {
ReactDOM.render(<input />, document.createElement('div'));
ReactDOM.render(<input />, document.createElement('div'));
} finally {
document.addEventListener = originalDocAddEventListener;
}

expect(log).toEqual([false]);
});
});
8 changes: 2 additions & 6 deletions packages/react-dom/src/client/ReactDOMEventHandle.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ import {
addEventHandleToTarget,
} from './ReactDOMComponentTree';
import {ELEMENT_NODE} from '../shared/HTMLNodeType';
import {listenToNativeEvent} from '../events/DOMPluginEventSystem';

import {IS_EVENT_HANDLE_NON_MANAGED_NODE} from '../events/EventSystemFlags';
import {listenToNativeEventForNonManagedEventTarget} from '../events/DOMPluginEventSystem';

import {
enableScopeAPI,
Expand Down Expand Up @@ -69,12 +67,10 @@ function registerReactDOMEvent(
const eventTarget = ((target: any): EventTarget);
// These are valid event targets, but they are also
// non-managed React nodes.
listenToNativeEvent(
listenToNativeEventForNonManagedEventTarget(
domEventName,
isCapturePhaseListener,
eventTarget,
null,
IS_EVENT_HANDLE_NON_MANAGED_NODE,
);
} else {
invariant(
Expand Down
139 changes: 70 additions & 69 deletions packages/react-dom/src/events/DOMPluginEventSystem.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,15 @@ export function listenToNonDelegatedEvent(
domEventName: DOMEventName,
targetElement: Element,
): void {
if (__DEV__) {
if (!nonDelegatedEvents.has(domEventName)) {
console.error(
'Did not expect a listenToNonDelegatedEvent() call for "%s". ' +
'This is a bug in React. Please file an issue.',
domEventName,
);
}
}
const isCapturePhaseListener = false;
const listenerSet = getEventListenerSet(targetElement);
const listenerSetKey = getListenerSetKey(
Expand All @@ -312,88 +321,46 @@ export function listenToNonDelegatedEvent(
}
}

const listeningMarker =
'_reactListening' +
Math.random()
.toString(36)
.slice(2);

export function listenToAllSupportedEvents(rootContainerElement: EventTarget) {
if ((rootContainerElement: any)[listeningMarker]) {
// Performance optimization: don't iterate through events
// for the same portal container or root node more than once.
// TODO: once we remove the flag, we may be able to also
// remove some of the bookkeeping maps used for laziness.
return;
}
(rootContainerElement: any)[listeningMarker] = true;
allNativeEvents.forEach(domEventName => {
if (!nonDelegatedEvents.has(domEventName)) {
listenToNativeEvent(
export function listenToNativeEvent(
domEventName: DOMEventName,
isCapturePhaseListener: boolean,
target: EventTarget,
): void {
if (__DEV__) {
if (nonDelegatedEvents.has(domEventName) && !isCapturePhaseListener) {
console.error(
'Did not expect a listenToNativeEvent() call for "%s" in the bubble phase. ' +
'This is a bug in React. Please file an issue.',
domEventName,
false,
((rootContainerElement: any): Element),
null,
);
}
listenToNativeEvent(
domEventName,
true,
((rootContainerElement: any): Element),
null,
);
});
}

let eventSystemFlags = 0;
if (isCapturePhaseListener) {
eventSystemFlags |= IS_CAPTURE_PHASE;
}
addTrappedEventListener(
target,
domEventName,
eventSystemFlags,
isCapturePhaseListener,
);
}

export function listenToNativeEvent(
// This is only used by createEventHandle when the
// target is not a DOM element. E.g. window.
export function listenToNativeEventForNonManagedEventTarget(
domEventName: DOMEventName,
isCapturePhaseListener: boolean,
rootContainerElement: EventTarget,
targetElement: Element | null,
eventSystemFlags?: EventSystemFlags = 0,
target: EventTarget,
): void {
let target = rootContainerElement;

// selectionchange needs to be attached to the document
// otherwise it won't capture incoming events that are only
// triggered on the document directly.
if (
domEventName === 'selectionchange' &&
(rootContainerElement: any).nodeType !== DOCUMENT_NODE
) {
target = (rootContainerElement: any).ownerDocument;
}
// If the event can be delegated (or is capture phase), we can
// register it to the root container. Otherwise, we should
// register the event to the target element and mark it as
// a non-delegated event.
if (
targetElement !== null &&
!isCapturePhaseListener &&
nonDelegatedEvents.has(domEventName)
) {
// For all non-delegated events, apart from scroll, we attach
// their event listeners to the respective elements that their
// events fire on. That means we can skip this step, as event
// listener has already been added previously. However, we
// special case the scroll event because the reality is that any
// element can scroll.
// TODO: ideally, we'd eventually apply the same logic to all
// events from the nonDelegatedEvents list. Then we can remove
// this special case and use the same logic for all events.
if (domEventName !== 'scroll') {
return;
}
eventSystemFlags |= IS_NON_DELEGATED;
target = targetElement;
}
let eventSystemFlags = IS_EVENT_HANDLE_NON_MANAGED_NODE;
const listenerSet = getEventListenerSet(target);
const listenerSetKey = getListenerSetKey(
domEventName,
isCapturePhaseListener,
);
// If the listener entry is empty or we should upgrade, then
// we need to trap an event listener onto the target.
if (!listenerSet.has(listenerSetKey)) {
if (isCapturePhaseListener) {
eventSystemFlags |= IS_CAPTURE_PHASE;
Expand All @@ -408,6 +375,40 @@ export function listenToNativeEvent(
}
}

const listeningMarker =
'_reactListening' +
Math.random()
.toString(36)
.slice(2);

export function listenToAllSupportedEvents(rootContainerElement: EventTarget) {
if (!(rootContainerElement: any)[listeningMarker]) {
(rootContainerElement: any)[listeningMarker] = true;
allNativeEvents.forEach(domEventName => {
// We handle selectionchange separately because it
// doesn't bubble and needs to be on the document.
if (domEventName !== 'selectionchange') {
if (!nonDelegatedEvents.has(domEventName)) {
listenToNativeEvent(domEventName, false, rootContainerElement);
}
listenToNativeEvent(domEventName, true, rootContainerElement);
}
});
const ownerDocument =
(rootContainerElement: any).nodeType === DOCUMENT_NODE
? rootContainerElement
: (rootContainerElement: any).ownerDocument;
if (ownerDocument !== null) {
// The selectionchange event also needs deduplication
// but it is attached to the document.
if (!(ownerDocument: any)[listeningMarker]) {
(ownerDocument: any)[listeningMarker] = true;
listenToNativeEvent('selectionchange', false, ownerDocument);
}
}
}
}

function addTrappedEventListener(
targetContainer: EventTarget,
domEventName: DOMEventName,
Expand Down

0 comments on commit cddd747

Please sign in to comment.