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)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: cpearce, Assigned: Margaret)
References
Details
(Whiteboard: [QA+])
Attachments
(1 file)
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
#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•13 years ago
|
||
Just saw "mozfullscreenchange" event. That'll do.
Assignee | ||
Comment 3•13 years ago
|
||
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.
Reporter | ||
Comment 4•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 6•13 years ago
|
||
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)
Reporter | ||
Comment 7•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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+
Reporter | ||
Comment 10•13 years ago
|
||
(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.
Assignee | ||
Comment 11•13 years ago
|
||
(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•13 years ago
|
||
(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
Assignee | ||
Comment 13•13 years ago
|
||
(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!
Assignee | ||
Comment 14•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
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?
Comment 16•13 years ago
|
||
(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•13 years ago
|
||
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.
(separate bug of course)
Comment 20•13 years ago
|
||
(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.
Comment 23•13 years ago
|
||
These patches were backed while investigating Talos failures. Now that tests are green again, we will need to reland.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•13 years ago
|
||
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 25•13 years ago
|
||
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
Comment 26•13 years ago
|
||
Test case added: https://litmus.mozilla.org/show_test.cgi?id=43039
Flags: in-litmus?(fennec) → in-litmus+
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•