document.fullscreenElement in scroll event after entering fullscreen matters for compat

[foolip]

In https://crbug.com/680467 there is fallout from aligning the timing of document.fullscreenElement and fullscreenchange event with animation frames in https://crbug.com/402376.

On https://twitter.com/BCCI, if ones scrolls down a bit and plays a video in fullscreen, the iframe the video is in is removed from the document, leaving things in a broken-looking state. I speculate that this code tries to remove video that have scrolled out of view.

The crucial change is that in the scroll event fired after entering fullscreen, the fullscreen steps have not yet run, so to the page it looks like the page has simply been resized, causing a scroll.

I investigated current behavior:

• In Edge 14, the scroll event is fired after the fullscreenchange event, and so naturally the fullscreen element has already changed.
• In Firefox 54, the scroll event is fired before the fullscreenchange event when the fullscreen element has not yet changed. For some reason, it still does not get this bug on Twitter.
• In Chrome 55 (before my changes) and Safari 10, the scroll event is fired before the fullscreenchange event, and yet the fullscreen element has already changed.

I can see a few alternatives here, in rough order of preference:

• Apply the fullscreen state changes immediately on resize and delay only the events until animation frame timing.
• Tweak the order of scroll and fullscreen change events in https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model
• Introduce a document.pendingFullscreenElement that could be used by Twitter to avoid this problem.
• Ask Twitter to keep track of whether they are entering fullscreen themselves to avoid this.
• Suppress scroll events in fullscreen entirely.

@upsuper @jernoble @aliams

[upsuper]

Apply the fullscreen state changes immediately on resize and delay only the events until animation frame timing.

FWIW, this is what Gecko does, actually. I thought it is hard to distinguish between this behavior and the specced behavior. It seems I was wrong.

[upsuper]

BTW, if the element is already removed from the document, I suppose browser should exit fullscreen anyway... unless they just hide the element.

[foolip]

@upsuper, would you support changing the spec to be closer to Gecko? It is rather unfortunate to not make the state changes right before the events, but then the resize has already happened so there's no way to have everything happen at the very same time. (Unless, I suppose, every observable side effect of the resize is hidden until the resize event, which seems like a lot of effort to sort through.)

BTW, if the element is already removed from the document, I suppose browser should exit fullscreen anyway... unless they just hide the element.

Yeah, I recall you mentioned a change like this in Gecko, and I have a TODO in Blink around this. FWIW, it will be slightly easier to handle this situation at resize time, because at least one doesn't need to consider how things can change between then and when the animation frame task is run (in the current model).

[upsuper]

I would support that. I think it makes sense to have the document changes its state as soon as possible, hopefully before any script can get a chance to execute, so that we can limit the number of necessary reflow to the minimum.

In the HTML spec, scroll and resize are both dispatched before fullscreen steps, which is probably bad based on that... So yes, let's make it change the state immediately, and keep dispatching events (and resolving promise) in the animation frame task.

Not sure how spec should say about that, though.

[foolip]

@annevk, does a change like that make sense to you, at least enough that I should try a PR?

[foolip]

(I'd probably try to implement and update tests first.)

[annevk]

So we'd make changes to state just before the "rendering task" and then during the "rendering task" we invoke all the JavaScript callbacks? That sounds reasonable.

One problem that seems likely to come up again is that the infrastructure in HTML is on a per-document basis, whereas browsers do rendering on a per event loop basis, no? So shouldn't we adjust that too/first?

[foolip]

Yeah, that is a problem I've had to grapple with especially when exiting. I can use the phrasing "enqueue animation frame task for document" to make it clear what I intend, and then I can finally define that in HTML.

[foolip]

@upsuper, this issue is about the timing of fullscreenchange events, and some time in the past week I'm pretty sure I saw a comment for you, somewhere, about changes you were thinking about, to fire the events earlier than the spec currently says. I've searched and searched but can't find it.

Are there changes you think we should make timing-wise that would amount to not aligning the events with animation frame tasks?

I ask because I'm addition to https://crbug.com/680467 there was also a bit of trouble in https://crbug.com/679281 where the interaction with the Screen Orientation API was impacted, and it all boils down to timing.

@mounirlamouri FYI

[upsuper]

I think you mean this comment.

But it seems to me the previous plan in this issue was to make the document state change immediately and only delay the event dispatch?

[mounirlamouri]

To give more context about that interaction with Screen Orientation API: we implemented a feature in the Blink native controls to lock the orientation when the video goes fullscreen using the web events. @foolip told me that with the new event timing, that feature wouldn't work and we needed to have internal callbacks to keep the feature working. The way it was implemented in Blink is exactly the way a website would implement this feature which makes me think it might break some experiences. For example, there was a Web Fundamentals article advertising deeper interactions between fullscreen and orientation changes: https://developers.google.com/web/fundamentals/media/mobile-web-video-playback

[foolip]

@upsuper, that was it indeed, "I have a feeling that we may actually want fullscreen events to be dispatched at a very early stage, probably even before resize."

The problem has to do with restoring the original scale and scroll position after exiting fullscreen. This isn't something the spec covers, and I'd be surprised if it works exactly the same in all browsers. In any case, if the orientation lock changes are applied in reaction to fullscreenchange events, it means that when exiting fullscreen, the orientation will still be locked for a few more frames. The problem is the same as with this sequence of events, with a few seconds between each step:

1. Scroll and zoom in portrait
2. Enter fullscreen
3. Lock orientation to landscape
4. Exit fullscreen
5. Unlock the orientation

After this, will the scale and scroll position be the same as in the beginning? If yes, then it must mean that unlocking the screen orientation is what restores the pre-fullscreen state, i.e. the APIs are entangled in a way that isn't suggested by either spec. Maybe they should be?

Or, possibly, some change to the timing would happen to fix this problem.

[foolip]

@mounirlamouri, #92 fixes my original problem in this issue but not the Screen Orientation API issues.

The earliest possible time that one could fire the events I think is in a microtask following the resize, but I don't think that'd really solve anything. The orientation change itself can involve an animation and is in any event out-of-process in some implementations, so it couldn't be guaranteed to happen before the layout at which the original scale and scroll position should be restored.

Maybe a good start would be to write (manual) testharness.js tests for the expected behavior that are as unforgiving timing-wise as possible. Is that something you have the bandwidth for?