Closed Bug 688082 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: cpearce, Assigned: Margaret)

References

Details

(Whiteboard: [QA+])

Attachments

(1 file)

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.
#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.
Just saw "mozfullscreenchange" event. That'll do.
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.
Sounds reasonable.
Product: Fennec → Fennec Native
Summary: UI considerations for DOM full-screen API for Fennec → Implement "You've entered full-screen" warning for Fennec
Version: Trunk → unspecified
And I can take it :)
Assignee: nobody → margaret.leibovic
Depends on: 698836
No longer depends on: 684620
Attached patch patchSplinter Review
This depends on the patch in bug 698836.

We should probably ask a UX person about exactly what the string should say.
Attachment #572688 - Flags: review?(mark.finkle)
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?
(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 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.
Attachment #572688 - Flags: review?(mark.finkle) → review+
(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.
Blocks: 700678
Blocks: 700679
(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.
(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
(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!
https://hg.mozilla.org/projects/birch/rev/79690fc1fbf4
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-litmus?(fennec)
Whiteboard: [QA+]
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?
(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.
more...

It does not affect zooming and panning. The content is given the entire device screen area, but zooming an panning would still function.
Arguably we should disable panning and zooming in fullscreen mode.
(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?
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.
If a poorly designed app makes content overflow the screen in fullscreen mode, it might make sense to reenable panning and zooming.
These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
Test case added: https://litmus.mozilla.org/show_test.cgi?id=43039
Flags: in-litmus?(fennec) → in-litmus+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: