-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Use passive event listeners for touchstart/touchmove #4440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment/thought, but LGTM.
src/js/utils/events.js
Outdated
let options = false; | ||
|
||
if (_supportsPassive && | ||
['touchstart', 'touchmove'].indexOf(type) > -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this array ought to be defined once in the parent scope rather than inline. Something like:
const passiveEvents = [
'touchstart',
'touchmove'
];
src/js/utils/events.js
Outdated
*/ | ||
let _supportsPassive = false; | ||
|
||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should wrap this try/catch in an iife.
Looks like this will fix #4432 (we should not that in the merge commit). |
We probably should make this against 5.x as well. |
fix: Use passive event listeners for touchstart/touchmove (videojs#4440)
Description
Not using passive event listeners now causes Chrome to log warnings to the console, and possibly has a performance impact
Fixes #4432
Specific Changes proposed
Where supported, use passive event listeners for
touchmove
andtouchstart
Requirements Checklist