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.

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

Fix a regression that caused us to listen to extra events at the top #12878

Merged
merged 9 commits into from
May 22, 2018
161 changes: 161 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,4 +217,165 @@ describe('ReactDOMEventListener', () => {
expect(mouseOut.mock.calls[0][0]).toEqual(instance.getInner());
document.body.removeChild(container);
});

// Regression test for https://github.com/facebook/react/pull/12877
it('should not fire form events twice', () => {
const container = document.createElement('div');
document.body.appendChild(container);

const formRef = React.createRef();
const inputRef = React.createRef();

const handleInvalid = jest.fn();
const handleReset = jest.fn();
const handleSubmit = jest.fn();
ReactDOM.render(
<form ref={formRef} onReset={handleReset} onSubmit={handleSubmit}>
<input ref={inputRef} onInvalid={handleInvalid} />
</form>,
container,
);

inputRef.current.dispatchEvent(
new Event('invalid', {
// https://developer.mozilla.org/en-US/docs/Web/Events/invalid
bubbles: false,
}),
);
expect(handleInvalid).toHaveBeenCalledTimes(1);

formRef.current.dispatchEvent(
new Event('reset', {
// https://developer.mozilla.org/en-US/docs/Web/Events/reset
bubbles: true,
}),
);
expect(handleReset).toHaveBeenCalledTimes(1);

formRef.current.dispatchEvent(
new Event('submit', {
// https://developer.mozilla.org/en-US/docs/Web/Events/submit
bubbles: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be nice to have a bubbles-false test for this too if we don't.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When would it be false?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Old browsers I think. That's why we listen on the element at all.

}),
);
expect(handleSubmit).toHaveBeenCalledTimes(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 failed before, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it failed before your fix


formRef.current.dispatchEvent(
new Event('submit', {
// Might happen on older browsers.
bubbles: true,
}),
);
expect(handleSubmit).toHaveBeenCalledTimes(2); // It already fired in this test.

document.body.removeChild(container);
});

it('should dispatch loadstart only for media elements', () => {
const container = document.createElement('div');
document.body.appendChild(container);

const imgRef = React.createRef();
const videoRef = React.createRef();

const handleImgLoadStart = jest.fn();
const handleVideoLoadStart = jest.fn();
ReactDOM.render(
<div>
<img ref={imgRef} onLoadStart={handleImgLoadStart} />
<video ref={videoRef} onLoadStart={handleVideoLoadStart} />
</div>,
container,
);

// Note for debugging: loadstart currently doesn't fire in Chrome.
// https://bugs.chromium.org/p/chromium/issues/detail?id=458851
imgRef.current.dispatchEvent(
new ProgressEvent('loadstart', {
bubbles: false,
}),
);
// Historically, we happened to not support onLoadStart
// on <img>, and this test documents that lack of support.
// If we decide to support it in the future, we should change
// this line to expect 1 call. Note that fixing this would
// be simple but would require attaching a handler to each
// <img>. So far nobody asked us for it.
expect(handleImgLoadStart).toHaveBeenCalledTimes(0);

videoRef.current.dispatchEvent(
new ProgressEvent('loadstart', {
bubbles: false,
}),
);
expect(handleVideoLoadStart).toHaveBeenCalledTimes(1);

document.body.removeChild(container);
});

it('should not attempt to listen to unnecessary events on the top level', () => {
const container = document.createElement('div');
document.body.appendChild(container);

const videoRef = React.createRef();
const handleVideoPlay = jest.fn(); // We'll test this one.
const mediaEvents = {
onAbort() {},
onCanPlay() {},
onCanPlayThrough() {},
onDurationChange() {},
onEmptied() {},
onEncrypted() {},
onEnded() {},
onError() {},
onLoadedData() {},
onLoadedMetadata() {},
onLoadStart() {},
onPause() {},
onPlay() {},
onPlaying() {},
onProgress() {},
onRateChange() {},
onSeeked() {},
onSeeking() {},
onStalled() {},
onSuspend() {},
onTimeUpdate() {},
onVolumeChange() {},
onWaiting() {},
};

const originalAddEventListener = document.addEventListener;
document.addEventListener = function(type) {
throw new Error(
`Did not expect to add a top-level listener for the "${type}" event.`,
);
};

try {
// We expect that mounting this tree will
// *not* attach handlers for any top-level events.
ReactDOM.render(
<div>
<video ref={videoRef} {...mediaEvents} onPlay={handleVideoPlay} />
<audio {...mediaEvents}>
<source {...mediaEvents} />
</audio>
<form onReset={() => {}} onSubmit={() => {}} />
</div>,
container,
);

// Also verify dispatching one of them works
videoRef.current.dispatchEvent(
new Event('play', {
bubbles: false,
}),
);
expect(handleVideoPlay).toHaveBeenCalledTimes(1);
} finally {
document.addEventListener = originalAddEventListener;
document.body.removeChild(container);
}
});
});
3 changes: 3 additions & 0 deletions packages/react-dom/src/events/DOMTopLevelEventTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ export const TOP_VOLUME_CHANGE = unsafeCastStringToDOMTopLevelType(
export const TOP_WAITING = unsafeCastStringToDOMTopLevelType('waiting');
export const TOP_WHEEL = unsafeCastStringToDOMTopLevelType('wheel');

// List of events that need to be individually attached to media elements.
// Note that events in this list will *not* be listened to at the top level
// unless they're explicitly whitelisted in `ReactBrowserEventEmitter.listenTo`.
export const mediaEventTypes = [
TOP_ABORT,
TOP_CAN_PLAY,
Expand Down
58 changes: 36 additions & 22 deletions packages/react-dom/src/events/ReactBrowserEventEmitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,12 @@ import {
TOP_CANCEL,
TOP_CLOSE,
TOP_FOCUS,
TOP_INVALID,
TOP_RESET,
TOP_SCROLL,
TOP_SUBMIT,
getRawEventName,
mediaEventTypes,
} from './DOMTopLevelEventTypes';
import {
setEnabled,
Expand Down Expand Up @@ -130,29 +133,40 @@ export function listenTo(
for (let i = 0; i < dependencies.length; i++) {
const dependency = dependencies[i];
if (!(isListening.hasOwnProperty(dependency) && isListening[dependency])) {
if (dependency === TOP_SCROLL) {
trapCapturedEvent(TOP_SCROLL, mountAt);
} else if (dependency === TOP_FOCUS || dependency === TOP_BLUR) {
trapCapturedEvent(TOP_FOCUS, mountAt);
trapCapturedEvent(TOP_BLUR, mountAt);

// to make sure blur and focus event listeners are only attached once
isListening[TOP_BLUR] = true;
isListening[TOP_FOCUS] = true;
} else if (dependency === TOP_CANCEL) {
if (isEventSupported('cancel', true)) {
trapCapturedEvent(TOP_CANCEL, mountAt);
}
isListening[TOP_CANCEL] = true;
} else if (dependency === TOP_CLOSE) {
if (isEventSupported('close', true)) {
trapCapturedEvent(TOP_CLOSE, mountAt);
}
isListening[TOP_CLOSE] = true;
} else if (dependency !== TOP_SUBMIT && dependency !== TOP_RESET) {
trapBubbledEvent(dependency, mountAt);
switch (dependency) {
case TOP_SCROLL:
trapCapturedEvent(TOP_SCROLL, mountAt);
break;
case TOP_FOCUS:
case TOP_BLUR:
trapCapturedEvent(TOP_FOCUS, mountAt);
trapCapturedEvent(TOP_BLUR, mountAt);
// We set the flag for a single dependency later in this function,
// but this ensures we mark both as attached rather than just one.
isListening[TOP_BLUR] = true;
isListening[TOP_FOCUS] = true;
break;
case TOP_CANCEL:
case TOP_CLOSE:
if (isEventSupported(getRawEventName(dependency), true)) {
trapCapturedEvent(dependency, mountAt);
}
break;
case TOP_INVALID:
case TOP_SUBMIT:
case TOP_RESET:
// We listen to them on the target DOM elements.
// Some of them bubble so we don't want them to fire twice.
break;
default:
// By default, listen on the top level to all non-media events.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add:

// Media events don't bubble so adding the listener wouldn't do anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

// Media events don't bubble so adding the listener wouldn't do anything.
const isMediaEvent = mediaEventTypes.indexOf(dependency) !== -1;
if (!isMediaEvent) {
trapBubbledEvent(dependency, mountAt);
}
break;
}

isListening[dependency] = true;
}
}
Expand Down