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(fs): fix double fullscreenchange event #5756
Conversation
(Looks like maybe would've been easier not to revert it, but it was easier to get to this state this way, so, 🤷♂️ ) |
@nackd hey, would I be able to recruit you to take a look at this PR for fullscreenchange stuff? The previous fix unfortunately broke the vjs-fullscreen class, so, I made this PR, which actually is a bit simpler than the other one. Seems to work just as well. Would love it if you'll be able to take a look again! |
@blacktrash hey, would you be able to grab this PR and test it to verify that vjs-fullscreen is added and removed appropriately? I think the only downside right now is that it's possible that on a |
@gkatsev - thanks, will do. May take me a couple of days though, because of other stuff on my plate, sorry. |
@blacktrash That's fine, just want as many eyes on this as possible :). I hope to be able to release this early next week. |
With this version I see the video disappearing when exiting from fullscreen (all browsers). Also, on iOS the event seems to fire only when entering fullscreen. |
Thanks for looking @nackd. Interesting that you see the video disappear when exiting fullscreen, I do not see that. Guess more work is needed... |
@gkatsev, I didn't build the library myself, took it from here where it happens to me too: https://deploy-preview-5756--videojs-docs.netlify.com/test-example/ |
That's what it's for :) Was just able to reproduce it. |
huh, weirdly enough I can't reproduce locally but can reproduce on the netlify build. Guess I'll make sure to verify with netlify before calling it done. |
I think the last commit fixes the disappearing player on exiting fullscreen. Looking at iOS now. |
older iOS with the old style fullscreen API is getting the events as expected. Going to look at an iOS 12 ipad, since it has support for the spec fs api. |
Interestingly, on iOS with the fs api, we get a fullscreenchange event when exiting fs when using pinch-to-exit or using the native button and not when using our fs toggle. |
Looks like trying to make sure that the document handler is removed causes issues, and it isn't strictly necessary. @nackd can you retest please? Thank you so much! |
@gkatsev - test case demo at https://blacktrash.org/test/videojs/issue5745.html now is at 8d6d000 and works as expected so far with a bit of cursory testing.
I'll try to keep the test case at the tip of the fs-part-deux branch.
|
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 minor nitpick.
src/js/player.js
Outdated
@@ -15,13 +15,13 @@ import * as Dom from './utils/dom.js'; | |||
import * as Fn from './utils/fn.js'; | |||
import * as Guid from './utils/guid.js'; | |||
import * as browser from './utils/browser.js'; | |||
import {IE_VERSION} from './utils/browser.js'; | |||
import {IE_VERSION} from './utils/browser'; |
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.
Might as well make this consistent with the above line. Or remove .js
from the rest of the file. I don't have an opinion either way as long as it's consistent.
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'll probably make it so this isn't a change.
@blacktrash thanks! appreciate any time you spend on this! |
I gave it a try and it's working fine now as far as I can see. Cheers! |
Awesome, appreciate the help @nackd! |
This reverts the previous fix (2bc90a1)
and also simplifies handling. Seems like this works a lot better while
also ensuring that the vjs-fullscreen class is still on the player.
Fixes #5685, fixes #5745