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

DO NOT MERGE: Investigate switching out event delegation for direct attachment (mostly) #11550

Closed
wants to merge 4 commits into from
Closed

DO NOT MERGE: Investigate switching out event delegation for direct attachment (mostly) #11550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fcbfbd
Switch out event delegation for most events
nhunzaker Nov 14, 2017
fa2501f
Copy over production builds to fixtures during build
nhunzaker Nov 14, 2017
ca75e2e
Remove unused imports in ReactDOMFiberComponent
nhunzaker Nov 14, 2017
cd44f7a
Revert ReactBrowserEventEmitter-test.js
nhunzaker Nov 14, 2017
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: 2 additions & 2 deletions fixtures/dom/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ coverage

# production
build
public/react.development.js
public/react-dom.development.js
public/react.*.js
public/react-dom.*.js

# misc
.DS_Store
Expand Down
3 changes: 2 additions & 1 deletion fixtures/dom/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
},
"scripts": {
"start": "react-scripts start",
"prestart": "cp ../../build/dist/{react,react-dom}.development.js public/",
"prestart": "cp ../../build/dist/{react,react-dom}.{development,production.min}.js public/",
"prebuild": "cp ../../build/dist/{react,react-dom}.{development,production.min}.js public/",
"build": "react-scripts build && cp build/index.html build/200.html",
"test": "react-scripts test --env=jsdom",
"eject": "react-scripts eject"
Expand Down
156 changes: 11 additions & 145 deletions packages/react-dom/src/client/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ import * as ReactDOMFiberTextarea from './ReactDOMFiberTextarea';
import * as inputValueTracking from './inputValueTracking';
import setInnerHTML from './setInnerHTML';
import setTextContent from './setTextContent';
import {listenTo, trapBubbledEvent} from '../events/ReactBrowserEventEmitter';
import {listenTo} from '../events/ReactBrowserEventEmitter';
import * as CSSPropertyOperations from '../shared/CSSPropertyOperations';
import {Namespaces, getIntrinsicNamespace} from '../shared/DOMNamespaces';
import {getPropertyInfo, shouldSetAttribute} from '../shared/DOMProperty';
import assertValidProps from '../shared/assertValidProps';
import {DOCUMENT_NODE, DOCUMENT_FRAGMENT_NODE} from '../shared/HTMLNodeType';
import {DOCUMENT_NODE} from '../shared/HTMLNodeType';
import isCustomComponent from '../shared/isCustomComponent';
import {validateProperties as validateARIAProperties} from '../shared/ReactDOMInvalidARIAHook';
import {validateProperties as validateInputProperties} from '../shared/ReactDOMNullInputValuePropHook';
Expand Down Expand Up @@ -188,16 +188,6 @@ if (__DEV__) {
};
}

function ensureListeningTo(rootContainerElement, registrationName) {
var isDocumentOrFragment =
rootContainerElement.nodeType === DOCUMENT_NODE ||
rootContainerElement.nodeType === DOCUMENT_FRAGMENT_NODE;
var doc = isDocumentOrFragment
? rootContainerElement
: rootContainerElement.ownerDocument;
listenTo(registrationName, doc);
}

