Video EXIT fullscreen with ESC key giving wrong isFullscreen info

[richardbushell]

Hi

When exiting Video Fullscreen using the ESC key, I get an incorrect value back from the fullscreenchange listener. BUT it's fine when exiting with the Video Fullscreen Exit Button instead.

Here's my self-explanatory function used to test, with notes inline.

myPlayer.on('fullscreenchange', function () {
	if (myPlayer.isFullscreen()) {
		console.log("Video fullscreenchange: Video entered fullscreen");
		// WEIRD: ESC to EXIT when VIDEO FULLSCREEN triggers this "Entered" message again!
		// wrap this in a setTimeout function and it's OK
		// p.s. but it works fine anyway when using video fullscreen button exit instead of ESC key
	} else {
		console.log("Video fullscreenchange: Video exited fullscreen");
	}
});

Is this a known issue? Is there already a best-practice or solution to this as we were getting entirely inconsistent behaviour.

Please advise, thanks.

[gkatsev]

Yeah, things got a bit hairy in recent chrome when they un-prefixed the fullscreen API. We have plans on refactoring how we handle fullscreen related stuff but that isn't a short term solution.
Your best bet is likely to add a player.setTimeout() in there.

[richardbushell]

@gkatsev Yes, as I stated in my inline code comments, I did get it working with a SetTimeout wrapper, so I'll stick with that until hopefully the refactor finds a good solution. Did you want to leave the issue open for addressing subsequently, or if not then close as you see fit. Thanks.

[gkatsev]

Ah, cool, totally missed that.
Looks like we don't currently have an issue to track the refactor (I have it on my end, though), so, let's leave this open.

[richardbushell]

Actually, the setTimeout hack didn't solve my issue. I have just realised that whilst setTimeout did fix the issue with ESC, it concurrently broke the Video Fullscreen Exit Button (which works without the setTimeout).

So, it's XOR. You can't have both. isFullscreen() will always report falsely as the result of one or the other. Will leave this issue open as suggested.

[gkatsev]

Have a live example of what you're seeing?

[richardbushell]

Sure, open:
https://living.video/direct.php?item=hphoverlay&fluid=false
Then open Chrome Dev Tools (I'm on Chrome 72)
Paste in this function to the console:-

myPlayer.on('fullscreenchange', function () {
	if (myPlayer.isFullscreen()) {
		console.log("Video fullscreenchange: Video entered fullscreen");
	} else {
		console.log("Video fullscreenchange: Video exited fullscreen");
	}
});

Then, play video, make it fullscreen with Video Fullscreen Button.
Exit with ESC key.
Shows 2 consecutive console logs that it has ENTERED fullscreen.

[richardbushell]

Closed via 3fbc4f5

[lanxan]

Hello @richardbushell
Seems like videojs 7.5.2 also has same problem on chrome 73.0.3683.86.

See the jsfiddle

I checked the code and found the fullscreenchange will fire before documentFullscreenChange_ while use esc on fullscreen mode. And I have no idea with that.

Still use setTimout to solve problem.
Should I open a new issue? thanks.

[richardbushell]

@lanxan Yes, the fix mentioned above was actually a bug fix for the separate Nested Fullscreen issue:
#5828

I actually closed this thread at the same time (only because I'd worked around it using alternative methods so I no longer relied upon the timing), but you are correct that the timing of the event firing with the ESC key remains inconsistent against exiting via the video fullscreen-exit button. I haven't looked at the official spec for ESC but I wonder if this uses Capture rather than Bubble phasing as the order seems inconsistent between the two. Please re-open the issue if you wish.

[gkatsev]

Going to re-open this. The issue has to do with how we know whether things change and when we trigger the fullscreenchange event on the player. Things broke with Chrome unprefixing the fullscreen API as it changed our expectations of event ordering, which is why current you'll need a setTimeout.
We're planning on refactoring how fullscreen handling is done in Video.js and we'll make sure to address this as part of that.

[richardbushell]

OK, in advance of looking at a possible fullscreen refactor, let me donate some code ideas:-

app.isFullscreenEl = function(el) {
	return (
			el === fscreen.fullscreenElement
				// only checks against the CURRENT (topmost) fullscreen element
		) || (
			el.matches(app.fullscreenPseudoClass)
				// ANY fullscreen element should match the :fullscreen pseudo-class
					// unfortunately, we can't rely on fullscreenPseudoClass as the ONLY test UNTIL Browsers FIX matching of ALL :fullscreen elements
		) || (
			Array.from(document.querySelectorAll(app.fullscreenPseudoClass)).includes(el)
			// checks inclusion within the NODELIST Collection of ALL fullscreen elements matching the :fullscreen pseudo-class. Note: There is no direct ".includes()" method of an Iterable NodeList collection, so we convert it into an array first
				// again, awaiting Browser FIX of ALL :fullscreen elements
		);
};

You obviously wouldn't need to use BOTH pseudo-class tests concurrently (I include them only for reference and discussion). Bizarrely, the only way to definitively test whether on element is fullscreen is through CSS! Several elements can be fullscreen concurrently, only one of which is topmost in the fullscreen Stack. But it is a Stack, not an array, and only the topmost element in the stack is exposed to standard Javascript methods, but CSS provides a way to know whether the element is in the fullscreen stack.

See: whatwg/fullscreen#70 (comment)

And my test page here:-
https://living.video/fullscreenchange.html

[richardbushell]

p.s. Firefox, Edge, and IE (amazingly) do report ALL fullscreen elements correctly when several are fullscreen concurrently. Chrome and WebKit (Safari) have bugs filed (by me) to fix this. BUT when fixed the fullscreen pseudo-class test should probably be the only test required to determine if an element is fullscreen.

[richardbushell]

I have submitted a PR here:
#5912
which also takes into account that the VJS player may not be the ONLY element in the fullscreen stack as an initial step towards any further fullscreen api development. This checks whether the VJS player element matches the fullscreen pseudo-class. As per my note above regarding the discussion here:
whatwg/fullscreen#70 (comment)

[gkatsev]

One idea I had for fixing this without needing a whole refactor of the entire fullscreen workflow is to update our checks (maybe even if we're not using prefixed APIs) to see whether document.fullscreenElement is the player or the tech elements.

[gkatsev]

@richardbushell hey, can you give #6009 a test? (for example, via https://deploy-preview-6009--videojs-docs.netlify.com/test-example/)

[richardbushell]

Apologies, I have been away on vacation, but saw that you were doing some work on this. I am back next week so hopefully will have a chance to test too. Thanks.

[richardbushell]

@gkatsev Yep, works as expected now, so LGTM.

[gkatsev]

Thanks for verifying!

[Snowbell92]

I am currently having the same issue, although strangely, this sandbox works for me. I am using videojs.7.6.6 straight from the CDN.

player.on('fullscreenchange', function () {
    if (this.isFullscreen()){
        console.log('fullscreen');
    } else {
        console.log('exit')
    }
})

In the console, I always see fullscreen if I press ESC, the button in the controlbar is working properly.

what's happening here? :/