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): make sure there's only one fullscreenchange event #5686

Merged
merged 9 commits into from Jan 7, 2019

Conversation

Projects
None yet
3 participants
@gkatsev
Copy link
Member

gkatsev commented Dec 14, 2018

Before the fullscreen API was un-prefixed, we listened to triggered a
fullscreenchange event on the player manually. This worked fine
because a prefixed fullscreenchange event was triggered on the player
element itself. But when it was unprefixed, we ended up with two events
now, the unprefixed event and the one we triggered.

Instead, we should listen the event the browser supports and re-trigger
it as the unprefixed event as necessary.

We also make sure that the handler gets calls when the document level
fullscreenchange handler gets called so that it is cancelled properly.

Fixes #5685.

Show resolved Hide resolved src/js/player.js Outdated
@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Dec 14, 2018

Looks like this works great everywhere except IE11.

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Dec 14, 2018

Firefox has problems with closing.

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Dec 17, 2018

@misteroneill
Copy link
Member

misteroneill left a comment

👍

Verified in Chrome, Firefox, Safari, Edge, and IE11.

@gkatsev gkatsev added the patch label Dec 18, 2018

@nackd

This comment has been minimized.

Copy link

nackd commented Dec 19, 2018

I tried this on a lot of different browsers and devices, including GC and FF from before and after the move to unprefixed. It worked everywhere, except on Android and desktop Firefox before unprefixed, where the event didn't fire. Note that depending on the Android device, that might be the latest release available.

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Dec 19, 2018

@nackd that is firefox on android? Also, thanks for checking!

@nackd

This comment has been minimized.

Copy link

nackd commented Dec 19, 2018

Firefox on Android, yes.

@nackd

This comment has been minimized.

Copy link

nackd commented Dec 19, 2018

In case there are doubts yet: besides Firefox on Android, it also failed to fire on desktop Firefox (I tried Firefox 61, but I presume it fails with 62 and 63 too).

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Dec 19, 2018

Thanks! Yeah, it's likely the same issue. I'll take a look.

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Dec 21, 2018

@nackd I just pushed a change (c026af8) that I think fixes older firefox, would you be able to give it a try again?

@nackd

This comment has been minimized.

Copy link

nackd commented Dec 21, 2018

Sure, but only on Thursday, @gkatsev.

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Dec 21, 2018

@nackd that's fine. We're more or less all on break for the week anyway :)

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Jan 2, 2019

@nackd hope you had a good new year, did you have a chance to take a look at this PR? Thanks!

@nackd

This comment has been minimized.

Copy link

nackd commented Jan 3, 2019

Thanks, @gkatsev, happy new year! I'm busier than I anticipated, but I plan to have a look at this tomorrow or during the weekend.

@gkatsev gkatsev added this to the 7.4.x milestone Jan 3, 2019

@gkatsev gkatsev added the needs: LGTM label Jan 4, 2019

@nackd

This comment has been minimized.

Copy link

nackd commented Jan 7, 2019

I was finally able to get to this and it seems to work great, be it prefixed, unprefixed, desktop or Android Firefox. Thanks a lot, @gkatsev!

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Jan 7, 2019

Awesome, glad to hear!

gkatsev added some commits Dec 14, 2018

fix(fs): make sure there's only one fullscreenchange event
Before the fullscreen API was un-prefixed, we listened to triggered a
`fullscreenchange` event on the player manually. This worked fine
because a prefixed `fullscreenchange` event was triggered on the player
element itself. But when it was unprefixed, we ended up with two events
now, the unprefixed event and the one we triggered.

Instead, we should listen the event the browser supports and re-trigger
it as the unprefixed event as necessary.

We also make sure that the handler gets calls when the document level
fullscreenchange handler gets called so that it is cancelled properly.

Fixes #5685.

@gkatsev gkatsev force-pushed the unprefixed-fs branch from ac820c2 to 63dd30b Jan 7, 2019

@gkatsev gkatsev merged commit 2f00a68 into master Jan 7, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
netlify/videojs-docs/deploy-preview Deploy preview ready!
Details

@gkatsev gkatsev deleted the unprefixed-fs branch Jan 7, 2019

gkatsev added a commit that referenced this pull request Jan 8, 2019

fix(fs): make sure there's only one fullscreenchange event (#5686)
Before the fullscreen API was un-prefixed, we listened to triggered a
`fullscreenchange` event on the player manually. This worked fine
because a prefixed `fullscreenchange` event was triggered on the player
element itself. But when it was unprefixed, we ended up with two events
now, the unprefixed event and the one we triggered.

Instead, we should listen the event the browser supports and re-trigger
it as the unprefixed event as necessary.

We also make sure that the handler gets calls when the document level
fullscreenchange handler gets called so that it is cancelled properly.

Fixes #5685.

gkatsev added a commit that referenced this pull request Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment