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

Drop some top-level events from the list #11912

Merged
merged 2 commits into from
Jan 11, 2018
Merged

Drop some top-level events from the list #11912

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
e8e7a66
Drop some top-level events from the list
gaearon Dec 22, 2017
2dbf22e
Put both whitelists in one file
gaearon Dec 22, 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
41 changes: 7 additions & 34 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 @@ -22,6 +22,7 @@ import * as inputValueTracking from './inputValueTracking';
import setInnerHTML from './setInnerHTML';
import setTextContent from './setTextContent';
import {listenTo, trapBubbledEvent} from '../events/ReactBrowserEventEmitter';
import {mediaEventTypes} from '../events/BrowserEventConstants';
import * as CSSPropertyOperations from '../shared/CSSPropertyOperations';
import {Namespaces, getIntrinsicNamespace} from '../shared/DOMNamespaces';
import {
Expand Down Expand Up @@ -223,34 +224,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
const 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
Expand Down Expand Up @@ -472,9 +445,9 @@ export function setInitialProperties(
case 'video':
case 'audio':
// Create listener for each media event
for (const event in mediaEvents) {
if (mediaEvents.hasOwnProperty(event)) {
trapBubbledEvent(event, mediaEvents[event], domElement);
for (const event in mediaEventTypes) {
if (mediaEventTypes.hasOwnProperty(event)) {
trapBubbledEvent(event, mediaEventTypes[event], domElement);
Copy link
Contributor

Choose a reason for hiding this comment

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

trapBubbledEvent is kind of a confusing name now since it's attaching the listener directly to the event target in some cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. It's more like "attach this event locally to emulate bubbling"

}
}
props = rawProps;
Expand Down Expand Up @@ -860,9 +833,9 @@ export function diffHydratedProperties(
case 'video':
case 'audio':
// Create listener for each media event
for (const event in mediaEvents) {
if (mediaEvents.hasOwnProperty(event)) {
trapBubbledEvent(event, mediaEvents[event], domElement);
for (const event in mediaEventTypes) {
if (mediaEventTypes.hasOwnProperty(event)) {
trapBubbledEvent(event, mediaEventTypes[event], domElement);
}
}
break;
Expand Down
63 changes: 33 additions & 30 deletions packages/react-dom/src/events/BrowserEventConstants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,16 @@ import getVendorPrefixedEventName from './getVendorPrefixedEventName';
/**
* Types of raw signals from the browser caught at the top level.
*
* For events like 'submit' which don't consistently bubble (which we
* trap at a lower node than `document`), binding at `document` would
* cause duplicate events so we don't include them here.
* For events like 'submit' or audio/video events which don't consistently
* bubble (which we trap at a lower node than `document`), binding
* at `document` would cause duplicate events so we don't include them here.
*/
const topLevelTypes = {
topAbort: 'abort',
export const topLevelTypes = {
topAnimationEnd: getVendorPrefixedEventName('animationend'),
topAnimationIteration: getVendorPrefixedEventName('animationiteration'),
topAnimationStart: getVendorPrefixedEventName('animationstart'),
topBlur: 'blur',
topCancel: 'cancel',
topCanPlay: 'canplay',
topCanPlayThrough: 'canplaythrough',
topChange: 'change',
topClick: 'click',
topClose: 'close',
Expand All @@ -41,54 +38,60 @@ const topLevelTypes = {
topDragOver: 'dragover',
topDragStart: 'dragstart',
topDrop: 'drop',
topDurationChange: 'durationchange',
topEmptied: 'emptied',
topEncrypted: 'encrypted',
topEnded: 'ended',
topError: 'error',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one also fires for JS errors and images (not just media) but AFAIK it doesn't truly bubble either way.

Copy link
Contributor

@nhunzaker nhunzaker Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Correct. It does not fire bubble for images, iframes, videos, or audio tags.

But it does capture. We might be able to attach them at the top level using capture to avoid eagerly attaching a listener to each element.

topFocus: 'focus',
topInput: 'input',
topKeyDown: 'keydown',
topKeyPress: 'keypress',
topKeyUp: 'keyup',
topLoadedData: 'loadeddata',
topLoad: 'load',
topLoadedMetadata: 'loadedmetadata',
topLoadStart: 'loadstart',
topMouseDown: 'mousedown',
topMouseMove: 'mousemove',
topMouseOut: 'mouseout',
topMouseOver: 'mouseover',
topMouseUp: 'mouseup',
topPaste: 'paste',
topScroll: 'scroll',
topSelectionChange: 'selectionchange',
topTextInput: 'textInput',
topToggle: 'toggle',
topTouchCancel: 'touchcancel',
topTouchEnd: 'touchend',
topTouchMove: 'touchmove',
topTouchStart: 'touchstart',
topTransitionEnd: getVendorPrefixedEventName('transitionend'),
topWheel: 'wheel',
};

// There are so many media events, it makes sense to just
// maintain a list of them. Note these aren't technically
// "top-level" since they don't bubble. We should come up
// with a better naming convention if we come to refactoring
// the event system.
export const mediaEventTypes = {
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',
topScroll: 'scroll',
topSeeked: 'seeked',
topSeeking: 'seeking',
topSelectionChange: 'selectionchange',
topStalled: 'stalled',
topSuspend: 'suspend',
topTextInput: 'textInput',
topTimeUpdate: 'timeupdate',
topToggle: 'toggle',
topTouchCancel: 'touchcancel',
topTouchEnd: 'touchend',
topTouchMove: 'touchmove',
topTouchStart: 'touchstart',
topTransitionEnd: getVendorPrefixedEventName('transitionend'),
topVolumeChange: 'volumechange',
topWaiting: 'waiting',
topWheel: 'wheel',
};

export type TopLevelTypes = $Enum<typeof topLevelTypes>;

const BrowserEventConstants = {
topLevelTypes,
};

export default BrowserEventConstants;
4 changes: 1 addition & 3 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,9 +13,7 @@ import {
trapCapturedEvent,
} from './ReactDOMEventListener';
import isEventSupported from './isEventSupported';
import BrowserEventConstants from './BrowserEventConstants';

const {topLevelTypes} = BrowserEventConstants;
import {topLevelTypes} from './BrowserEventConstants';

/**
* Summary of `ReactBrowserEventEmitter` event handling:
Expand Down
4 changes: 1 addition & 3 deletions packages/react-dom/src/test-utils/ReactTestUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
import SyntheticEvent from 'events/SyntheticEvent';
import invariant from 'fbjs/lib/invariant';

import BrowserEventConstants from '../events/BrowserEventConstants';
import {topLevelTypes} from '../events/BrowserEventConstants';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops I think this might not be enough now? We need to do the same for mediaEventTypes or we'll "lose" some events?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah man. I think you're right. Darn.


const {findDOMNode} = ReactDOM;
const {
Expand All @@ -30,8 +30,6 @@ const {
ReactDOMEventListener,
} = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;

const topLevelTypes = BrowserEventConstants.topLevelTypes;

function Event(suffix) {}

/**
Expand Down