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

Fix #21972: Add onResize event to video elements #21973

Merged
merged 2 commits into from
Sep 7, 2021
Merged

Fix #21972: Add onResize event to video elements #21973

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ec46b2
Fix #21972: Add `onResize` event to video elements
rileyjshaw Jul 27, 2021
3c336f0
Consolidate `videoEventTypes` array into `mediaEventTypes`
rileyjshaw Sep 7, 2021
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
1 change: 1 addition & 0 deletions fixtures/dom/src/components/fixtures/media-events/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default class MediaEvents extends React.Component {
onPlaying: false,
onProgress: false,
onRateChange: false,
onResize: false,
onSeeked: false,
onSeeking: false,
onSuspend: false,
Expand Down
2 changes: 2 additions & 0 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 @@ -388,6 +388,7 @@ describe('ReactDOMEventListener', () => {
onPlaying() {},
onProgress() {},
onRateChange() {},
onResize() {},
onSeeked() {},
onSeeking() {},
onStalled() {},
Expand Down Expand Up @@ -430,6 +431,7 @@ describe('ReactDOMEventListener', () => {
case 'playing':
case 'progress':
case 'ratechange':
case 'resize':
case 'seeked':
case 'seeking':
case 'stalled':
Expand Down
16 changes: 16 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMEventPropagation-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1095,6 +1095,22 @@ describe('ReactDOMEventListener', () => {
});
});

it('onResize', () => {
testEmulatedBubblingEvent({
type: 'video',
reactEvent: 'onResize',
reactEventType: 'resize',
nativeEvent: 'resize',
dispatch(node) {
const e = new Event('resize', {
bubbles: false,
cancelable: true,
});
node.dispatchEvent(e);
},
});
});

it('onSeeked', () => {
testEmulatedBubblingEvent({
type: 'video',
Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/__tests__/__snapshots__/ReactTestUtils-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ Array [
"progress",
"rateChange",
"reset",
"resize",
"scroll",
"seeked",
"seeking",
Expand Down
9 changes: 9 additions & 0 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 @@ -71,6 +71,7 @@ import {REACT_OPAQUE_ID_TYPE} from 'shared/ReactSymbols';

import {enableTrustedTypesIntegration} from 'shared/ReactFeatureFlags';
import {
videoEventTypes,
mediaEventTypes,
listenToNonDelegatedEvent,
} from '../events/DOMPluginEventSystem';
Expand Down Expand Up @@ -496,6 +497,10 @@ export function setInitialProperties(
props = rawProps;
break;
case 'video':
for (let i = 0; i < videoEventTypes.length; i++) {
listenToNonDelegatedEvent(videoEventTypes[i], domElement);
}
// falls through
case 'audio':
// We listen to these events in case to ensure emulated bubble
// listeners still fire for all the media events.
Expand Down Expand Up @@ -885,6 +890,10 @@ export function diffHydratedProperties(
listenToNonDelegatedEvent('load', domElement);
break;
case 'video':
for (let i = 0; i < videoEventTypes.length; i++) {
listenToNonDelegatedEvent(videoEventTypes[i], domElement);
}
// falls through
case 'audio':
// We listen to these events in case to ensure emulated bubble
// listeners still fire for all the media events.
Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/events/DOMEventNames.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export type DOMEventName =
| 'progress'
| 'ratechange'
| 'reset'
| 'resize'
| 'scroll'
| 'seeked'
| 'seeking'
Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/events/DOMEventProperties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ const simpleEventPluginEvents = [
'progress',
'rateChange',
'reset',
'resize',
'seeked',
'seeking',
'stalled',
Expand Down
4 changes: 4 additions & 0 deletions packages/react-dom/src/events/DOMPluginEventSystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,9 @@ export const mediaEventTypes: Array<DOMEventName> = [
'waiting',
];

// List of events that need to be individually attached to video elements.
export const videoEventTypes: Array<DOMEventName> = ['resize'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I created a separate videoEventTypes array, since I doubt anyone will want to add onResize to an audio event. It would simplify the code a bit to just add resize to the mediaEventTypes array, if that’s preferred.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A one-item array seems like overkill to me. Are there any downsides to adding it to mediaEventTypes? I'd suggest that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to make the change! The only “downside” is that since the resize event spec lists:

Media element is a video element

in its preconditions, we might want to log a warning if someone attaches onResize to an audio element. But I agree, I think the simplification is worth it. I’ll make the change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✨


// We should not delegate these events to the container, but rather
// set them on the actual target element itself. This is primarily
// because these events do not consistently bubble in the DOM.
Expand All @@ -216,6 +219,7 @@ export const nonDelegatedEvents: Set<DOMEventName> = new Set([
// and can occur on other elements too. Rather than duplicate that event,
// we just take it from the media events array.
...mediaEventTypes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that adding resize here would disable delegation for it. Are there any existing cases where we relied on delegation for it? I suppose not because we used to warn, so it was definitely unused.

Copy link
Contributor Author

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 it was unused. Perhaps there could be a future conflict if a Synthetic Event was created for the Window:resize event?

It seems safe to me for now, but let me know if there are any further tests you’d like me to run.

...videoEventTypes,
]);

function executeDispatch(
Expand Down
1 change: 1 addition & 0 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 @@ -321,6 +321,7 @@ export function getEventPriority(domEventName: DOMEventName): * {
case 'pointerup':
case 'ratechange':
case 'reset':
case 'resize':
case 'seeked':
case 'submit':
case 'touchcancel':
Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/events/TopLevelEventTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export type TopLevelType =
| 'progress'
| 'ratechange'
| 'reset'
| 'resize'
| 'scroll'
| 'seeked'
| 'seeking'
Expand Down
1 change: 1 addition & 0 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 @@ -642,6 +642,7 @@ const simulatedEventTypes = [
'pointerUp',
'rateChange',
'reset',
'resize',
'seeked',
'submit',
'touchCancel',
Expand Down