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

Add infrastructure for passive/non-passive event support for future API exploration #15036

Merged
merged 20 commits into from
Mar 15, 2019
Merged

Add infrastructure for passive/non-passive event support for future API exploration #15036

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
858608b
Add infrastructure for passive/non-passive event support for future e…
trueadm Mar 6, 2019
0ae3ba3
More Flow fixes
trueadm Mar 6, 2019
3bd6176
Add fallback to Map if there is no WeakMap
trueadm Mar 6, 2019
06f2c3b
Fix prettier
trueadm Mar 6, 2019
bbae02d
Update comment
trueadm Mar 6, 2019
68934d8
Add isLegacy flag to EventPlugins
trueadm Mar 13, 2019
37224ea
Handle fallback for no passive support
trueadm Mar 13, 2019
c683859
Fixed typo + improve code size
trueadm Mar 13, 2019
e7e742f
Changed to enum for passive event types and addressed PR feedback
trueadm Mar 13, 2019
ccf3884
Added comment to describe intent
trueadm Mar 13, 2019
c9fc178
use canUseDOM
trueadm Mar 13, 2019
5d5b972
Cleaned up some logic and reduces more bytes
trueadm Mar 13, 2019
e186b09
Fix typo
trueadm Mar 14, 2019
1dff1fc
Address nits
trueadm Mar 14, 2019
8f79ee8
Addressed feedback and took a different approach with flags
trueadm Mar 14, 2019
159280c
Merge branch 'master' into passive-event-listening
trueadm Mar 14, 2019
2acacf8
Wrap function in flags
trueadm Mar 14, 2019
3b67255
Remove more code that is not needed anymore
trueadm Mar 14, 2019
5b044cf
Adds flag to another location
trueadm Mar 14, 2019
8b34efa
Address feedback on addEventListener
trueadm Mar 15, 2019
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
4 changes: 4 additions & 0 deletions packages/events/EventPluginHub.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ function extractEvents(
targetInst: null | Fiber,
nativeEvent: AnyNativeEvent,
nativeEventTarget: EventTarget,
passive: null | boolean,
trueadm marked this conversation as resolved.
Show resolved Hide resolved
): Array<ReactSyntheticEvent> | ReactSyntheticEvent | null {
let events = null;
for (let i = 0; i < plugins.length; i++) {
Expand All @@ -174,6 +175,7 @@ function extractEvents(
targetInst,
nativeEvent,
nativeEventTarget,
passive,
);
if (extractedEvents) {
events = accumulateInto(events, extractedEvents);
Expand Down Expand Up @@ -214,12 +216,14 @@ export function runExtractedEventsInBatch(
targetInst: null | Fiber,
nativeEvent: AnyNativeEvent,
nativeEventTarget: EventTarget,
passive: null | boolean,
) {
const events = extractEvents(
topLevelType,
targetInst,
nativeEvent,
nativeEventTarget,
passive,
);
runEventsInBatch(events);
}
1 change: 1 addition & 0 deletions packages/events/PluginModuleType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export type PluginModule<NativeEvent> = {
targetInst: null | Fiber,
nativeTarget: NativeEvent,
nativeEventTarget: EventTarget,
passive?: null | boolean,
) => ?ReactSyntheticEvent,
tapMoveThreshold?: number,
};
8 changes: 4 additions & 4 deletions packages/events/ReactGenericBatching.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import {
let _batchedUpdatesImpl = function(fn, bookkeeping) {
return fn(bookkeeping);
};
let _interactiveUpdatesImpl = function(fn, a, b) {
return fn(a, b);
let _interactiveUpdatesImpl = function(fn, a, b, c) {
return fn(a, b, c);
};
let _flushInteractiveUpdatesImpl = function() {};

Expand Down Expand Up @@ -52,8 +52,8 @@ export function batchedUpdates(fn, bookkeeping) {
}
}

export function interactiveUpdates(fn, a, b) {
return _interactiveUpdatesImpl(fn, a, b);
export function interactiveUpdates(fn, a, b, c) {
return _interactiveUpdatesImpl(fn, a, b, c);
}

export function flushInteractiveUpdates() {
Expand Down
8 changes: 4 additions & 4 deletions packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,15 +331,15 @@ describe('ReactBrowserEventEmitter', () => {

it('should listen to events only once', () => {
spyOnDevAndProd(EventTarget.prototype, 'addEventListener');
ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document);
ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document);
ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document, true);
ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document, true);
expect(EventTarget.prototype.addEventListener).toHaveBeenCalledTimes(1);
});

it('should work with event plugins without dependencies', () => {
spyOnDevAndProd(EventTarget.prototype, 'addEventListener');

ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document);
ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document, true);

expect(EventTarget.prototype.addEventListener.calls.argsFor(0)[0]).toBe(
'click',
Expand All @@ -349,7 +349,7 @@ describe('ReactBrowserEventEmitter', () => {
it('should work with event plugins with dependencies', () => {
spyOnDevAndProd(EventTarget.prototype, 'addEventListener');

ReactBrowserEventEmitter.listenTo(ON_CHANGE_KEY, document);
ReactBrowserEventEmitter.listenTo(ON_CHANGE_KEY, document, true);

const setEventListeners = [];
const listenCalls = EventTarget.prototype.addEventListener.calls.allArgs();
Expand Down
51 changes: 27 additions & 24 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 @@ -253,14 +253,17 @@ if (__DEV__) {
};
}

function ensureListeningTo(rootContainerElement, registrationName) {
function ensureListeningTo(
rootContainerElement: Element | Node,
registrationName: string,
): void {
const isDocumentOrFragment =
rootContainerElement.nodeType === DOCUMENT_NODE ||
rootContainerElement.nodeType === DOCUMENT_FRAGMENT_NODE;
const doc = isDocumentOrFragment
? rootContainerElement
: rootContainerElement.ownerDocument;
listenTo(registrationName, doc);
listenTo(registrationName, doc, true /* isLegacy */);
}

function getOwnerDocumentFromRootContainer(
Expand Down Expand Up @@ -494,41 +497,41 @@ export function setInitialProperties(
switch (tag) {
case 'iframe':
case 'object':
trapBubbledEvent(TOP_LOAD, domElement);
trapBubbledEvent(TOP_LOAD, domElement, true);
props = rawProps;
break;
case 'video':
case 'audio':
// Create listener for each media event
for (let i = 0; i < mediaEventTypes.length; i++) {
trapBubbledEvent(mediaEventTypes[i], domElement);
trapBubbledEvent(mediaEventTypes[i], domElement, true);
}
props = rawProps;
break;
case 'source':
trapBubbledEvent(TOP_ERROR, domElement);
trapBubbledEvent(TOP_ERROR, domElement, true);
props = rawProps;
break;
case 'img':
case 'image':
case 'link':
trapBubbledEvent(TOP_ERROR, domElement);
trapBubbledEvent(TOP_LOAD, domElement);
trapBubbledEvent(TOP_ERROR, domElement, true);
trapBubbledEvent(TOP_LOAD, domElement, true);
props = rawProps;
break;
case 'form':
trapBubbledEvent(TOP_RESET, domElement);
trapBubbledEvent(TOP_SUBMIT, domElement);
trapBubbledEvent(TOP_RESET, domElement, true);
trapBubbledEvent(TOP_SUBMIT, domElement, true);
props = rawProps;
break;
case 'details':
trapBubbledEvent(TOP_TOGGLE, domElement);
trapBubbledEvent(TOP_TOGGLE, domElement, true);
props = rawProps;
break;
case 'input':
ReactDOMInputInitWrapperState(domElement, rawProps);
props = ReactDOMInputGetHostProps(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapBubbledEvent(TOP_INVALID, domElement, true);
Copy link
Collaborator

@gaearon gaearon Mar 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Per our discussion, it seems like these code paths will never "want" to get two listeners attached/invoked. So instead of branching deeply and passing an argument through, let's fork the innermost implementation and call the fork from the new code when we want to. That will also likely let us DCE more based on a feature flag.

// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
Expand All @@ -540,15 +543,15 @@ export function setInitialProperties(
case 'select':
ReactDOMSelectInitWrapperState(domElement, rawProps);
props = ReactDOMSelectGetHostProps(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapBubbledEvent(TOP_INVALID, domElement, true);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
break;
case 'textarea':
ReactDOMTextareaInitWrapperState(domElement, rawProps);
props = ReactDOMTextareaGetHostProps(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapBubbledEvent(TOP_INVALID, domElement, true);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
Expand Down Expand Up @@ -888,34 +891,34 @@ export function diffHydratedProperties(
switch (tag) {
case 'iframe':
case 'object':
trapBubbledEvent(TOP_LOAD, domElement);
trapBubbledEvent(TOP_LOAD, domElement, true);
break;
case 'video':
case 'audio':
// Create listener for each media event
for (let i = 0; i < mediaEventTypes.length; i++) {
trapBubbledEvent(mediaEventTypes[i], domElement);
trapBubbledEvent(mediaEventTypes[i], domElement, true);
}
break;
case 'source':
trapBubbledEvent(TOP_ERROR, domElement);
trapBubbledEvent(TOP_ERROR, domElement, true);
break;
case 'img':
case 'image':
case 'link':
trapBubbledEvent(TOP_ERROR, domElement);
trapBubbledEvent(TOP_LOAD, domElement);
trapBubbledEvent(TOP_ERROR, domElement, true);
trapBubbledEvent(TOP_LOAD, domElement, true);
break;
case 'form':
trapBubbledEvent(TOP_RESET, domElement);
trapBubbledEvent(TOP_SUBMIT, domElement);
trapBubbledEvent(TOP_RESET, domElement, true);
trapBubbledEvent(TOP_SUBMIT, domElement, true);
break;
case 'details':
trapBubbledEvent(TOP_TOGGLE, domElement);
trapBubbledEvent(TOP_TOGGLE, domElement, true);
break;
case 'input':
ReactDOMInputInitWrapperState(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapBubbledEvent(TOP_INVALID, domElement, true);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
Expand All @@ -925,14 +928,14 @@ export function diffHydratedProperties(
break;
case 'select':
ReactDOMSelectInitWrapperState(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapBubbledEvent(TOP_INVALID, domElement, true);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
break;
case 'textarea':
ReactDOMTextareaInitWrapperState(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapBubbledEvent(TOP_INVALID, domElement, true);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
Expand Down
24 changes: 20 additions & 4 deletions packages/react-dom/src/events/EventListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,33 @@
*/

export function addEventBubbleListener(
element: Document | Element,
element: Document | Element | Node,
eventType: string,
listener: Function,
isPassive: boolean | null,
): void {
element.addEventListener(eventType, listener, false);
if (isPassive === null) {
element.addEventListener(eventType, listener, false);
} else {
element.addEventListener(eventType, listener, {
passive: isPassive,
capture: false,
});
trueadm marked this conversation as resolved.
Show resolved Hide resolved
}
}

export function addEventCaptureListener(
element: Document | Element,
element: Document | Element | Node,
eventType: string,
listener: Function,
isPassive: boolean | null,
): void {
element.addEventListener(eventType, listener, true);
if (isPassive === null) {
element.addEventListener(eventType, listener, true);
} else {
element.addEventListener(eventType, listener, {
passive: isPassive,
capture: true,
});
}
}
Loading