Fix #21972: Add onResize event to video elements#21973
Fix #21972: Add onResize event to video elements#21973gaearon merged 2 commits intofacebook:mainfrom
onResize event to video elements#21973Conversation
This is a simple fix for facebook#21972 that adds support for the `onResize` media event. 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. Pre-commit checklist ([source](https://reactjs.org/docs/how-to-contribute.html#sending-a-pull-request)) ✅ Fork the repository and create your branch from `main`. ✅ Run `yarn` in the repository root. ✅ If you’ve fixed a bug or added code that should be tested, add tests! ✅ Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. ✅ Run `yarn test --prod` to test in the production environment. ✅ If you need a debugger, run `yarn debug-test --watch TestName`, open chrome://inspect, and press “Inspect”. ✅ Format your code with prettier (`yarn prettier`). ✅ Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. ✅ Run the Flow typechecks (`yarn flow`). ✅ If you haven’t already, complete the CLA.
| // List of events that need to be individually attached to video elements. | ||
| export const videoEventTypes: Array<DOMEventName> = ['resize']; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A one-item array seems like overkill to me. Are there any downsides to adding it to mediaEventTypes? I'd suggest that.
There was a problem hiding this comment.
Happy to make the change! The only “downside” is that since the resize event spec lists:
Media element is a
videoelement
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.
|
Comparing: 8723932...5ec46b2 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
Bumping this in case nobody has seen it yet 👀 |
gaearon
left a comment
gaearon
left a comment
There was a problem hiding this comment.
Thanks for a detailed PR. Unless there are good reasons, let's remove the second array and keep them unified.
| // List of events that need to be individually attached to video elements. | ||
| export const videoEventTypes: Array<DOMEventName> = ['resize']; |
There was a problem hiding this comment.
A one-item array seems like overkill to me. Are there any downsides to adding it to mediaEventTypes? I'd suggest that.
| @@ -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, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks for the review, @gaearon! I consolidated the arrays as requested. |
|
Thanks! |
…21973) * Fix facebook#21972: Add `onResize` event to video elements This is a simple fix for facebook#21972 that adds support for the `onResize` media event. 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. Pre-commit checklist ([source](https://reactjs.org/docs/how-to-contribute.html#sending-a-pull-request)) ✅ Fork the repository and create your branch from `main`. ✅ Run `yarn` in the repository root. ✅ If you’ve fixed a bug or added code that should be tested, add tests! ✅ Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. ✅ Run `yarn test --prod` to test in the production environment. ✅ If you need a debugger, run `yarn debug-test --watch TestName`, open chrome://inspect, and press “Inspect”. ✅ Format your code with prettier (`yarn prettier`). ✅ Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. ✅ Run the Flow typechecks (`yarn flow`). ✅ If you haven’t already, complete the CLA. * Consolidate `videoEventTypes` array into `mediaEventTypes`
5 participants
Summary
This is a simple fix for #21972 that adds support for the
onResizemedia event. See #21972 for a detailed description of the problem.Test Plan
onResizehandler should not throw a warningPre-commit checklist (source)
✅ Fork the repository and create your branch from
main.✅ Run
yarnin the repository root.✅ If you’ve fixed a bug or added code that should be tested, add tests!
✅ Ensure the test suite passes (
yarn test). Tip:yarn test --watch TestNameis helpful in development.✅ Run
yarn test --prodto test in the production environment.✅ If you need a debugger, run
yarn debug-test --watch TestName, open chrome://inspect, and press “Inspect”.✅ Format your code with prettier (
yarn prettier).✅ Make sure your code lints (
yarn lint). Tip:yarn lincto only check changed files.✅ Run the Flow typechecks (
yarn flow).✅ If you haven’t already, complete the CLA.
Post-commit checklist
The synthetic event reference docs will need to be updated.