Closed Bug 1188256 Opened 5 years ago Closed 2 years ago

Make Element.requestFullscreen() return a promise

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files)

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.
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
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.
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
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.
Priority: -- → P5
It seems Chrome is going to ship with this, so we may need to do this as well now.
Blocks: 1269276
Priority: P5 → P2
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...
Depends on: 1375319
Assignee: nobody → xidorn+moz
Depends on: 1490956
Blocks: 1491212
Summary: Make Element.requestFullscreen() and Document.exitFullscreen() return a promise → Make Element.requestFullscreen() return a promise
This is done so that we can encapsulate more logic in this struct in following commits.

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

Depends on D5851
Attachment #9009056 - Attachment description: Bug 1188256 part 6 - Have Element.requestFullscreen return a Promise. r=smaug → Bug 1188256 part 7 - Have Element.requestFullscreen return a Promise. r=smaug
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.
Attachment #9009094 - Flags: review+
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.
Attachment #9009050 - Flags: review+
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.
Attachment #9009051 - Flags: review+
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.
Attachment #9009053 - Flags: review+
Comment on attachment 9009054 [details]
Bug 1188256 part 4 - Move fullscreenerror dispatching into FullscreenRequest. r=smaug

Olli Pettay [:smaug] has approved the revision.
Attachment #9009054 - Flags: review+
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.
Attachment #9009055 - Flags: review+
Comment on attachment 9009056 [details]
Bug 1188256 part 7 - Have Element.requestFullscreen return a Promise. r=smaug

Olli Pettay [:smaug] has approved the revision.
Attachment #9009056 - Flags: review+
Pushed by xquan@mozilla.com:
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
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
(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.
Flags: needinfo?(eshepherd)
:xidorn -- Great catch. Thank you! I've updated that page, and hopefully all should be well now.
Flags: needinfo?(eshepherd)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.