Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Description • 11 years ago

The DOM/HTML fullscreen API doesn't work in metro.

To be clear, this is different from bug 842130 which refers to getting fullsceen in the video controls working. This bug refers to getting the fullscreen DOM/HTML API working in general HTML content/javascript.

STR:
1. Load http://pearce.org.nz/f in Metrofox
2. Observe the text "Fullscreen API is either not present or disabled in your browser" at the top of page, that means that HTMLDocument.mozFullScreenEnabled is returning false. It should return true.
3. Click the "Fullscreen" in the page, or one of the "Request fullscreen" in the iframes, and note that the page doesn't enter fullscreen. Clicking on the "Video full screen" button does not make the video fullscreen.

Expected result:
1. The text "Fullscreen API is either not present or disabled in your browser" should not appear; HTMLDocument.mozFullScreenEnabled should return true.
2. Clicking on the "Fullscreen" button in the page should make the page fullscreen. Clicking on either of the "Request fullscreen" buttons in the iframes should make those iframes fullscreen. Clicking on the "Video full screen" button should make the video fullscreen.

As I see it, there are four issues we need to address:

*Firstly*, the fullscreen API is pref'd off by default in all.js, it needs to be pref'd on on a product-by-product basis.

*Secondly*, once we enable the fullscreen API, enter fullscreen is still stuffed up. It seems the chrome document can't handle its child documents being made fullscreen.

*Thirdly*, we actually need to decide how fullscreen is going to affect chrome; on desktop, B2G and Android we hide the browser chrome when content enters fullscreen. In the case of Metrofox however, I think that we should not change the behaviour of the browser chrome when content is fullscreen. In Metrofox the content area's <browser> is already effectively fullscreen, and when you want to access the chrome, you swipe up or down and the chrome slides in over-top of content; i.e. making chrome visible doesn't cause content to resize. I think keeping this behaviour while fullscreen makes perfect sense.

In addition if we keep the browser chrome's behaviour the same we don't need to bother with the approval dialog upon entering fullscreen, since I don't think it's possible for a HTML fullscreen app to impersonate browser chrome as browser chrome appears above content upon swipe-in in the z-order. And anyway, a non-fullscreen app has exactly the same ability as a fullscreen app to impersonate browser chrome in Metrofox, so I don't think we need to bother with the fullscreen approval warning/dialog.

