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

player fullscreenchange firing twice #5685

Closed
nackd opened this Issue Dec 13, 2018 · 13 comments

Comments

Projects
None yet
2 participants
@nackd
Copy link

nackd commented Dec 13, 2018

In browsers that support the unprefixed fullscreen API (FF 64+ GC71+), the fullscreenchange event is firing twice every time fullscreen is toggled.

https://jsfiddle.net/jbrhq4s1/1/

@welcome

This comment has been minimized.

Copy link

welcome bot commented Dec 13, 2018

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
To help make it easier for us to investigate your issue, please follow the contributing guidelines.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 13, 2018

This is a side-effect of using alert(). The alert brings the element out of fullscreen causing the second fullscreenchange event to trigger.
If you change the alert to a console.log, as in this example https://jsfiddle.net/jbrhq4s1/2/, you will only get one fullscreenchange event.

Hope that helps.

@gkatsev gkatsev closed this Dec 13, 2018

@nackd

This comment has been minimized.

Copy link
Author

nackd commented Dec 13, 2018

No it's not a side effect, it also happens with console.log.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 13, 2018

I only see a single console.log event for each time the fullscreen toggle button is pressed.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 13, 2018

Interesting, just tried it in Firefox, I can reproduce it there but can't reproduce it in my version of chrome.

@gkatsev gkatsev reopened this Dec 13, 2018

@nackd

This comment has been minimized.

Copy link
Author

nackd commented Dec 13, 2018

Funny. Is that Chrome 71 or later, @gkatsev?

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 13, 2018

I was on chrome 70, I upgraded to 71 and can reproduce it in chrome as well now.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 13, 2018

So, looks like this is due to a change in fullscreen behavior in browsers. We listen to the fullscreenchange event on the player

this.on('fullscreenchange', this.handleFullscreenChange_);

and previously, this would only be from our own triggering of fullscreenchange, starting with Chrome 71 (and a recent version of Firefox) which fully enabled spec version of the fullscreen API, we now received a fullscreenchange event from the element itself in addition to us triggering it manually.

I was able to test with this codepen https://codepen.io/gkatsev/pen/GPovor in older browsers and did not get a fullscreenchange event from the element itself.

The new browser behavior is likely the current spec based behavior and so we probably need to update Video.js to support both.

@gkatsev gkatsev added the confirmed label Dec 13, 2018

@nackd

This comment has been minimized.

Copy link
Author

nackd commented Dec 14, 2018

BTW, in case this helps others, I took advantage of the two events coming from different sources and having some different properties to workaround this by adding this code to the top of the event handler in our product:

    if (event.composed) {
      // Workaround events firing twice
      return;
    }

In unaffected browsers the rest of the event handler still runs and in the affected ones it runs once.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 14, 2018

Looking at it more, this only worked previously by accident. Elements fired prefixed fullscreenchange events, so webkitfullscreenchange still fired. So, had we been listening to webkitfullscreenchange we would've been bit by this previously as well.
Chrome 71 unprefixed the fs API: https://bugs.chromium.org/p/chromium/issues/detail?id=383813

gkatsev added a commit that referenced this issue 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

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 14, 2018

I have a potential fix in #5686, if you can take it out for a spin, that would be great!

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 14, 2018

It works in Chrome but has some issues in firefox and ie11...

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Dec 17, 2018

Ok, I think I have it working across all browsers now in that same PR

gkatsev added a commit that referenced this issue Jan 7, 2019

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 closed this in #5686 Jan 7, 2019

gkatsev added a commit that referenced this issue Jan 7, 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 issue 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 issue Jan 15, 2019

fix(fs): fix double fullscreenchange event
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

gkatsev added a commit that referenced this issue Jan 22, 2019

fix(fs): fix double fullscreenchange event (#5756)
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

gkatsev added a commit that referenced this issue Jan 22, 2019

fix(fs): fix double fullscreenchange event (#5756)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment