Skip to content
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

gkatsev
Copy link
Member

@gkatsev 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.

src/js/player.js Outdated Show resolved Hide resolved
@gkatsev
Copy link
Member Author

gkatsev commented Dec 14, 2018

Looks like this works great everywhere except IE11.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 14, 2018

Firefox has problems with closing.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 17, 2018

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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

@gkatsev gkatsev added the patch This PR can be added to a patch release. label Dec 18, 2018
@nackd
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
Copy link
Member Author

gkatsev commented Dec 19, 2018

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

@nackd
Copy link

nackd commented Dec 19, 2018

Firefox on Android, yes.

@nackd
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
Copy link
Member Author

gkatsev commented Dec 19, 2018

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

@gkatsev
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
Copy link

nackd commented Dec 21, 2018

Sure, but only on Thursday, @gkatsev.

@gkatsev
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
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
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 Needs one or more additional approvals label Jan 4, 2019
@nackd
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
Copy link
Member Author

gkatsev commented Jan 7, 2019

Awesome, glad to hear!

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 merged commit 2f00a68 into master Jan 7, 2019
@gkatsev gkatsev deleted the unprefixed-fs branch January 7, 2019 21:38
gkatsev added a commit that referenced this pull request Jan 8, 2019
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
@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

player fullscreenchange firing twice
3 participants