Also, if we prevent fullscreen requests in content propagating up into the chrome document (fullscreen requests cause all ancestor frames to enter fullscreen, it's how the fullscreen API works), then the second issue above goes away, i.e. fullscreen works as expected.

*Fourthly*, we also need to decide how to provide a way to enable the user to exit fullscreen. If the web site doesn't provide a way to exit fullscreen, there must still be a way provided by our UI to exit fullscreen. I'm not sure what is the best way to do this...

How about we have the browser back button exit fullscreen if HTML content is in fullscreen mode? Another idea is having swiping down the tab bar causing us to exit fullscreen, though I think the using the back button makes more sense, but is perhaps less discoverable.

So those are the issues I'm aware of. I'm happy to work on this, as I did the fullscreen implementation on desktop and B2G, and I have a Win8 touch tablet, and I'm familiar with the Gecko code so I know what Gecko changes will need to be made to make Metrofox Chrome not be affected by fullscreen.

	Jim Mathies [:jimm]
	Comment 1 • 11 years ago

> *Secondly*, once we enable the fullscreen API, enter fullscreen is still
> stuffed up. It seems the chrome document can't handle its child documents
> being made fullscreen.

I'm not sure what the fs api does, but if it is trying to create a xul window on top of the browser window, that will fail. Metrofx is a single window app. So the video will probably have to be in a xul overlay of some sort defined in browser.xul.
 
> *Thirdly*, we actually need to decide how fullscreen is going to affect
> chrome; on desktop, B2G and Android we hide the browser chrome when content
> enters fullscreen. In the case of Metrofox however, I think that we should
> not change the behaviour of the browser chrome when content is fullscreen.
> In Metrofox the content area's <browser> is already effectively fullscreen,
> and when you want to access the chrome, you swipe up or down and the chrome
> slides in over-top of content; i.e. making chrome visible doesn't cause
> content to resize. I think keeping this behaviour while fullscreen makes
> perfect sense.

I like this as well. it should just expand out to cover the existing tab. The chrome still invokes as usual.

> In addition if we keep the browser chrome's behaviour the same we don't need
> to bother with the approval dialog upon entering fullscreen, since I don't
> think it's possible for a HTML fullscreen app to impersonate browser chrome
> as browser chrome appears above content upon swipe-in in the z-order. And
> anyway, a non-fullscreen app has exactly the same ability as a fullscreen
> app to impersonate browser chrome in Metrofox, so I don't think we need to
> bother with the fullscreen approval warning/dialog.

agreed.

> *Fourthly*, we also need to decide how to provide a way to enable the user
> to exit fullscreen. If the web site doesn't provide a way to exit
> fullscreen, there must still be a way provided by our UI to exit fullscreen.
> I'm not sure what is the best way to do this...
> 
> How about we have the browser back button exit fullscreen if HTML content is
> in fullscreen mode? Another idea is having swiping down the tab bar causing
> us to exit fullscreen, though I think the using the back button makes more
> sense, but is perhaps less discoverable.

The lower app bar is context sensitive, so swiping in fullscreen should bring up an app bar that has one option, "exit fullscreen video". 

> So those are the issues I'm aware of. I'm happy to work on this, as I did
> the fullscreen implementation on desktop and B2G, and I have a Win8 touch
> tablet, and I'm familiar with the Gecko code so I know what Gecko changes
> will need to be made to make Metrofox Chrome not be affected by fullscreen.

We would love the help!

Note Fryn is working on overhauling the app bar/nav bar currently as we'll be changing up the look and feel a bit. You might chat with him about potential app bar behavior.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 2 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #1)
> > *Secondly*, once we enable the fullscreen API, enter fullscreen is still
> > stuffed up. It seems the chrome document can't handle its child documents
> > being made fullscreen.
> 
> I'm not sure what the fs api does, but if it is trying to create a xul
> window on top of the browser window, that will fail. Metrofx is a single
> window app. So the video will probably have to be in a xul overlay of some
> sort defined in browser.xul.

The fullscreen API doesn't create a XUL window, it works by setting the following styles on the fullscreen element, and on all containing frames (the <iframe>, <browser> elements, etc) in ancestor documents:

*|*:fullscreen {
  position:fixed;
  top:0; right:0; bottom:0; left:0;
  margin:0;
  width:100%;
  height:100%;
}

So basically it stretches the element requesting fullscreen to the max size of its container, and stretches the containers in parent documents to the max size of their containers, and so on. On desktop we also make the XUL window sizemode=fullscreen, thus the element that requests fullscreen becomes fullscreen.

> Note Fryn is working on overhauling the app bar/nav bar currently as we'll
> be changing up the look and feel a bit. You might chat with him about
> potential app bar behavior.

Cool will do. Is the app bar/nav overhaul work tracked in a bug somewhere? I'll get started with everything except the exit fullscreen UI.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 3 • 11 years ago

* Toggle prefs to enable fullscreen API in metrofox, but to not require approval (see discussion above regarding this).
* Add pref to tell Gecko to not apply :-moz-full-screen style rules to elements in the chrome document, enable this in metrofox. This is so that our chrome/UI in metrofox can still be available when the user swipes from top/bottom.

Requesting review from bz for the content/ changes, jimm for browser/ changes.

We still need an exit fullscreen option in the UI, as discussed above.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 4 • 11 years ago

Oops, my Preferences::AddBoolVarCache() was totally wrong... ;)

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 5 • 11 years ago

Comment on attachment 716856 [details] [diff] [review]
Patch: Enable fullscreen API on Metrofox

Nuts. We crash with this patch if we make multiple tabs fullscreen and then cancel fullscreen in one of the tabs. 

We can't just use the patch in bug 805613 (which is *finally* getting a sec review on Monday!) either to handle this, as it assumes that each fullscreen branch in the doctree is in a separate doctree, i.e. in separate windows.

I'll have to rework this, probably rebasing it on top of the patch in bug 805613.

Sorry for review spam.

	Jim Mathies [:jimm]
	Comment 6 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #2)
> Cool will do. Is the app bar/nav overhaul work tracked in a bug somewhere?
> I'll get started with everything except the exit fullscreen UI.

bug 835623

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 7 • 11 years ago

This turned out to be a little more complicated than I thought. ;)

Our fullscreen code assumes that only one branch in a doctree (rooted at the chrome document) is fullscreen, i.e. it assumes that the chrome document is the root of the branch that is fullscreen and that it doesn't have multiple fullscreen children. But we want the chrome document to be able to have multiple fullscreen children in Metrofox, so that multiple tabs can be fullscreen and switched between. We also want the container element in the chrome docunent which contains the content document which is fullscreen to not be made fullscreen so that the chrome document's UI is still visible.

I resolved this issue by adding a pref to make fullscreen content-only in Metrofox, i.e. don't allow the chrome document to enter fullscreen. We then can maintain the invariant that no fullscreen document has more than one fullscreen child document, so the code needs minimal changes to keep working (basically I just need to change the code that determines the root of the branch which is fullscreen to instead retrieve the ancestor which is the child of the chrome document, rather than the chrome document itself).

Making this change has a consequence that we can't rely on asking the chrome document whether it's fullscreen to determine whether a child document is fullscreen, so I needed to change the ESC keypress handling code in nsPresShell.cpp a little to reflect that.

When we implement the app bar button to exit fullscreen, we'll be able to tell whether we should show the "exit fullscreen" button by checking if the focused tab's contentDocument.mozFullScreenElement!=null.

r? bz for content changes
r? jimm for enabling this in Metrofox.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

Comment on attachment 718672 [details] [diff] [review]
Patch: Make fullscreen API work on Metrofox

