@upsuper, I wrote this tests because this test began failing when working on the top layer rewrite in Chromium, and it turns out that Firefox already behaves this way.

Do the comments here match the reason it behaves like this in Firefox? I think it's quite surprising, but to fix it would require one of these:

• Making "exit fullscreen" keep track of which element it is exiting for, and doing nothing if it's no longer the fullscreen element in the async part.
• Change the removing steps so that they don't invoke "exit fullscreen" at all, but instead just resize if necessary and enqueue an event.
• Others?

WDYT?

This is also rather odd, because first is the fullscreen element when the async steps run, that's where the event will be fired, even though it's only the fullscreen element for a brief period.

@upsuper, I noticed this test is still wrong. If the fullscreen element is null, as it is in Firefox Nightly here, then no fullscreenchange event should be fired, because of "If doc’s fullscreen element is null, then resolve promise with undefined and terminate these steps."

So we need to change either the spec of Firefox in some way. In your implementation, what is it that causes the event to be fired?

So in Firefox, when any of the fullscreen element is unbound from the tree, we exit fullscreen and queue events synchronously, then force the window to restore asynchronously (as it needs to be).

I guess either way is fine to me, although I'm not very motivated to change code here, because removing fullscreen element should be considered as an error case, so the bottom line is we should exit fullscreen safely, and I don't think we should expect any legitimate content to rely on behavior here.

@upsuper, by "exit fullscreen ... synchronously", do you mean that some of the steps of https://fullscreen.spec.whatwg.org/#exit-fullscreen are run? In https://gist.github.com/foolip/47ee26fa3d6463f5ca3edc364d915beb it looks like you fully exit fullscreen even if you were in nested fullscreen, while Chrome (before recent changes), Safari and Edge do not. (@aliams @jernoble in case I'm testing it wrong.)

I could live with what the spec already says, but that would require a change to Gecko. Changing the spec to say something more similar to Gecko would also be fine. Since this isn't a terribly important issue, if nobody feels strongly, then maybe leave the spec alone and make these test changes. @upsuper?

By doing that synchronously, I mean we reset the document state to non-fullscreen immediately (fullscreenElement becomes null, and :fullscreen matches nothing) without waiting for the window to change.

I'm fine with the spec way, but as I mentioned before, since I don't think this is important, I'm not going to change that anytime soonish... If someone submits patch for that, we may accept. But I myself don't have much time working on this recently.

Thanks, I think I understand how to spec something similar then. I'll decide what to do before landing the top layer change in Chromium, which is what will affect these tests, or comment back here if I decide on no change. Can you review so the spec and tests come into agreement first?

This sounds a bit counter-intuitive.

Assuming we have three fullscreen elements in the stack, e.g. #first, #second, and #third. Given the description here, I guess when #third is removed from the document, document.fullscreenElement becomes #second synchronously, and then an "exit fullscreen" is triggered from that removal and #second is popped out, and document.fullscreenElement becomes #first? Is my understanding correct?

If it is really hard to handle it intuitively, I'd rather we just fully exit fullscreen when the fullscreen element is removed, which would at least look more consistent and less surprising.

Yep, because we collect documents to unfullscreen twice, and one time is after the original fullscreen element has been removed from top layer, removing an element will effectively exit two levels of fullscreen. This is quite related to whatwg/fullscreen#105.

I think that either fully exiting fullscreen or only calling "collect documents to unfullscreen" once are the most reasonable choices here. WDYT?

I guess I've forgot some of the context of this PR. IIUC, we are discussing about updating the document state (e.g. fullscreen element, selector matching, etc.) synchronously in the removing steps of element, and that isn't part of the current spec at the moment?

If that's correct, I guess doing "fully exit fullscreen" doesn't help either, because that relies on "exit fullscreen a document" as well. It is also not clear to me how having "collect documents to unfullscreen" called once helps either. That function only collects document, and you may still end up popping an additional element in the document (because the original fullscreen element has been remove synchronously). Actually, how would this all ensure that we dispatch the fullscreen event for the element removal...

That wasn't the original context, but yes, now this is supposed to match whatwg/fullscreen#102 which you've already approved :)

What I would suggest here is to merge this and whatwg/fullscreen#102, and then to treat whatwg/fullscreen#105 as a regression from that in need of fixing.

You're right that "fully exit fullscreen" wouldn't fix whatwg/fullscreen#105.

The way that "collect documents to unfullscreen" could help is if we record both what documents to exit and what the fullscreen element is. If, after actually exiting fullscreen, that's no longer the fullscreen element, then we can still fire the fullscreenchange event at the document. Some finesse required, but I have something of this rough shape in an experimental patch for Blink. I'd like to handle it separately though, this set of test changes is mostly independent.

What I would suggest here is to merge this and whatwg/fullscreen#102, and then to treat whatwg/fullscreen#105 as a regression from that in need of fixing.

This makes sense, but... it doesn't really match the spec after whatwg/fullscreen#102 due to whatwg/fullscreen#105. It seems many of the fullscreenchange listener in the tests wouldn't be triggered with whatwg/fullscreen#102.

I guess... we can live with this mismatch between test and spec as far as we can resolve whatwg/fullscreen#105 soonish?

Yeah, I need to resolve whatwg/fullscreen#105 before I can land the top layer changes at all in Chromium.

Let me walk through the tests to see which ones don't match the spec now but will soon, and I'll rename those to tentative. Writing tests to match the somewhat broken state doesn't seem meaningful.

Many test which expect to receive fullscreenchange event after removing the last fullscreen element probably don't match the spec. And I agree that writing tests to match a broken spec state doesn't make much sense.