688082 - Implement "You've entered full-screen" warning for Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We need to do something sensible for the DOM full-screen API on fennec (bug 545812). The DOM full-screen API uses nsGlobalWindow::SetFullScreen(), and that works appropriately on Fennec apparently, so the base API should Just Work (once bug 684620 lands to proxy calls to nsGlobalWindow::SetFullScreen() to the chrome process)

Then we need to:
1. Implement a "you've entered full-screen, press the back button to exit" warning drop down.
2. Implement exiting full-screen when back button is pressed.

I'm not sure how to do (2). I can't initiate leaving full-screen mode from chrome since there (currently) appears to be no way to get hold of a corresponding PBrowserParent from any of the C++ entry points in the chrome process.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

#1 could be implemented via the "fullscreen" event and displaying a popup (toaster) alert. Might be handy if we knew the "fullscreen" event was triggered via DOM and not some other reason. Maybe an event.reason or event.detail ?

#2 seems like a hole in the e10s/IPC framework.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Just saw "mozfullscreenchange" event. That'll do.

Comment 3

[:Margaret Leibovic]

I'm implementing #2 for native fennec in bug 698836. Since we're not developing XUL fennec anymore, we could probably move this bug to the native fennec component and make it about implementing #1.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Sounds reasonable.

Comment 5

[:Margaret Leibovic]

And I can take it :)

Comment 6

[:Margaret Leibovic]

Attachment: patch

This depends on the patch in bug 698836.

We should probably ask a UX person about exactly what the string should say.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 572688 [details] [diff] [review]
patch

The warning on desktop Firefox is "Press ESC to exit full-screen mode".

Is there any way to enter window full-screen mode in Fennec (i.e. having chrome set |window.fullscreen=true|? In desktop Firefox you can do this by pressing F11, so we listen for the "mozfullscreenchange" event instead. This way we can also catch the case where the user is in window/browser full-screen mode and switches to DOM full-screen mode. Maybe it makes sense to do that here too?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #7)
> Comment on attachment 572688 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> The warning on desktop Firefox is "Press ESC to exit full-screen mode".
> 
> Is there any way to enter window full-screen mode in Fennec (i.e. having
> chrome set |window.fullscreen=true|? In desktop Firefox you can do this by
> pressing F11, so we listen for the "mozfullscreenchange" event instead. This
> way we can also catch the case where the user is in window/browser
> full-screen mode and switches to DOM full-screen mode. Maybe it makes sense
> to do that here too?

Bug 698836 adds a context menu for <video> allowing the window to go fullscreen, but it does not use window.fullScreen. It uses document.mozRequestFullScreen.

So you are saying that we should be using "mozfullscreenchange" event instead of "fullscreen" so we know anytime the system tries to use DOM fullscreen API, even if the window is already fullscreen?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 572688 [details] [diff] [review]
patch


>diff --git a/mobile/locales/en-US/chrome/browser.properties b/mobile/locales/en-US/chrome/browser.properties

>+alertFullScreenToast=You've entered full-screen mode. Press the back button to exit.

Let's mimic the desktop string for now:

"Press BACK to leave full-screen mode"

===========
We should think about listening for "mozfullscreenchange" although, I'm not sure it gives us anything over "fullscreen" yet - given our simple implementation and chrome.

Looking at the ways we exit fullscreen mode on desktop makes me think we should add a few more to mobile:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3915

Places we could toggle out of fullscreen mode:
* In GeckoApp.onPause  (app is deactivated)
* In GeckoApp.handleAddTab, handleCloseTab and handleSelectTab ("TabOpen", "TabClose" and "TabSelect" events in desktop)
* In GeckoApp.onPrepareOptionsMenu when the menu is about to be shown (?? not sure about this one)

These could be a followup bug, after we talk to UX.

r+, but make the string change. the rest needs more thought and can be a followup.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> So you are saying that we should be using "mozfullscreenchange" event
> instead of "fullscreen" so we know anytime the system tries to use DOM
> fullscreen API, even if the window is already fullscreen?

If there aren't any places that set window.fullscreen on mobile, then adding your listener to the "fullscreen" event is ok. The "fullscreen" event fires synchronously with the transition to full-screen mode, whereas the mozfullscreenchange event fires asynchronously after the fact. So if there's no uses of the window.fullscreen chrome API that could confuse matters, you're better off listening to "fullscreen" so you can get your warning up faster.

On desktop we also show the warning whenever there's alpha-numeric key input as a safeguard against password phishing. I guess it makes sense to show the warning on text input on mobile too; the bad guys could still fake the fennec UI going to paypal.com or whatever and phish for passwords.

Comment 11

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> >+alertFullScreenToast=You've entered full-screen mode. Press the back button to exit.
> 
> Let's mimic the desktop string for now:
> 
> "Press BACK to leave full-screen mode"

s/leave/exit/ as per comment 7?

Also, I filed bug 700678 and 700679 as follow-ups.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Margaret Leibovic [:margaret] from comment #11)
> (In reply to Mark Finkle (:mfinkle) from comment #9)
> > >+alertFullScreenToast=You've entered full-screen mode. Press the back button to exit.
> > 
> > Let's mimic the desktop string for now:
> > 
> > "Press BACK to leave full-screen mode"
> 
> s/leave/exit/ as per comment 7?
> 
> Also, I filed bug 700678 and 700679 as follow-ups.

Desktop uses "leave":
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#93

Comment 13

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> (In reply to Margaret Leibovic [:margaret] from comment #11)
> > (In reply to Mark Finkle (:mfinkle) from comment #9)
> > > >+alertFullScreenToast=You've entered full-screen mode. Press the back button to exit.
> > > 
> > > Let's mimic the desktop string for now:
> > > 
> > > "Press BACK to leave full-screen mode"
> > 
> > s/leave/exit/ as per comment 7?
> > 
> > Also, I filed bug 700678 and 700679 as follow-ups.
> 
> Desktop uses "leave":
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.dtd#93

Oh, okay, I should have looked up the string myself. Thanks!

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/projects/birch/rev/79690fc1fbf4

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What does fullscreen actually do on mobile? Does it disable the displayport/viewport features and just make the document fill the screen exactly? Does it disable zooming?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> What does fullscreen actually do on mobile? Does it disable the
> displayport/viewport features and just make the document fill the screen
> exactly? Does it disable zooming?

It simply removes the browser chrome and also allows the content to fill the entire device screen. Meaning, it hides the "status bar" across the top of the device where you normally see network, battery and status indicators.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

more...

It does not affect zooming and panning. The content is given the entire device screen area, but zooming an panning would still function.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Arguably we should disable panning and zooming in fullscreen mode.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(separate bug of course)

Comment 20

[Paul [pwd]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Arguably we should disable panning and zooming in fullscreen mode.

I'm not sure I understand why?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The use-cases for fullscreen generally involve the app filling the entire screen with content designed to fit the screen. For them, panning and zooming is unneeded and unwanted.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If a poorly designed app makes content overflow the screen in fullscreen mode, it might make sense to reenable panning and zooming.

Comment 23

[Wesley Johnston (:wesj)]

These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547

Comment 25

[Aaron Train [:aaronmt]]

20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)

Comment 26

[Andreea Pod]

Test case added: https://litmus.mozilla.org/show_test.cgi?id=43039