Skip to content
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

fullscreenchange should behave like webkitfullscreenchange #89

Closed
mounirlamouri opened this issue May 18, 2017 · 7 comments
Closed

fullscreenchange should behave like webkitfullscreenchange #89

mounirlamouri opened this issue May 18, 2017 · 7 comments

Comments

@mounirlamouri
Copy link
Member

At the moment, my understanding is that fullscreenchange fires on the document while webkitfullscreenchange fires on the element that enters/exits fullscreen and bubbles up to the document.

I see the following reasons to keep the webkitfullscreenchange behaviour:

  • compatibility: likely, some websites that try to assume a non-prefix API will exist add the event listener to the element and simply change the event name. I definitively tried to do that once until @foolip told me I'm wrong;
  • simplicity/less error prone: it will increase the chances of developers to trigger behaviour change when a specific element goes fullscreen instead of anything inside the document (ie. it is easy to not check which element is fullscreen when the document becomes fullscreen).

The only reason I see to fire the event on the document is for the API to be wrapped around the Document interface instead of being spread around multiple interfaces but it's already the case with enterFullscreen() so maybe that battle is already lost?

@foolip
Copy link
Member

foolip commented May 18, 2017

I've lately been thinking that this is a good idea as well, for largely the same reasons.

It gets a bit weird when elements are removed or moved between documents, but it is possible to ensure that the event always fires on the same document as currently. The solution is to pick the event target right before dispatch, usually targeting the element, but targeting the original document if the element is no longer connected to it. There is something similar in https://w3c.github.io/webappsec-csp/#report-violation and when I discussed it with @mikewest I had in mind copying the model to Fullscreen.

@upsuper, Gecko is the only engine where the prefixed events target the document as the spec currently describes. There would be some compat risk both if you change it (obviously) and if you make prefixed and unprefixed events behave differently. WDYT?

We could go further and use add these as legacy event types in https://dom.spec.whatwg.org/#concept-event-listener-invoke, but I think that should be considered separately. There's simplicity in doing it, but would also make it possible to use requestFullscreen() + webkitfullscreenchange together, making it harder to know when one has successfully transitioned to the unprefixed APIs. I'm conflicted.

@foolip
Copy link
Member

foolip commented May 18, 2017

I wrote #90 to make it more concrete how this would work, for discussion.

@foolip
Copy link
Member

foolip commented May 18, 2017

I see that webkitfullscreenerror also targets the element and bubbles. If we change fullscreenchange then I suggest that we do so for fullscreenerror as well, using the same target-picking rules for all events.

@upsuper
Copy link
Member

upsuper commented May 18, 2017

I would never want to make prefixed and unprefixed events have different behavior. That sounds terribly complicated and I don't want to think about it unless it is proven to be necessary.

compatibility: likely, some websites that try to assume a non-prefix API will exist add the event listener to the element and simply change the event name. I definitively tried to do that once until @foolip told me I'm wrong;

I don't think compat risk is high for this, because Gecko always uses this model, for quite long, and I don't recall we received compat issue due to this difference. Also as far as I can see, people usually do polyfill for fullscreen so they handle them together, rather than listening different prefixed event on different target. So I don't think this may be a real compat risk.

@miketaylr, Blink and WebKit fires their prefixed fullscreen events (webkitfullscreenchange) on element, but Gecko fires that (mozfullscreenchange) on document. Have you seen compat issue due to this difference?

simplicity/less error prone: it will increase the chances of developers to trigger behaviour change when a specific element goes fullscreen instead of anything inside the document (ie. it is easy to not check which element is fullscreen when the document becomes fullscreen).

This is probably reasonable.

One question, though, where should the fullscreenchange event be fired for a change exiting fullscreen? Maybe the element which is exiting fullscreen? What if there are multiple elements all exiting fullscreen, because, for example, user preses esc, or an fullscreen iframe is kicked out from fullscreen by its parent?

Currently, firing fullscreenchange for exiting fullscreen on element may make sense because there is hierarchy restriction in all current implementations that an element can enter fullscreen only if the current fullscreen element is its ancestor or null. It means you only need to fire the event on the current fullscreen element even if you are exiting multiple.

But the latest spec has removed that restriction, which indicates that you may need to fire the event for every element, I mean every element in the old fullscreen stack, and all those events would bubble themselves up to the window. This would also be a behavior change for you.

Maybe entering fullscreenchange to element but exiting fullscreenchange to document? I guess this behavior may be confusing, though...

@upsuper
Copy link
Member

upsuper commented May 18, 2017

I have a feeling that fullscreen is somehow really a document-level thing, and developer ignoring that fact could probably cause things to be more error-prone, not less...

@foolip
Copy link
Member

foolip commented May 18, 2017

On exiting fullscreen in Blink, the fullscreen element is the target, even if that's not the only element in top layer. It's a very good point about the hierarchy restrictions, which Blink still has. If we get rid of those, then it'll be very hard to guarantee that each element gets a pair of fullscreenchange events.

At this point I would be fine with adding back the hierarchy restrictions. They seemed artificial after the fullscreen element stack was merged into top layer, but if they make the model simpler, then that's simpler :) (Another case I've noticed is that when exiting from nested fullscreen, there's actually no guarantee that the iframe is still the topmost fullscreen element in the ancestor document, but the spec doesn't handle that.)

@miketaylr
Copy link
Member

@miketaylr, Blink and WebKit fires their prefixed fullscreen events (webkitfullscreenchange) on element, but Gecko fires that (mozfullscreenchange) on document. Have you seen compat issue due to this difference?

Sorry, just found this notification. We don't know about any compat issues like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants