Make Element.requestFullscreen() return a promise

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(7 attachments)

46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
bzbarsky
: review+
Details | Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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
(Assignee)

Comment 2

2 years ago
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

2 years ago
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
(Assignee)

Comment 4

2 years ago
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.

Updated

10 months ago
Priority: -- → P5
(Assignee)

Comment 5

8 months ago
It seems Chrome is going to ship with this, so we may need to do this as well now.
Blocks: 1269276
Priority: P5 → P2
(Assignee)

Comment 6

7 months ago
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)

Updated

7 months ago
Assignee: nobody → xidorn+moz
(Assignee)

Updated

7 months ago
Depends on: 1490956
(Assignee)

Updated

7 months ago
Blocks: 1491212
(Assignee)

Updated

7 months ago
Summary: Make Element.requestFullscreen() and Document.exitFullscreen() return a promise → Make Element.requestFullscreen() return a promise
(Assignee)

Comment 8

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

Depends on D5847
(Assignee)

Comment 11

7 months ago
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+

Comment 21

7 months ago
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
(Assignee)

Comment 24

6 months ago
(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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.