Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refine event registration + event signatures #19244

Merged
merged 2 commits into from Jul 6, 2020
Merged

Refine event registration + event signatures #19244

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84b4b42
Refine event registration + event signatures
trueadm Jul 2, 2020
0709a3c
Address feedback
trueadm Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions packages/react-dom/src/client/ReactDOMComponent.js
View file
Open in desktop
Expand Up @@ -7,6 +7,8 @@
* @flow
*/

import type {ElementListenerMapEntry} from '../client/ReactDOMComponentTree';

import {
registrationNameDependencies,
possibleRegistrationNames,
Expand Down Expand Up @@ -82,7 +84,7 @@ import {
enableDeprecatedFlareAPI,
enableTrustedTypesIntegration,
} from 'shared/ReactFeatureFlags';
import {listenToEvent} from '../events/DOMModernPluginEventSystem';
import {listenToReactPropEvent} from '../events/DOMModernPluginEventSystem';
import {getEventListenerMap} from './ReactDOMComponentTree';

let didWarnInvalidHydration = false;
Expand Down Expand Up @@ -262,7 +264,7 @@ if (__DEV__) {

export function ensureListeningTo(
rootContainerInstance: Element | Node,
registrationName: string,
reactPropEvent: string,
): void {
// If we have a comment node, then use the parent node,
// which should be an element.
Expand All @@ -279,7 +281,10 @@ export function ensureListeningTo(
'ensureListeningTo(): received a container that was not an element node. ' +
'This is likely a bug in React.',
);
listenToEvent(registrationName, ((rootContainerElement: any): Element));
listenToReactPropEvent(
reactPropEvent,
((rootContainerElement: any): Element),
);
}

function getOwnerDocumentFromRootContainer(
Expand Down Expand Up @@ -1267,7 +1272,9 @@ export function listenToEventResponderEventTypes(
// existing passive event listener before we add the
// active event listener.
const passiveKey = targetEventType + '_passive';
const passiveItem = listenerMap.get(passiveKey);
const passiveItem = ((listenerMap.get(
passiveKey,
): any): ElementListenerMapEntry | void);
if (passiveItem !== undefined) {
removeTrappedEventListener(
document,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/client/ReactDOMComponentTree.js
View file
Open in desktop
Expand Up @@ -42,7 +42,7 @@ const internalEventHandlerListenersKey = '__reactListeners$' + randomKey;

export type ElementListenerMap = Map<
DOMTopLevelEventType | string,
ElementListenerMapEntry,
ElementListenerMapEntry | null,
>;

export type ElementListenerMapEntry = {
Expand Down
5 changes: 4 additions & 1 deletion packages/react-dom/src/client/ReactDOMEventHandle.js
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ import {ELEMENT_NODE} from '../shared/HTMLNodeType';
import {
listenToTopLevelEvent,
addEventTypeToDispatchConfig,
capturePhaseEvents,
} from '../events/DOMModernPluginEventSystem';

import {HostRoot, HostPortal} from 'react-reconciler/src/ReactWorkTags';
Expand Down Expand Up @@ -98,11 +99,13 @@ function registerEventOnNearestTargetContainer(
);
}
const listenerMap = getEventListenerMap(targetContainer);
const capture = capturePhaseEvents.has(topLevelType);
listenToTopLevelEvent(
topLevelType,
targetContainer,
listenerMap,
PLUGIN_EVENT_SYSTEM,
capture,
passive,
priority,
);
Expand Down Expand Up @@ -201,9 +204,9 @@ export function createEventHandle(
eventTarget,
listenerMap,
PLUGIN_EVENT_SYSTEM | IS_TARGET_PHASE_ONLY,
capture,
passive,
priority,
capture,
);
} else {
invariant(
Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
View file
Open in desktop
Expand Up @@ -81,7 +81,7 @@ import {
import {HostComponent, HostText} from 'react-reconciler/src/ReactWorkTags';
import {TOP_BEFORE_BLUR, TOP_AFTER_BLUR} from '../events/DOMTopLevelEventTypes';
import {
listenToEvent,
listenToReactPropEvent,
clearEventHandleListenersForTarget,
} from '../events/DOMModernPluginEventSystem';

Expand Down Expand Up @@ -1122,7 +1122,7 @@ export function makeOpaqueHydratingObject(
}

export function preparePortalMount(portalInstance: Instance): void {
listenToEvent('onMouseEnter', portalInstance);
listenToReactPropEvent('onMouseEnter', portalInstance);
}

export function prepareScopeUpdate(
Expand Down
37 changes: 25 additions & 12 deletions packages/react-dom/src/events/DOMModernPluginEventSystem.js
View file
Open in desktop
Expand Up @@ -30,6 +30,7 @@ import {
LEGACY_FB_SUPPORT,
IS_REPLAYED,
IS_TARGET_PHASE_ONLY,
IS_CAPTURE_PHASE,
} from './EventSystemFlags';

import {
Expand Down Expand Up @@ -301,9 +302,9 @@ export function listenToTopLevelEvent(
target: EventTarget,
listenerMap: ElementListenerMap,
eventSystemFlags: EventSystemFlags,
capture: boolean,
passive?: boolean,
priority?: EventPriority,
capture?: boolean,
): void {
// TOP_SELECTION_CHANGE needs to be attached to the document
// otherwise it won't capture incoming events that are only
Expand All @@ -312,12 +313,10 @@ export function listenToTopLevelEvent(
target = (target: any).ownerDocument || target;
listenerMap = getEventListenerMap(target);
}
capture =
capture === undefined ? capturePhaseEvents.has(topLevelType) : capture;
const listenerMapKey = getListenerMapKey(topLevelType, capture);
const listenerEntry: ElementListenerMapEntry | void = listenerMap.get(
const listenerEntry = ((listenerMap.get(
listenerMapKey,
);
): any): ElementListenerMapEntry | void);
const shouldUpgrade = shouldUpgradeListener(listenerEntry, passive);

// If the listener entry is empty or we should upgrade, then
Expand All @@ -333,6 +332,9 @@ export function listenToTopLevelEvent(
((listenerEntry: any): ElementListenerMapEntry).listener,
);
}
if (capture) {
eventSystemFlags |= IS_CAPTURE_PHASE;
}
const listener = addTrappedEventListener(
target,
topLevelType,
Expand All @@ -346,20 +348,31 @@ export function listenToTopLevelEvent(
}
}

export function listenToEvent(
registrationName: string,
export function listenToReactPropEvent(
reactPropEvent: string,
rootContainerElement: Element,
): void {
const listenerMap = getEventListenerMap(rootContainerElement);
const dependencies = registrationNameDependencies[registrationName];
// For optimization, let's check if we have the registration name
// on the rootContainerElement.
if (listenerMap.has(reactPropEvent)) {
return;
}
// Add the registration name to the map, so we can avoid processing
// this React prop event again.
listenerMap.set(reactPropEvent, null);
const dependencies = registrationNameDependencies[reactPropEvent];

for (let i = 0; i < dependencies.length; i++) {
const dependency = dependencies[i];
const capture = capturePhaseEvents.has(dependency);

listenToTopLevelEvent(
dependency,
rootContainerElement,
listenerMap,
PLUGIN_EVENT_SYSTEM,
capture,
);
}
}
Expand Down Expand Up @@ -619,7 +632,7 @@ function createDispatchQueueItem(
};
}

export function accumulateTwoPhaseListeners(
export function accumulatePhaseListeners(
trueadm marked this conversation as resolved.
Show resolved Hide resolved
targetFiber: Fiber | null,
dispatchQueue: DispatchQueue,
event: ReactSyntheticEvent,
Expand Down Expand Up @@ -896,6 +909,7 @@ export function accumulateEventTargetListeners(
dispatchQueue: DispatchQueue,
trueadm marked this conversation as resolved.
Show resolved Hide resolved
event: ReactSyntheticEvent,
currentTarget: EventTarget,
inCapturePhase: boolean,
): void {
const capturePhase: DispatchQueueItemPhase = [];
const bubblePhase: DispatchQueueItemPhase = [];
Expand All @@ -904,17 +918,16 @@ export function accumulateEventTargetListeners(
if (eventListeners !== null) {
const listenersArr = Array.from(eventListeners);
const targetType = ((event.type: any): DOMTopLevelEventType);
const isCapturePhase = (event: any).eventPhase === 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a significant change. Instead of reading this from the event, we now pass it from outside based on whether it's in a list. Can you please explain why this is a refactoring rather than a semantic change? And what does it accomplish, exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there are two things here to note:

  • This code is only used with createEventHandle logic, so this isn't that much of a significant change given we have no product code anywhere that uses this code path right now.
  • It avoids us having to rely on eventPhase which can change if you add a listener to a node and the listener triggers on it (which would mean eventPhase is 2, even if it were a capture phase). When we setup the listener we can add this information to the event flags as a detail, completely avoiding this check.


for (let i = 0; i < listenersArr.length; i++) {
const listener = listenersArr[i];
const {callback, capture, type} = listener;
if (type === targetType) {
if (isCapturePhase && capture) {
if (inCapturePhase && capture) {
capturePhase.push(
createDispatchQueueItemPhaseEntry(null, callback, currentTarget),
);
} else if (!isCapturePhase && !capture) {
} else if (!inCapturePhase && !capture) {
bubblePhase.push(
createDispatchQueueItemPhaseEntry(null, callback, currentTarget),
);
Expand Down
11 changes: 6 additions & 5 deletions packages/react-dom/src/events/EventSystemFlags.js
View file
Open in desktop
Expand Up @@ -12,8 +12,9 @@ export type EventSystemFlags = number;
export const PLUGIN_EVENT_SYSTEM = 1;
export const RESPONDER_EVENT_SYSTEM = 1 << 1;
export const IS_TARGET_PHASE_ONLY = 1 << 2;
export const IS_PASSIVE = 1 << 3;
export const PASSIVE_NOT_SUPPORTED = 1 << 4;
export const IS_REPLAYED = 1 << 5;
export const IS_FIRST_ANCESTOR = 1 << 6;
export const LEGACY_FB_SUPPORT = 1 << 7;
export const IS_CAPTURE_PHASE = 1 << 3;
export const IS_PASSIVE = 1 << 4;
export const PASSIVE_NOT_SUPPORTED = 1 << 5;
export const IS_REPLAYED = 1 << 6;
export const IS_FIRST_ANCESTOR = 1 << 7;
export const LEGACY_FB_SUPPORT = 1 << 8;
1 change: 1 addition & 0 deletions packages/react-dom/src/events/ReactDOMEventReplaying.js
View file
Open in desktop
Expand Up @@ -220,6 +220,7 @@ function trapReplayableEventForContainer(
((container: any): Element),
listenerMap,
PLUGIN_EVENT_SYSTEM,
false,
);
}

Expand Down
6 changes: 3 additions & 3 deletions packages/react-dom/src/events/plugins/ModernBeforeInputEventPlugin.js
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ import {
} from '../FallbackCompositionState';
import SyntheticCompositionEvent from '../SyntheticCompositionEvent';
import SyntheticInputEvent from '../SyntheticInputEvent';
import {accumulateTwoPhaseListeners} from '../DOMModernPluginEventSystem';
import {accumulatePhaseListeners} from '../DOMModernPluginEventSystem';

const END_KEYCODES = [9, 13, 27, 32]; // Tab, Return, Esc, Space
const START_KEYCODE = 229;
Expand Down Expand Up @@ -241,7 +241,7 @@ function extractCompositionEvent(
nativeEvent,
nativeEventTarget,
);
accumulateTwoPhaseListeners(targetInst, dispatchQueue, event);
accumulatePhaseListeners(targetInst, dispatchQueue, event);

if (fallbackData) {
// Inject data generated from fallback path into the synthetic event.
Expand Down Expand Up @@ -411,7 +411,7 @@ function extractBeforeInputEvent(
nativeEvent,
nativeEventTarget,
);
accumulateTwoPhaseListeners(targetInst, dispatchQueue, event);
accumulatePhaseListeners(targetInst, dispatchQueue, event);
event.data = chars;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/events/plugins/ModernChangeEventPlugin.js
View file
Open in desktop
Expand Up @@ -37,7 +37,7 @@ import {disableInputAttributeSyncing} from 'shared/ReactFeatureFlags';
import {batchedUpdates} from '../ReactDOMUpdateBatching';
import {
dispatchEventsInBatch,
accumulateTwoPhaseListeners,
accumulatePhaseListeners,
} from '../DOMModernPluginEventSystem';

function registerEvents() {
Expand All @@ -63,7 +63,7 @@ function createAndAccumulateChangeEvent(
event.type = 'change';
// Flag this event loop as needing state restore.
enqueueStateRestore(((target: any): Node));
accumulateTwoPhaseListeners(inst, dispatchQueue, event);
accumulatePhaseListeners(inst, dispatchQueue, event);
}
/**
* For IE shims
Expand Down
48 changes: 6 additions & 42 deletions packages/react-dom/src/events/plugins/ModernSelectEventPlugin.js
View file
Open in desktop
Expand Up @@ -29,11 +29,7 @@ import {
} from '../../client/ReactDOMComponentTree';
import {hasSelectionCapabilities} from '../../client/ReactInputSelection';
import {DOCUMENT_NODE} from '../../shared/HTMLNodeType';
import {
accumulateTwoPhaseListeners,
getListenerMapKey,
capturePhaseEvents,
} from '../DOMModernPluginEventSystem';
import {accumulatePhaseListeners} from '../DOMModernPluginEventSystem';

const skipSelectionChangeEvent =
canUseDOM && 'documentMode' in document && document.documentMode <= 11;
Expand Down Expand Up @@ -140,38 +136,8 @@ function constructSelectEvent(dispatchQueue, nativeEvent, nativeEventTarget) {
syntheticEvent.type = 'select';
syntheticEvent.target = activeElement;

accumulateTwoPhaseListeners(
activeElementInst,
dispatchQueue,
syntheticEvent,
);
}
}

function isListeningToEvents(
events: Array<string>,
mountAt: Document | Element,
): boolean {
const listenerMap = getEventListenerMap(mountAt);
for (let i = 0; i < events.length; i++) {
const event = events[i];
const capture = capturePhaseEvents.has(event);
const listenerMapKey = getListenerMapKey(event, capture);
if (!listenerMap.has(listenerMapKey)) {
return false;
}
accumulatePhaseListeners(activeElementInst, dispatchQueue, syntheticEvent);
}
return true;
}

function isListeningToEvent(
registrationName: string,
mountAt: Document | Element,
): boolean {
const listenerMap = getEventListenerMap(mountAt);
const capture = capturePhaseEvents.has(registrationName);
const listenerMapKey = getListenerMapKey(registrationName, capture);
return listenerMap.has(listenerMapKey);
}

/**
Expand All @@ -197,19 +163,17 @@ function extractEvents(
eventSystemFlags,
targetContainer,
) {
const doc = getEventTargetDocument(nativeEventTarget);
const eventListenerMap = getEventListenerMap(targetContainer);
// Track whether all listeners exists for this plugin. If none exist, we do
// not extract events. See #3639.
if (
// We only listen to TOP_SELECTION_CHANGE on the document, never the
// root.
!isListeningToEvent(TOP_SELECTION_CHANGE, doc) ||
// If we are handling TOP_SELECTION_CHANGE, then we don't need to
// check for the other dependencies, as TOP_SELECTION_CHANGE is only
// event attached from the onChange plugin and we don't expose an
// onSelectionChange event from React.
(topLevelType !== TOP_SELECTION_CHANGE &&
!isListeningToEvents(rootTargetDependencies, targetContainer))
topLevelType !== TOP_SELECTION_CHANGE &&
!eventListenerMap.has('onSelect') &&
!eventListenerMap.has('onSelectCapture')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this check is equivalent? It's not obvious to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than going through all the dependencies one by one, we can instead check if we've registered onSelect or onSelectCapture. The dependencies are both the same, as we listen to the same events for both onSelect and onSelectCapture.

) {
return;
}
Expand Down