> +   * This mirrors the pref "full-screen-api.content-only". If this is true.

Comma after "true"?

>+  sFullscreenApiIsContentOnly = Preferences::GetBool("full-screen-api.content-only", false);

Why not a pref var cache?  Do we expect this to never change value after nsContentUtils::Init?  If so, why?

>     nsIDocument* parent = child->GetParentDocument();
>-    if (!parent) {
>+    if (child == fullScreenRootDoc) {

Should the GetParentDocument call move to after the if?

>+++ b/layout/base/nsPresShell.cpp
>         nsIDocument* root = nullptr;

This is now unused and can go away, right?

Why are the changes to pass null to ExitFullScreen correct?

r-me with that last bit explained.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 9 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 718672 [details] [diff] [review]
> Patch: Make fullscreen API work on Metrofox
> >+  sFullscreenApiIsContentOnly = Preferences::GetBool("full-screen-api.content-only", false);
> 
> Why not a pref var cache?  Do we expect this to never change value after
> nsContentUtils::Init?  If so, why?

If the value of sFullscreenApiIsContentOnly changes while something is fullscreen (which can happen if full-screen-api.content-only is true, but not if it's false), and more fullscreen operation are then performed I don't think we'd be in a consistent state, so I don't think it's sensible to make this dynamic.


> Why are the changes to pass null to ExitFullScreen correct?
> 
> r-me with that last bit explained.

Oops! I didn't properly page-in nsIDocument::ExitFullscreen()'s behaviour to my brain.

The nsFocusManager use of nsIDocument::ExitFullscreen() should indeed be:

+    nsIDocument::ExitFullscreen(fullscreenAncestor, /* async */ true);

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 10 • 11 years ago

Ah, and in the nsPresShell usage of ExitFullscreen needs to be called with the fullscreenAncestor too, as we can't be sure that doc is fullscreen, so GetFullscreenRoot() might fail and return nullptr. That would not give the desired effect in the content-only case of only exiting current tab from fullscreen.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

> so I don't think it's sensible to make this dynamic.

OK.  And we're sure that either the profile is loaded before Init() or we don't care about users setting this pref in their about:config or whatnot (something I can totally buy)?

As far as ExitFullscreen goes, passing fullscreenAncestor makes sense, I think, but why do we need to pass the fullscreen root in the content-only case?

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 12 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #11)
> > so I don't think it's sensible to make this dynamic.
> 
> OK.  And we're sure that either the profile is loaded before Init() or we
> don't care about users setting this pref in their about:config or whatnot
> (something I can totally buy)?

Huh... Based on my testing no, the profile isn't loaded before Init(), so I'd better make that that a BoolVarCache. Good thing you queried this!

I'd rather users didn't mess with that pref, but it's not the end of the world if they do.

> As far as ExitFullscreen goes, passing fullscreenAncestor makes sense, I
> think, but why do we need to pass the fullscreen root in the content-only
> case?

In the content-only case we don't actually need to pass the fullscreen root, because ExitFullscreen(doc) asks doc for its fullscreen root anyway.

However, if we pass a non fullscreen document to ExitFullscreen, ExitFullscreen doesn't do anything, so we must ensure the document we pass to ExitFullscreen is fullscreen, and here we know that fullscreenAncestor will be fullscreen.

Does that make sense?

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

> I'd rather users didn't mess with that pref, but it's not the end of the world if they do.

Well, all reading it before the profile is loaded would do is they _couldn't_ effectively mess with it, right?  So it might be ok to not have the bool var cache.

> Does that make sense?

Yes.  It's worth documenting somewhere!

	Jim Mathies [:jimm]
	Comment 14 • 11 years ago

Comment on attachment 718672 [details] [diff] [review]
Patch: Make fullscreen API work on Metrofox

This is great thanks. Can we use bug 842130 for ripping out the old fennec code?

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 15 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #13)
> > I'd rather users didn't mess with that pref, but it's not the end of the world if they do.
> 
> Well, all reading it before the profile is loaded would do is they
> _couldn't_ effectively mess with it, right?  So it might be ok to not have
> the bool var cache.

Ah yes, of course. Let's roll with just reading the pref in Init() then.

> > Does that make sense?
> 
> Yes.  It's worth documenting somewhere!

OK, I'll add to the comment on nsIDocument::ExitFullscreen().

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 16 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #14)
> Can we use bug 842130 for ripping out the old fennec code?

Sounds like a plan.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 17 • 11 years ago

Addressed review comments, and I added more comments into the code to document the things bz flagged.

Am just going to test on my touch device again to make sure everything's OK before landing.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9ce076f19d

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 19 • 11 years ago

dbaron's recently made assertions show up as errors on tbpl. My patch fixed an assertion firing, so on landing it showed up as an UNEXPECTED-PASS error on tbpl.

This patch removes the code that expects the error.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 20 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5eaadae8e474

	Gregory Szorc [:gps]
	Comment 21 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/bf9ce076f19d
https://hg.mozilla.org/mozilla-central/rev/5eaadae8e474