I have reviewed https://github.com/web-platform-tests/wpt/blob/master/fullscreen/model/move-fullscreen-element.html and agree it's an appropriate test for this change. https://wpt.fyi/results/fullscreen/model/move-fullscreen-element.html?label=experimental&label=master&aligned shows it passing in Chrome/Edge/Firefox already.

However, I think the reason the test passes in Firefox is different. If the comment here is correct, Gecko fully exits fullscreen when the fullscreen element is removed.

The difference should be observable in a test written for #133.

Per #217 (comment) I think this is the right fix. I've sent #244 to fix Bikeshed errors so that this will build cleanly.

Thanks! I appreciate it.

Also note that there is a remaining question mark in #133 that was filed while this very step was discussed in #128.

Hmm, I guess I don't feel too strongly. I just went with whatever Chrome shipped to fix the YouTube regression. Gecko's approach also works too.

I have reviewed https://github.com/web-platform-tests/wpt/blob/master/fullscreen/model/move-fullscreen-element.html and agree it's an appropriate test for this change. https://wpt.fyi/results/fullscreen/model/move-fullscreen-element.html?label=experimental&label=master&aligned shows it passing in Chrome/Edge/Firefox already.

However, I think the reason the test passes in Firefox is different. If the comment here is correct, Gecko fully exits fullscreen when the fullscreen element is removed.

The difference should be observable in a test written for #133.

I don't currently have time to do this, but I assume we could make a variant of the test that pushes 2 things in fullscreen, then moves one of them to see the difference?

I don't currently have time to do this, but I assume we could make a variant of the test that pushes 2 things in fullscreen, then moves one of them to see the difference?

I started writing such a test, but found that we already have it tested for removing both positions in the stack in a nested fullscreen case:

https://wpt.fyi/results/fullscreen/model/remove-first.html?label=master&label=experimental&aligned
https://wpt.fyi/results/fullscreen/model/remove-last.html?label=master&label=experimental&aligned

@nt1m the failures in Safari seem unrelated to the problem at hand. Is there some tweak to the tests that would make them run correctly and reveal what the current consensus behavior is?

I don't currently have time to do this, but I assume we could make a variant of the test that pushes 2 things in fullscreen, then moves one of them to see the difference?

I started writing such a test, but found that we already have it tested for removing both positions in the stack in a nested fullscreen case:

https://wpt.fyi/results/fullscreen/model/remove-first.html?label=master&label=experimental&aligned https://wpt.fyi/results/fullscreen/model/remove-last.html?label=master&label=experimental&aligned

@nt1m the failures in Safari seem unrelated to the problem at hand. Is there some tweak to the tests that would make them run correctly and reveal what the current consensus behavior is?

I think I fixed most of the WPT failures recently, but it hasn't reached STP.

Might be more useful to check these results: https://searchfox.org/wubkat/search?q=imported%2Fw3c%2Fweb-platform-tests%2Ffullscreen%2Fmodel%2Fremove-&path=&case=false&regexp=false

Thanks @nt1m! I've looked at those results, and also run the tests manually in different browsers.

Chrome passes both remove-first.html and remove-last.html.

Firefox passes remove-first.html but fails remove-last.html. The failures is on this assert, because document.fullscreenElement was synchronously changed to null instead of the first element being left as the fullscreen element. I assume this is because of the synchronous "fully exit fullscreen" call linked in #243 (comment).

Safari stable fails both tests, but per the results @nt1m shared the remove-first.html failures has been fixed. I can't tell from the failure message which exact assert failed. I've sent web-platform-tests/wpt#50625 to make the tests more debuggable.

Since this PR fixes a very real problem and matches an existing implementation (Chromium) I'll go ahead and merge it now. In #133 I'll summarize the current situation and things we might still change on top of this PR.