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

Always use event capturing. Remove media events. #12919

Closed
wants to merge 3 commits into from
Closed

Always use event capturing. Remove media events. #12919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1655f4f
Always use event capturing. Remove media events.
nhunzaker May 28, 2018
3b74469
Remove bubbled listener from EventListener-www.js
nhunzaker May 28, 2018
fec5dd3
Add test case to cover proper capture/bubble order
nhunzaker Jun 6, 2018
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
94 changes: 70 additions & 24 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,30 +965,6 @@ describe('ReactDOMComponent', () => {
};
});

it('should work error event on <source> element', () => {
spyOnDevAndProd(console, 'log');
const container = document.createElement('div');
ReactDOM.render(
<video>
<source
src="http://example.org/video"
type="video/mp4"
onError={e => console.log('onError called')}
/>
</video>,
container,
);

const errorEvent = document.createEvent('Event');
errorEvent.initEvent('error', false, false);
container.getElementsByTagName('source')[0].dispatchEvent(errorEvent);

if (__DEV__) {
expect(console.log).toHaveBeenCalledTimes(1);
expect(console.log.calls.argsFor(0)[0]).toContain('onError called');
}
});

it('should not duplicate uppercased selfclosing tags', () => {
class Container extends React.Component {
render() {
Expand Down Expand Up @@ -2440,4 +2416,74 @@ describe('ReactDOMComponent', () => {
expect(node.getAttribute('onx')).toBe('bar');
});
});

describe('when dispatching events', () => {
it('should receive error events on <source> elements', () => {
const onError = jest.fn();
const container = document.createElement('div');

ReactDOM.render(
<video>
<source
src="http://example.org/video"
type="video/mp4"
onError={onError}
/>
</video>,
container,
);

try {
document.body.appendChild(container);

const errorEvent = document.createEvent('Event');
errorEvent.initEvent('error', false, false);
container.getElementsByTagName('source')[0].dispatchEvent(errorEvent);

expect(onError).toHaveBeenCalledTimes(1);
} finally {
container.remove();
}
});

it('receives events in the correct order', () => {
let eventOrder = [];
let track = tag => () => eventOrder.push(tag);

class TestCase extends React.Component {
componentDidMount() {
document.addEventListener('click', track('document bubble'));
document.addEventListener('click', track('document capture'), true);
}
render() {
return (
<div
onClick={track('element bubble')}
onClickCapture={track('element capture')}
/>
);
}
}

const container = document.createElement('div');
ReactDOM.render(<TestCase />, container);

try {
document.body.appendChild(container);

const errorEvent = document.createEvent('Event');
errorEvent.initEvent('click', true, true);
container.querySelector('div').dispatchEvent(errorEvent);

expect(eventOrder).toEqual([
'document capture',
'element capture',
'element bubble',
'document bubble',
]);
} finally {
container.remove();
}
});
});
});
116 changes: 65 additions & 51 deletions packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,49 +271,7 @@ describe('ReactDOMEventListener', () => {
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', () => {
it('should dispatch media events', () => {
const container = document.createElement('div');
document.body.appendChild(container);

Expand Down Expand Up @@ -345,13 +303,6 @@ describe('ReactDOMEventListener', () => {
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.
Expand All @@ -374,8 +325,71 @@ describe('ReactDOMEventListener', () => {
);
expect(handleVideoPlay).toHaveBeenCalledTimes(1);
} finally {
document.addEventListener = originalAddEventListener;
document.body.removeChild(container);
}
});

it('does not double dispatch scroll events at the deepest leaf', () => {
const top = jest.fn();
const middle = jest.fn();
const bottom = jest.fn();
const container = document.createElement('div');

ReactDOM.render(
<div id="top" onScroll={top}>
<div id="middle" onScroll={middle}>
<div id="bottom" onScroll={bottom} />
</div>
</div>,
container,
);

const target = container.querySelector('#bottom');
const event = document.createEvent('Event');

try {
document.body.appendChild(container);

event.initEvent('scroll', true, true);
target.dispatchEvent(event);

expect(top).toHaveBeenCalledTimes(1);
expect(middle).toHaveBeenCalledTimes(1);
expect(bottom).toHaveBeenCalledTimes(1);
} finally {
container.remove();
}
});

it('does not double dispatch scroll events at the middle leaf', () => {
const top = jest.fn();
const middle = jest.fn();
const bottom = jest.fn();
const container = document.createElement('div');

ReactDOM.render(
<div id="top" onScroll={top}>
<div id="middle" onScroll={middle}>
<div id="bottom" onScroll={bottom} />
</div>
</div>,
container,
);

const target = container.querySelector('#middle');
const event = document.createEvent('Event');

try {
document.body.appendChild(container);

event.initEvent('scroll', true, true);
target.dispatchEvent(event);

expect(top).toHaveBeenCalledTimes(1);
expect(middle).toHaveBeenCalledTimes(1);
expect(bottom).toHaveBeenCalledTimes(0);
} finally {
container.remove();
}
});
});
60 changes: 20 additions & 40 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 @@ -29,8 +29,7 @@ import {
TOP_SUBMIT,
TOP_TOGGLE,
} from '../events/DOMTopLevelEventTypes';
import {listenTo, trapBubbledEvent} from '../events/ReactBrowserEventEmitter';
import {mediaEventTypes} from '../events/DOMTopLevelEventTypes';
import {listenTo, trapEvent} from '../events/ReactBrowserEventEmitter';
import * as CSSPropertyOperations from '../shared/CSSPropertyOperations';
import {Namespaces, getIntrinsicNamespace} from '../shared/DOMNamespaces';
import {
Expand Down Expand Up @@ -452,41 +451,29 @@ export function setInitialProperties(
switch (tag) {
case 'iframe':
case 'object':
trapBubbledEvent(TOP_LOAD, domElement);
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);
}
props = rawProps;
break;
case 'source':
trapBubbledEvent(TOP_ERROR, domElement);
trapEvent(TOP_LOAD, domElement);
props = rawProps;
break;
case 'img':
case 'image':
case 'link':
trapBubbledEvent(TOP_ERROR, domElement);
trapBubbledEvent(TOP_LOAD, domElement);
trapEvent(TOP_ERROR, domElement);
trapEvent(TOP_LOAD, domElement);
props = rawProps;
break;
case 'form':
trapBubbledEvent(TOP_RESET, domElement);
trapBubbledEvent(TOP_SUBMIT, domElement);
trapEvent(TOP_RESET, domElement);
trapEvent(TOP_SUBMIT, domElement);
props = rawProps;
break;
case 'details':
trapBubbledEvent(TOP_TOGGLE, domElement);
trapEvent(TOP_TOGGLE, domElement);
props = rawProps;
break;
case 'input':
ReactDOMFiberInput.initWrapperState(domElement, rawProps);
props = ReactDOMFiberInput.getHostProps(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapEvent(TOP_INVALID, domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
Expand All @@ -498,15 +485,15 @@ export function setInitialProperties(
case 'select':
ReactDOMFiberSelect.initWrapperState(domElement, rawProps);
props = ReactDOMFiberSelect.getHostProps(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapEvent(TOP_INVALID, domElement);
// 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':
ReactDOMFiberTextarea.initWrapperState(domElement, rawProps);
props = ReactDOMFiberTextarea.getHostProps(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapEvent(TOP_INVALID, domElement);
// 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 @@ -843,34 +830,27 @@ export function diffHydratedProperties(
switch (tag) {
case 'iframe':
case 'object':
trapBubbledEvent(TOP_LOAD, domElement);
break;
case 'video':
case 'audio':
// Create listener for each media event
for (let i = 0; i < mediaEventTypes.length; i++) {
trapBubbledEvent(mediaEventTypes[i], domElement);
}
trapEvent(TOP_LOAD, domElement);
break;
case 'source':
trapBubbledEvent(TOP_ERROR, domElement);
trapEvent(TOP_ERROR, domElement);
break;
case 'img':
case 'image':
case 'link':
trapBubbledEvent(TOP_ERROR, domElement);
trapBubbledEvent(TOP_LOAD, domElement);
trapEvent(TOP_ERROR, domElement);
trapEvent(TOP_LOAD, domElement);
break;
case 'form':
trapBubbledEvent(TOP_RESET, domElement);
trapBubbledEvent(TOP_SUBMIT, domElement);
trapEvent(TOP_RESET, domElement);
trapEvent(TOP_SUBMIT, domElement);
break;
case 'details':
trapBubbledEvent(TOP_TOGGLE, domElement);
trapEvent(TOP_TOGGLE, domElement);
break;
case 'input':
ReactDOMFiberInput.initWrapperState(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapEvent(TOP_INVALID, domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
Expand All @@ -880,14 +860,14 @@ export function diffHydratedProperties(
break;
case 'select':
ReactDOMFiberSelect.initWrapperState(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapEvent(TOP_INVALID, domElement);
// 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':
ReactDOMFiberTextarea.initWrapperState(domElement, rawProps);
trapBubbledEvent(TOP_INVALID, domElement);
trapEvent(TOP_INVALID, domElement);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
ensureListeningTo(rootContainerElement, 'onChange');
Expand Down
Loading