-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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: issue where changing focus is causing unwanted scrolling behavior on iOS #4607
Conversation
src/js/menu/menu-button.js
Outdated
@@ -8,6 +8,7 @@ import * as Dom from '../utils/dom.js'; | |||
import * as Fn from '../utils/fn.js'; | |||
import * as Events from '../utils/events.js'; | |||
import toTitleCase from '../utils/to-title-case.js'; | |||
import IS_IOS from '../utils/browser.js'; |
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.
IS_IOS
needs to be wrapped in curly braces when you want a named import
import { IS_IOS } from '../utils/browser.js';
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.
This is also why the build is failing.
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.
facepalm. Fixed.
I checked the fix on iOS 10.2.1 (iPad), using master branch of socrates and fix-menu-focus-ios branch of video.js. It works fine, I don't see it scroll abruptly anymore. |
@alex-barstow so, since this only happens when the player is embedded in an iframe, maybe we should add that to the check? |
…d it to existing conditional in pressButton()
@gkatsev Changes made! I added the iframe check as a new utils function in dom.js |
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 small proposal.
src/js/utils/dom.js
Outdated
* @return {boolean} | ||
* | ||
*/ | ||
export function isPlayerInFrame() { |
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 propose changing this to isInIframe
more generally. Might be pedantic or whatever, but that seems like a better name.
Description
Changing focus to a sub-menu (ex. by tapping the captions menu button) when a player is embedded in an iframe is resulting in unwanted scrolling behavior on iOS. @gkatsev is planning to refactor all focus handling soon, but this will serve as a temporary fix in the meantime.
Specific Additions
Modify the MenuButton
pressButton()
method to only change focus to a sub-menu on platforms other than iOS