function getOwnerDocumentFromRootContainer(
rootContainerElement: Element | Document,
): Document {
Expand All @@ -206,47 +196,6 @@ function getOwnerDocumentFromRootContainer(
: rootContainerElement.ownerDocument;
}

// There are so many media events, it makes sense to just
// maintain a list rather than create a `trapBubbledEvent` for each
var mediaEvents = {
topAbort: 'abort',
topCanPlay: 'canplay',
topCanPlayThrough: 'canplaythrough',
topDurationChange: 'durationchange',
topEmptied: 'emptied',
topEncrypted: 'encrypted',
topEnded: 'ended',
topError: 'error',
topLoadedData: 'loadeddata',
topLoadedMetadata: 'loadedmetadata',
topLoadStart: 'loadstart',
topPause: 'pause',
topPlay: 'play',
topPlaying: 'playing',
topProgress: 'progress',
topRateChange: 'ratechange',
topSeeked: 'seeked',
topSeeking: 'seeking',
topStalled: 'stalled',
topSuspend: 'suspend',
topTimeUpdate: 'timeupdate',
topVolumeChange: 'volumechange',
topWaiting: 'waiting',
};

function trapClickOnNonInteractiveElement(node: HTMLElement) {
// Mobile Safari does not fire properly bubble click events on
// non-interactive elements, which means delegated click listeners do not
// fire. The workaround for this bug involves attaching an empty click
// listener on the target node.
// http://www.quirksmode.org/blog/archives/2010/09/click_event_del.html
// Just set it using the onclick property so that we don't have to manage any
// bookkeeping for it. Not sure if we need to clear it when the listener is
// removed.
// TODO: Only do this for the relevant Safaris maybe?
node.onclick = emptyFunction;
}

function setInitialDOMProperties(
tag: string,
domElement: Element,
Expand Down Expand Up @@ -300,7 +249,7 @@ function setInitialDOMProperties(
if (__DEV__ && typeof nextProp !== 'function') {
warnForInvalidEventListener(propKey, nextProp);
}
ensureListeningTo(rootContainerElement, propKey);
listenTo(propKey, domElement, rootContainerElement);
}
} else if (isCustomComponentTag) {
DOMPropertyOperations.setValueForAttribute(domElement, propKey, nextProp);
Expand Down Expand Up @@ -455,47 +404,12 @@ export function setInitialProperties(
// TODO: Make sure that we check isMounted before firing any of these events.
var props: Object;
switch (tag) {
case 'iframe':
case 'object':
trapBubbledEvent('topLoad', 'load', domElement);
props = rawProps;
break;
case 'video':
case 'audio':
// Create listener for each media event
for (var event in mediaEvents) {
if (mediaEvents.hasOwnProperty(event)) {
trapBubbledEvent(event, mediaEvents[event], domElement);
}
}
props = rawProps;
break;
case 'source':
trapBubbledEvent('topError', 'error', domElement);
props = rawProps;
break;
case 'img':
case 'image':
trapBubbledEvent('topError', 'error', domElement);
trapBubbledEvent('topLoad', 'load', domElement);
props = rawProps;
break;
case 'form':
trapBubbledEvent('topReset', 'reset', domElement);
trapBubbledEvent('topSubmit', 'submit', domElement);
props = rawProps;
break;
case 'details':
trapBubbledEvent('topToggle', 'toggle', domElement);
props = rawProps;
break;
case 'input':
ReactDOMFiberInput.initWrapperState(domElement, rawProps);
props = ReactDOMFiberInput.getHostProps(domElement, rawProps);
trapBubbledEvent('topInvalid', 'invalid', domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
listenTo('onChange', domElement, rootContainerElement);
break;
case 'option':
ReactDOMFiberOption.validateProps(domElement, rawProps);
Expand All @@ -504,18 +418,16 @@ export function setInitialProperties(
case 'select':
ReactDOMFiberSelect.initWrapperState(domElement, rawProps);
props = ReactDOMFiberSelect.getHostProps(domElement, rawProps);
trapBubbledEvent('topInvalid', 'invalid', domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
listenTo('onChange', domElement);
break;
case 'textarea':
ReactDOMFiberTextarea.initWrapperState(domElement, rawProps);
props = ReactDOMFiberTextarea.getHostProps(domElement, rawProps);
trapBubbledEvent('topInvalid', 'invalid', domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
listenTo('onChange', domElement);
break;
default:
props = rawProps;
Expand Down Expand Up @@ -551,10 +463,6 @@ export function setInitialProperties(
ReactDOMFiberSelect.postMountWrapper(domElement, rawProps);
break;
default:
if (typeof props.onClick === 'function') {
// TODO: This cast may not be sound for SVG, MathML or custom elements.
trapClickOnNonInteractiveElement(((domElement: any): HTMLElement));
}
break;
}
}
Expand Down Expand Up @@ -599,13 +507,6 @@ export function diffProperties(
default:
lastProps = lastRawProps;
nextProps = nextRawProps;
if (
typeof lastProps.onClick !== 'function' &&
typeof nextProps.onClick === 'function'
) {
// TODO: This cast may not be sound for SVG, MathML or custom elements.
trapClickOnNonInteractiveElement(((domElement: any): HTMLElement));
}
break;
}

Expand Down Expand Up @@ -736,7 +637,7 @@ export function diffProperties(
if (__DEV__ && typeof nextProp !== 'function') {
warnForInvalidEventListener(propKey, nextProp);
}
ensureListeningTo(rootContainerElement, propKey);
listenTo(propKey, domElement, rootContainerElement);
}
if (!updatePayload && lastProp !== nextProp) {
// This is a special case. If any listener updates we need to ensure
Expand Down Expand Up @@ -823,57 +724,26 @@ export function diffHydratedProperties(

// TODO: Make sure that we check isMounted before firing any of these events.
switch (tag) {
case 'iframe':
case 'object':
trapBubbledEvent('topLoad', 'load', domElement);
break;
case 'video':
case 'audio':
// Create listener for each media event
for (var event in mediaEvents) {
if (mediaEvents.hasOwnProperty(event)) {
trapBubbledEvent(event, mediaEvents[event], domElement);
}
}
break;
case 'source':
trapBubbledEvent('topError', 'error', domElement);
break;
case 'img':
case 'image':
trapBubbledEvent('topError', 'error', domElement);
trapBubbledEvent('topLoad', 'load', domElement);
break;
case 'form':
trapBubbledEvent('topReset', 'reset', domElement);
trapBubbledEvent('topSubmit', 'submit', domElement);
Copy link
Contributor

Choose a reason for hiding this comment

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

for these, is there a reason why we were calling them out specifically here vs in the ensureListeningTo above? Do they need to probably be attached like onChange now?

Copy link
Contributor Author

@nhunzaker nhunzaker Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I can tell, this is just a pre-emptive attachment because ensureListeningTo attaches on the owner document, not the DOM element. We need these here because they don't bubble, so there must be a listener on the element for the synthetic event system to know about it.

I think there is a change we could send to master here. We can attach reset, submit, load, etc... directly on the element. I do not think the savings is substantial because these events are almost only ever added to these elements (when was the last time you attached onError on a div or button?)

I'm going to check to see if we still need onChange to attach here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☝️ edited with some clarification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it consistent in 68b42c2

Copy link
Contributor

Choose a reason for hiding this comment

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

ah so since everything is being attached directly there is no need to handle it here anymore 👍 right

break;
case 'details':
trapBubbledEvent('topToggle', 'toggle', domElement);
break;
case 'input':
ReactDOMFiberInput.initWrapperState(domElement, rawProps);
trapBubbledEvent('topInvalid', 'invalid', domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
listenTo('onChange', rootContainerElement);
break;
case 'option':
ReactDOMFiberOption.validateProps(domElement, rawProps);
break;
case 'select':
ReactDOMFiberSelect.initWrapperState(domElement, rawProps);
trapBubbledEvent('topInvalid', 'invalid', domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
listenTo('onChange', rootContainerElement);
break;
case 'textarea':
ReactDOMFiberTextarea.initWrapperState(domElement, rawProps);
trapBubbledEvent('topInvalid', 'invalid', domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
listenTo('onChange', rootContainerElement);
break;
}

Expand Down Expand Up @@ -940,7 +810,7 @@ export function diffHydratedProperties(
if (__DEV__ && typeof nextProp !== 'function') {
warnForInvalidEventListener(propKey, nextProp);
}
ensureListeningTo(rootContainerElement, propKey);
listenTo(propKey, domElement);
}
} else if (__DEV__) {
// Validate that the properties correspond to their expected values.
Expand Down Expand Up @@ -1052,10 +922,6 @@ export function diffHydratedProperties(
// TODO: Consider not doing this for input and textarea.
break;
default:
if (typeof rawProps.onClick === 'function') {
// TODO: This cast may not be sound for SVG, MathML or custom elements.
trapClickOnNonInteractiveElement(((domElement: any): HTMLElement));
}
break;
}

Expand Down
29 changes: 27 additions & 2 deletions packages/react-dom/src/events/ReactBrowserEventEmitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,31 @@ import {
trapBubbledEvent,
trapCapturedEvent,
} from './ReactDOMEventListener';
import {DOCUMENT_NODE, DOCUMENT_FRAGMENT_NODE} from '../shared/HTMLNodeType';
import isEventSupported from './isEventSupported';
import BrowserEventConstants from './BrowserEventConstants';

export * from 'events/ReactEventEmitterMixin';

var {topLevelTypes} = BrowserEventConstants;

const forceDelegation = {
topMouseOver: 1,
topMouseOut: 2,
topMouseEnter: 3,
topMouseLeave: 4,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

why do these need delegation? I wonder what the likelihood of this growing annoyingly is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this so that the EnterLeavePlugin event plugin works with elements React isn't watching.

Copy link
Contributor

Choose a reason for hiding this comment

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

blah that's annoying, this should need to include topMouseEnter and topMouseLeave, at the moment we don't support the native event (we always polyfill it) and I think even if we did it wouldn't be needed, since those events always originate from the target element (react owned) whereas the other events originate from the relatedTarget, which may be not be react-owned

Copy link
Contributor

Choose a reason for hiding this comment

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

also did we end up some place on using the native event vs continuing to polyfill this forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Still I can't help but wonder if there's work that could be done here to do the attachment local to the event plugin. It's a bummer that ReactBrowserEventEmitter and EnterLeavePlugin are coupled like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a similar problem for change events on inputs. We have to attach change events to controlled inputs even if there isn't an event listener. This is so that controlled inputs that don't have readOnly prevent value changes on user input.

IMO, we should warn users when they don't have a change event and tell them to use readOnly, but we might already do that. Or, at the very least, we should attach the event listeners inside of the input wrappers (input, select, and textarea).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also did we end up some place on using the native event vs continuing to polyfill this forever?

Let's give it a look. It looks like we might be able to use the native event now, but the support matrix for mouseleave on MDN is incomplete.

We should do this research, kill the event plugin if we can, which I think would also let us get rid of some tree traversal methods specific to this event.

At the very least we should update all of those question marks on MDN.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did the basic work checking for support, and we definitely can use the native event, the problem is Portals now.

naive approach: #10247
hack with portal support: #10269

In terms of moving attachment to the plugin, there used to be (still are?) hooks on the plugins that would get called for this sort of thing, something like willAttachListener they may have been removed tho since i can't seem to find them...


function documentForRoot(rootContainerElement, registrationName) {
var isDocumentOrFragment =
rootContainerElement.nodeType === DOCUMENT_NODE ||
rootContainerElement.nodeType === DOCUMENT_FRAGMENT_NODE;

return isDocumentOrFragment
? rootContainerElement
: rootContainerElement.ownerDocument;
}

/**
* Summary of `ReactBrowserEventEmitter` event handling:
*
Expand Down Expand Up @@ -115,15 +133,22 @@ function getListeningForDocument(mountAt) {
* @param {string} registrationName Name of listener (e.g. `onClick`).
* @param {object} contentDocumentHandle Document which owns the container
*/
export function listenTo(registrationName, contentDocumentHandle) {
export function listenTo(registrationName, contentDocumentHandle, root) {
var mountAt = contentDocumentHandle;
var isListening = getListeningForDocument(mountAt);
var dependencies = registrationNameDependencies[registrationName];

for (var i = 0; i < dependencies.length; i++) {
var dependency = dependencies[i];

if (!(isListening.hasOwnProperty(dependency) && isListening[dependency])) {
if (dependency === 'topWheel') {
if (forceDelegation.hasOwnProperty(dependency)) {
trapBubbledEvent(
dependency,
topLevelTypes[dependency],
documentForRoot(root),
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't root undefined here?

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'll look into this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 68b42c2

);
} else if (dependency === 'topWheel') {
if (isEventSupported('wheel')) {
trapBubbledEvent('topWheel', 'wheel', mountAt);
} else if (isEventSupported('mousewheel')) {
Expand Down
26 changes: 16 additions & 10 deletions packages/react-dom/src/events/ReactDOMEventListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,18 @@ export function isEnabled() {
return _enabled;
}

let typeCache = {};

function getDispatcher(topLevelType) {
if (topLevelType in typeCache) {
return typeCache[topLevelType];
}

typeCache[topLevelType] = dispatchEvent.bind(null, topLevelType);

return typeCache[topLevelType];
}

/**
* Traps top-level events by using event bubbling.
*
Expand All @@ -124,11 +136,8 @@ export function trapBubbledEvent(topLevelType, handlerBaseName, element) {
if (!element) {
return null;
}
return EventListener.listen(
element,
handlerBaseName,
dispatchEvent.bind(null, topLevelType),
);

EventListener.listen(element, handlerBaseName, getDispatcher(topLevelType));
}

/**
Expand All @@ -145,11 +154,8 @@ export function trapCapturedEvent(topLevelType, handlerBaseName, element) {
if (!element) {
return null;
}
return EventListener.capture(
element,
handlerBaseName,
dispatchEvent.bind(null, topLevelType),
);

EventListener.capture(element, handlerBaseName, getDispatcher(topLevelType));
}

export function dispatchEvent(topLevelType, nativeEvent) {
Expand Down
Loading