Closed Bug 777236 Opened 12 years ago Closed 11 years ago

Implement the "fullscreen" app manifest attribute for web apps on desktop

Categories

(Firefox Graveyard :: Webapp Runtime, defect, P1)

defect

Tracking

(firefox16 wontfix)

RESOLVED FIXED
Firefox 26
Tracking Status
firefox16 --- wontfix

People

(Reporter: jsmith, Assigned: marco)

References

Details

(Keywords: dev-doc-needed, productwanted)

Attachments

(1 file, 3 obsolete files)

See https://developer.mozilla.org/en/Apps/Manifest under "fullscreen." An app can specify an optional parameter in the manifest to specify whether the app should launch in full-screen mode or not. True indicates it does, false indicates it does not.
Version: 16 Branch → Trunk
Priority: -- → P1
This would be very useful for e.g. games, full-screen text editors, video players, etc.
So, I was wondering how this feature is supposed to work. The details I've been able to find are pretty scarce.

Should an app with the fullscreen attribute have the permission to go fullscreen whenever it wants? Should the webapp runtime manage the passage from fullscreen mode to windowed mode, or should the app itself do that?

Also, on desktop we probably want to allow apps to use the Pointer Lock API when they're in fullscreen mode.
Also the documentation https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#fullscreen differs from the specification http://www.w3.org/2008/webapps/manifest/#fullscreen-member.
In the documentation, the fullscreen attribute is a string.
In the specification, the fullscreen attribute is a boolean (and, per http://people.mozilla.org/~jorendorff/es6-draft.html#sec-9.1.2, something like "false" should be considered as the boolean true).
Attached patch fullscreen_webapps (obsolete) — Splinter Review
Adding Vishy to get some product management perspective here
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attached patch fullscreen_webapps (obsolete) — Splinter Review
Attachment #779528 - Attachment is obsolete: true
Attachment #789200 - Flags: review?(myk)
Comment on attachment 789200 [details] [diff] [review]
fullscreen_webapps

Review of attachment 789200 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Marco Castelluccio [:marco] from comment #2)
> Should an app with the fullscreen attribute have the permission to go
> fullscreen whenever it wants?

This is a Mounir/Travis question!


> Should the webapp runtime manage the passage
> from fullscreen mode to windowed mode, or should the app itself do that?

My instinct says that the runtime should manage the transition, but the app should manage the interface, i.e. the visual affordances (menu items, buttons) and keyboard shortcuts that trigger entering/exiting fullscreen mode.

But there's precedent for the runtime specifying this for all apps; the runtime currently provides a menubar with a default set of platform-standard menu options for all apps.  And there's value for users in having consistent affordances.

But ultimately it's a product call, so I'm looking for Vishy's feedback.  Also, Travis has been surveying other runtimes and may have useful findings to consider.


> Also, on desktop we probably want to allow apps to use the Pointer Lock API
> when they're in fullscreen mode.

Makes sense to me.

The other thing we'll want is for the app to enter Mac-native fullscreen mode on a Mac, which this patch doesn't yet do.

::: webapprt/content/webapp.js
@@ +58,5 @@
>    // This doesn't capture clicks so content can capture them itself and do
>    // something different if it doesn't want the default behavior.
>    gAppBrowser.addEventListener("click", onContentClick, false, true);
> +
> +  if (Boolean(WebappRT.config.app.manifest.fullscreen) && WebappRT.config.app.manifest.fullscreen != "false") {

It's unclear what type the manifest actually calls for, since the documentation is ambiguous <https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#fullscreen>.

But the value of this property should either be a string ("true", "false") or a boolean (true, false), so it should be simpler than this to test it.

cc: Mounir and Travis for clarification about the intended type of the property.
Attachment #789200 - Flags: review?(myk)
Attachment #789200 - Flags: review-
Attachment #789200 - Flags: feedback?(vkrishnamoorthy)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #8)
> (In reply to Marco Castelluccio [:marco] from comment #2)
> > Should an app with the fullscreen attribute have the permission to go
> > fullscreen whenever it wants?
> 
> This is a Mounir/Travis question!

Worth considering that at the moment apps can go fullscreen whenever they want, with or without the fullscreen manifest attribute, there isn't any permission to ask.

> > Also, on desktop we probably want to allow apps to use the Pointer Lock API
> > when they're in fullscreen mode.
> 
> Makes sense to me.

I've found out that in fullscreen mode apps can use the pointer lock API without asking for permissions (even in Firefox).

> The other thing we'll want is for the app to enter Mac-native fullscreen
> mode on a Mac, which this patch doesn't yet do.

I haven't tested yet on Mac, but the Fullscreen API should to that automatically.

> It's unclear what type the manifest actually calls for, since the
> documentation is ambiguous
> <https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#fullscreen>.
> 
> But the value of this property should either be a string ("true", "false")
> or a boolean (true, false), so it should be simpler than this to test it.

According to http://www.w3.org/2008/webapps/manifest/#fullscreen-member it is a value that should be converted to a boolean. In Gaia I've seen both boolean and strings, so, even if according to the specs "false" is true, we probably want to consider "false" as false.
Fullscreen request is always given to installed applications, see:
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#10380

The rationale behind this being that installing an application is an action weighting more trust than simply navigating to a page.
(In reply to Marco Castelluccio [:marco] from comment #9)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #8)
> > (In reply to Marco Castelluccio [:marco] from comment #2)
> > > Should an app with the fullscreen attribute have the permission to go
> > > fullscreen whenever it wants?
> > 
> > This is a Mounir/Travis question!
> 
> Worth considering that at the moment apps can go fullscreen whenever they
> want, with or without the fullscreen manifest attribute, there isn't any
> permission to ask.

Per Mounir in comment 10: all installed apps should be able to go fullscreen whenever they want without the runtime first asking the user's permission.


> I've found out that in fullscreen mode apps can use the pointer lock API
> without asking for permissions (even in Firefox).

It Just Works™ FTW!


> > The other thing we'll want is for the app to enter Mac-native fullscreen
> > mode on a Mac, which this patch doesn't yet do.
> 
> I haven't tested yet on Mac, but the Fullscreen API should to that
> automatically.

In my testing, it doesn't happen.  When Firefox goes fullscreen, it does so via a graceful animated transition and then becomes a "space" <http://en.wikipedia.org/wiki/Spaces_%28software%29>.  But when an app in the runtime does so, it abruptly takes over the whole screen, and it doesn't become a space.


> > It's unclear what type the manifest actually calls for, since the
> > documentation is ambiguous
> > <https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#fullscreen>.
> > 
> > But the value of this property should either be a string ("true", "false")
> > or a boolean (true, false), so it should be simpler than this to test it.
> 
> According to http://www.w3.org/2008/webapps/manifest/#fullscreen-member it
> is a value that should be converted to a boolean. In Gaia I've seen both
> boolean and strings, so, even if according to the specs "false" is true, we
> probably want to consider "false" as false.

*sigh*, that's unfortunate, but we should do whatever Gaia does.
FWIW - bug 903434 is on file for fixing the fullscreen manifest property in Gaia to use direct booleans consistently.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #11) 
> In my testing, it doesn't happen.  When Firefox goes fullscreen, it does so
> via a graceful animated transition and then becomes a "space"
> <http://en.wikipedia.org/wiki/Spaces_%28software%29>.  But when an app in
> the runtime does so, it abruptly takes over the whole screen, and it doesn't
> become a space.

I've a new patch where this is fixed, it's really easy, we just need to add the fullscreenbutton
attribute to the xul window: http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaWindow.mm#1936

> > According to http://www.w3.org/2008/webapps/manifest/#fullscreen-member it
> > is a value that should be converted to a boolean. In Gaia I've seen both
> > boolean and strings, so, even if according to the specs "false" is true, we
> > probably want to consider "false" as false.
> 
> *sigh*, that's unfortunate, but we should do whatever Gaia does.

(In reply to Jason Smith [:jsmith] from comment #12)
> FWIW - bug 903434 is on file for fixing the fullscreen manifest property in
> Gaia to use direct booleans consistently.

We've had an inconsistent documentation for over a year, but the problem would only be there with apps that had the fullscreen attribute defined as "false". So I think we can forget about it and just use Boolean(manifest.fullscreen).

There's only one problem left to fix. I'm requesting fullscreen on the <browser> element, so apps can't cancel fullscreen mode because they instead use their document element.
If I request it on the app document element, the request fails because there's no window focused.
Attached patch fullscreen_webapps (obsolete) — Splinter Review
Attachment #789200 - Attachment is obsolete: true
Attachment #789200 - Flags: feedback?(vkrishnamoorthy)
Attachment #792951 - Flags: review?(myk)
Comment on attachment 792951 [details] [diff] [review]
fullscreen_webapps

Review of attachment 792951 [details] [diff] [review]:
-----------------------------------------------------------------

::: webapprt/content/webapp.js
@@ +73,5 @@
> +// Fullscreen handling.
> +
> +function enterFullScreen() {
> +  // We call mozRequestFullScreen here so that the app is directly executed
> +  // in fullscreen mode.

It isn't clear what this means by "directly executed".  Can you elaborate?
(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> Comment on attachment 792951 [details] [diff] [review]
> fullscreen_webapps
> 
> Review of attachment 792951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: webapprt/content/webapp.js
> @@ +73,5 @@
> > +// Fullscreen handling.
> > +
> > +function enterFullScreen() {
> > +  // We call mozRequestFullScreen here so that the app is directly executed
> > +  // in fullscreen mode.
> 
> It isn't clear what this means by "directly executed".  Can you elaborate?

I'll fix the comment, I meant that this way the app window goes in fullscreen mode as soon as it is loaded and not after the <browser> content is loaded.
Comment on attachment 792951 [details] [diff] [review]
fullscreen_webapps

Review of attachment 792951 [details] [diff] [review]:
-----------------------------------------------------------------

> I'll fix the comment, I meant that this way the app window goes in
> fullscreen mode as soon as it is loaded and not after the <browser> content
> is loaded.

Ah, that makes sense.  Yes, please do update the comment with this info!

Otherwise looks great, r=myk.

(I expect we'll want to persist the state of fullscreen mode across sessions, so apps start in whatever mode they were previously in, regardless of the value of the fullscreen manifest field.  Otherwise, users who don't want an app to be in fullscreen mode will have to undo it every time they start the app, which will be annoying.  But we can do that in a separate bug.)
Attachment #792951 - Flags: review?(myk) → review+
Carrying r+.
Attachment #792951 - Attachment is obsolete: true
Attachment #795559 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/002ded1402a8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: