Fire events at elements by default, falling back to documents

[foolip]

Fixes #89.

Tests: web-platform-tests/wpt#6017

---

Preview | Diff

[annevk]

Should these be composed? I guess not? We sort of decided to only do that for UI events iirc.

[foolip]

If not then the events won't reach the document, right?

[annevk]

When dispatched inside a shadow tree, yes.

@scheib you might also be interested in this change proposal as Pointer Lock mimics Fullscreen quite a bit.

cc @hayatoito

[foolip]

@annevk, is there a generic reason to prefer not using composed events?

And cc @TakayoshiKochi who's done work in Blink to make fullscreen work with shadow trees.

[annevk]

I don't think we ever arrived at a hard-and-fast rule that works. It might actually make sense here to "escape" the shadow tree though as it affects the whole document. It's not signifying a local change.

That does mean you need to change the PR, as by default composed is false.

[foolip]

I've pushed a new commit that gives both events the same treatment. I'll now see about tests for that.

[foolip]

This and the tests are ready for review.

I was going to add onfullscreenchange and onfullscreenerror to GlobalEventHandlers, but that doesn't add them to ShadowRoot. Other such cases in HTML also have event handler content attribute, but requestFullscreen() isn't limited to HTML elements.

I went with only adding them to Element, which matches the webkit-prefixed ones.

[annevk]

Should we note that this is intentionally different from DocumentAndElementEventHandlers? Perhaps in a comment in the source?

[upsuper]

If we want to do this, we should pull back hierarchy restriction first.

Before we do this, I'd like to ask if anyone recalls why it is currently fired on the document?

It seems to me the initial proposal from roc was to dispatch it to the element, and I don't find any mention of why Gecko doesn't follow that way in the bug of the initial impl of this API. Probably the current spec largely follows how Gecko is implemented, and it was just a history mistake to fire on document?

[foolip]

If we want to do this, we should pull back hierarchy restriction first.

Sure, I can do that.

Before we do this, I'd like to ask if anyone recalls why it is currently fired on the document?

I imagine @rocallahan is busy debugging, but maybe he still remembers?

[rocallahan]

I don't remember. Chris Pearce might know.

It might have just been to simplify things by avoiding the special-casing for elements not in a document.

[foolip]

@cpearce, do you have any additional insights here?

I think that even with the original reasoning shrouded in mystery, we should be able to move forward here, as long as @upsuper things a new model could work out for Gecko. (No guarantees possible, if things break left and right we could go back to targeting the document again.)

[foolip]

If we want to do this, we should pull back hierarchy restriction first.

That's #91, but does it need to block this change? I think that with or without hierarchy restrictions, fullscreenchange events can bubble through elements that both are and are not in top layer, so one has to look at the event target. I'm in no hurry to get rid of the hierarchy restrictions in Blink, but would it be unworkable to not have them? (As you can tell, I'm conflicted.)

[upsuper]

That's #91, but does it need to block this change?

Given your change here, probably no. I was thinking about firing an event for each fullscreen element in top layer for exiting fullscreen, which would be a breaking change. But it seems your change here only fires event for the top fullscreen element, which is fine with or without hierarchy restriction, although without hierarchy restriction, you would not be able to guarantee to have all fullscreen elements get the event.

[cpearce]

@cpearce, do you have any additional insights here?

I think it's as @rocallahan mentions; we exit fullscreen when the fullscreen element is removed from the document, and if the target for fullscreenchange is the fullscreen element then the event can't bubble, so you need to special case (as I see you have in your PR).

[foolip]

Thanks @cpearce!

@upsuper @annevk, at your convenience, could you review this and the tests?

[upsuper]

Looks good to me.

I especially wanted to check whether it would work when we are removing element, and it seems even if an element is being removed, it can still be the fullscreen element of the document until the window gets unfullscreened, which is probably fine, so that the rest steps of unfullscreen document would work as normal.

@annevk do you see any other issue in this change?

[scheib]

Thanks for pinging me re: Pointer Lock, @annevk . I don't see a problem with this change, and agree that Pointer Lock should follow. I've filed an issue to pointer lock w3c/pointerlock#21

[foolip]

I have another PR that's based on this, so I'll go ahead and merge, and make sure tests are merged soonish as well.