1188256 - Make Element.requestFullscreen() return a promise

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Xidorn Quan [:xidorn] UTC+11]

In the latest spec, those two functions should return a promise which will be resolved after fullscreenchange is dispatched, or rejected after fullscreenerror is dispatched.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

This is at the lowest priority, and we do not need to implement this before we unprefix the API, because Trident has been shipping unprefixed Fullscreen API without this feature [1], and given that fact, Blink may not put this in high priority either [2].

[1] https://msdn.microsoft.com/en-us/library/dn254939%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396
[2] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27674#c18

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

As a record, returning promise for requestFullscreen() is relatively, the Promise can just go to the FullscreenRequest object, and ensure to resolve / reject it before destructing the object. We need to take care of the timing, though, that it needs to be resolved in the next animation frame like the events, so we may need to change the mechanism of how we dispatch those events currently.

Returning promise from exitFullscreen() may need a bit more investigation.

Comment 3

[Andy Earnshaw]

I worry that leaving this too long will put the feature at risk of breaking compatibility. Although it seems unlikely that anyone would rely on requestFullscreen et al. returning undefined, the web is great at surprising you when you least expect it.

I also agree with Domenic's sentiment on the comment [1] directly below the one you linked.


[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27674#c19

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Having them returning promise is not in our list of features we want to ship with unprefixed Fullscreen API in our discussion with Blink and Edge, so I don't want to have it before we successfully ship the unprefixed API, which is the top priority.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

It seems Chrome is going to ship with this, so we may need to do this as well now.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Conceptually, it doesn't need to depend on bug 1375319, but code-wise there are lots of code changes overlap, so I'd just mark it blocking...

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1188256 part 1 - Move FullscreenRequest into a separate header and inline its methods. r=smaug

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1188256 part 2 - Make constructor of FullscreenRequest private and use create functions for building the object. r=smaug

This is done so that we can encapsulate more logic in this struct in following commits.

Depends on D5847

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1188256 part 3 - Remove Get-prefix for the getter methods of FullscreenRequest to reflect that they are not nullable. r=smaug

Depends on D5848

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1188256 part 4 - Move fullscreenerror dispatching into FullscreenRequest. r=smaug

Depends on D5850

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1188256 part 5 - Have nsIDocument::ApplyFullscreen take the ownership of FullscreenRequest. r=smaug

To make it clear that ApplyFullscreen is one of the places where
fullscreen requests are consumed.

Depends on D5851

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1188256 part 7 - Have Element.requestFullscreen return a Promise. r=smaug

Depends on D5852

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1188256 part 6 - Expose PromiseDebugging to plain mochitest via SpecialPowers. r=bzbarsky

Depends on D5852

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9009094 [details]
Bug 1188256 part 6 - Expose PromiseDebugging to plain mochitest via SpecialPowers. r=bzbarsky

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9009050 [details]
Bug 1188256 part 1 - Move FullscreenRequest into a separate header and inline its methods. r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9009051 [details]
Bug 1188256 part 2 - Make constructor of FullscreenRequest private and use create functions for building the object. r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9009053 [details]
Bug 1188256 part 3 - Remove Get-prefix for the getter methods of FullscreenRequest to reflect that they are not nullable. r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9009054 [details]
Bug 1188256 part 4 - Move fullscreenerror dispatching into FullscreenRequest. r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9009055 [details]
Bug 1188256 part 5 - Have nsIDocument::ApplyFullscreen take the ownership of FullscreenRequest. r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9009056 [details]
Bug 1188256 part 7 - Have Element.requestFullscreen return a Promise. r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9b30e7f7dc2
part 1 - Move FullscreenRequest into a separate header and inline its methods. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e922113d15da
part 2 - Make constructor of FullscreenRequest private and use create functions for building the object. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a24bd7445577
part 3 - Remove Get-prefix for the getter methods of FullscreenRequest to reflect that they are not nullable. r=smaug
https://hg.mozilla.org/integration/autoland/rev/933dd5b94277
part 4 - Move fullscreenerror dispatching into FullscreenRequest. r=smaug
https://hg.mozilla.org/integration/autoland/rev/9e246dedccbf
part 5 - Have nsIDocument::ApplyFullscreen take the ownership of FullscreenRequest. r=smaug
https://hg.mozilla.org/integration/autoland/rev/4f7527b669df
part 6 - Expose PromiseDebugging to plain mochitest via SpecialPowers. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/0665a323aec7
part 7 - Have Element.requestFullscreen return a Promise. r=smaug

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b9b30e7f7dc2
https://hg.mozilla.org/mozilla-central/rev/e922113d15da
https://hg.mozilla.org/mozilla-central/rev/a24bd7445577
https://hg.mozilla.org/mozilla-central/rev/933dd5b94277
https://hg.mozilla.org/mozilla-central/rev/9e246dedccbf
https://hg.mozilla.org/mozilla-central/rev/4f7527b669df
https://hg.mozilla.org/mozilla-central/rev/0665a323aec7

Comment 23

[Eric Shepherd [:sheppy]]

The Fullscreen API documentation has just been heavily overhauled, and this is now covered.

https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullScreen

Listed on Firefox 64 for developers

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Eric Shepherd [:sheppy] from comment #23)
> The Fullscreen API documentation has just been heavily overhauled, and this
> is now covered.
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullScreen
> 
> Listed on Firefox 64 for developers

There is one small mistake: with bug 1375319 implemented, the event (both fullscreenchange and fullscreenerror) will be dispatched to the element rather than the docuemnt, unless the element has been detached from the original document.

This applies to both requestFullscreen and exitFullscreen.

Comment 25

[Eric Shepherd [:sheppy]]

:xidorn -- Great catch. Thank you! I've updated that page, and hopefully all should be well now.