Skip to content
New issue

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

#

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

Already on GitHub? # to your account

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 all commits
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions packages/react-dom/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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 @@ -892,10 +905,11 @@ export function accumulateEnterLeaveListeners(
}
}

export function accumulateEventTargetListeners(
export function accumulateEventHandleTargetListeners(
dispatchQueue: DispatchQueue,
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ function trapReplayableEventForContainer(
((container: any): Element),
listenerMap,
PLUGIN_EVENT_SYSTEM,
false,
);
}

Expand Down
42 changes: 5 additions & 37 deletions packages/react-dom/src/events/plugins/ModernSelectEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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 {accumulateTwoPhaseListeners} from '../DOMModernPluginEventSystem';

const skipSelectionChangeEvent =
canUseDOM && 'documentMode' in document && document.documentMode <= 11;
Expand Down Expand Up @@ -148,32 +144,6 @@ function constructSelectEvent(dispatchQueue, nativeEvent, nativeEventTarget) {
}
}

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;
}
}
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);
}

/**
* This plugin creates an `onSelect` event that normalizes select events
* across form elements.
Expand All @@ -197,19 +167,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
12 changes: 9 additions & 3 deletions packages/react-dom/src/events/plugins/ModernSimpleEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ import {
} from '../DOMEventProperties';
import {
accumulateTwoPhaseListeners,
accumulateEventTargetListeners,
accumulateEventHandleTargetListeners,
} from '../DOMModernPluginEventSystem';
import {IS_TARGET_PHASE_ONLY} from '../EventSystemFlags';

import SyntheticAnimationEvent from '../SyntheticAnimationEvent';
import SyntheticClipboardEvent from '../SyntheticClipboardEvent';
import SyntheticFocusEvent from '../SyntheticFocusEvent';
Expand All @@ -40,6 +39,7 @@ import SyntheticTransitionEvent from '../SyntheticTransitionEvent';
import SyntheticUIEvent from '../SyntheticUIEvent';
import SyntheticWheelEvent from '../SyntheticWheelEvent';
import getEventCharCode from '../getEventCharCode';
import {IS_CAPTURE_PHASE} from '../EventSystemFlags';

import {enableCreateEventHandleAPI} from 'shared/ReactFeatureFlags';

Expand Down Expand Up @@ -158,7 +158,13 @@ function extractEvents(
eventSystemFlags & IS_TARGET_PHASE_ONLY &&
targetContainer != null
) {
accumulateEventTargetListeners(dispatchQueue, event, targetContainer);
const inCapturePhase = (eventSystemFlags & IS_CAPTURE_PHASE) !== 0;
accumulateEventHandleTargetListeners(
dispatchQueue,
event,
targetContainer,
inCapturePhase,
);
} else {
accumulateTwoPhaseListeners(targetInst, dispatchQueue, event, true);
}
Expand Down
9 changes: 5 additions & 4 deletions packages/react-interactions/events/src/dom/create-event-handle/Focus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ function handleGlobalFocusVisibleEvent(
}

const passiveObject = {passive: true};
const passiveCaptureObject = {capture: true, passive: false};

function handleFocusVisibleTargetEvent(
type: string,
Expand Down Expand Up @@ -242,8 +243,8 @@ export function useFocus(
): void {
// Setup controlled state for this useFocus hook
const stateRef = useRef({isFocused: false, isFocusVisible: false});
const focusHandle = useEvent('focus', passiveObject);
const blurHandle = useEvent('blur', passiveObject);
const focusHandle = useEvent('focus', passiveCaptureObject);
const blurHandle = useEvent('blur', passiveCaptureObject);
const focusVisibleHandles = useFocusVisibleInputHandles();

useEffect(() => {
Expand Down Expand Up @@ -329,8 +330,8 @@ export function useFocusWithin(
) {
// Setup controlled state for this useFocus hook
const stateRef = useRef({isFocused: false, isFocusVisible: false});
const focusHandle = useEvent('focus', passiveObject);
const blurHandle = useEvent('blur', passiveObject);
const focusHandle = useEvent('focus', passiveCaptureObject);
const blurHandle = useEvent('blur', passiveCaptureObject);
const afterBlurHandle = useEvent('afterblur', passiveObject);
const beforeBlurHandle = useEvent('beforeblur', passiveObject);
const focusVisibleHandles = useFocusVisibleInputHandles();
Expand Down