Closed
Bug 700678
Opened 13 years ago
Closed 12 years ago
Exit full screen mode when the app goes into the background
Categories
(Firefox for Android Graveyard :: General, defect, P4)
Tracking
(firefox19 verified)
VERIFIED
FIXED
Firefox 19
Tracking | Status | |
---|---|---|
firefox19 | --- | verified |
People
(Reporter: Margaret, Assigned: pushkar.singh93)
References
Details
(Whiteboard: [mentor=margaret][lang=js])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Follow up from bug 688082 comment 9:
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)
Updated•13 years ago
|
Priority: -- → P4
Updated•13 years ago
|
Assignee: nobody → margaret.leibovic
Reporter | ||
Comment 1•12 years ago
|
||
Revisiting this bug, almost everything I said in comment 0 is out of date. I'm not sure what I was thinking about tab events, because I'm not sure how we could be encountering these tab events while in full screen mode. We have much more limited UI than desktop, so this might be a case where we don't need to do as much as desktop.
However, I think it does make sense to exit full screen mode when the application goes into the background, since it can be confusing if you come back and you forgot that you were in full screen mode. Also, if you're in full screen mode, put the app in the background, then open a link from another app, that new tab will be in full screen mode, which is even more confusing.
To test full screen mode, I usually just put this video in full screen mode:
http://clips.vorwaerts-gmbh.de/big_buck_bunny.webm
Assignee: margaret.leibovic → nobody
Summary: Add more ways to exit full screen mode → Exit full screen mode when the app goes into the background
Whiteboard: [mentor=margaret][lang=java]
Comment 2•12 years ago
|
||
I agree with the fact the full screen should be exited when the app goes to the background :)
Assignee | ||
Comment 4•12 years ago
|
||
Hi, I am a new contributor and I'm interested in working on this bug. If nobody else is working then I am eager to try my hands on this with some guidance. Can u give me some clue on how to start? A lot thanks!
Updated•12 years ago
|
Assignee: nobody → pushkar.singh93
Flags: needinfo?(margaret.leibovic)
Reporter | ||
Comment 5•12 years ago
|
||
(In reply to pushkar.singh93 from comment #4)
> Hi, I am a new contributor and I'm interested in working on this bug. If
> nobody else is working then I am eager to try my hands on this with some
> guidance. Can u give me some clue on how to start? A lot thanks!
Welcome! First of all, do you have a development environment set up for building fennec? If not, you should follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec
To write a patch for this bug, you're going to need to add some code that exits full screen mode when we go into the background. I marked this bug as a Java bug, but actually this can all be done on the JS side of things. We already listen for the application going into the background here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6412
In there, we can check to see if the currently selected tab is in fullscreen mode, and if so, we should exit it. To do this, you'll need the selected tab's content document, which you can get at with BrowserApp.selectedTab.browser.contentDocument. From there, you can check fullscreen mode with document.mozFullScreen, and exit fullscreen mode with document.mozCancelFullScreen().
I would encourage you to use mxr.mozilla.org to search around for code related to "fullscreen" to get a sense of how this all works.
Also, this will only exit DOM fullscreen mode, not fullscreen plugins, so we may want to file a separate bug for that.
Flags: needinfo?(margaret.leibovic)
Whiteboard: [mentor=margaret][lang=java] → [mentor=margaret][lang=js]
Reporter | ||
Comment 6•12 years ago
|
||
Also, please feel free to ask questions in #mobile on irc.mozilla.org. There's usually a bunch of friendly developers hanging out in there!
Assignee | ||
Comment 7•12 years ago
|
||
I built the code and its working fine, I am now going to tackle the main bug.
Assignee | ||
Comment 8•12 years ago
|
||
Hi!, I wanted to ask when can I talk to you over IRC. I think IRC will be great for clearing small doubts and queries.
Reporter | ||
Comment 9•12 years ago
|
||
(In reply to pushkar.singh93 from comment #8)
> Hi!, I wanted to ask when can I talk to you over IRC. I think IRC will be
> great for clearing small doubts and queries.
For those following along, we worked this out on IRC :)
Assignee | ||
Comment 10•12 years ago
|
||
Checking the condition for app, when in background and having full Screen mode then we exit full-screen mode for the app.
Attachment #676741 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 11•12 years ago
|
||
Comment on attachment 676741 [details] [diff] [review]
Bug 700678 : Escaping FullScreen when app goes in background
This looks like it's on the right track, but there are a few issues. First of all, just using |document| isn't guaranteed to give you the right document. You want to check the content document of the currently selected tab. To do that, you can just declare a local variable, like so:
let doc = BrowserApp.selectedTab.browser.contentDocument;
Then use |doc| instead of |document|.
Secondly, it appears your patch isn't formatted correctly, so I couldn't apply it to my tree. You should read these pages about making a patch:
https://developer.mozilla.org/en-US/docs/Creating_a_patch
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Attachment #676741 -
Flags: review?(margaret.leibovic) → review-
Assignee | ||
Comment 12•12 years ago
|
||
This patch contains all the required changes.
Attachment #676741 -
Attachment is obsolete: true
Attachment #676972 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 13•12 years ago
|
||
I am really sorry for the last patch, actually I instead of using Hg for diff just directly used the shell command diff to generate the universal patch. Sorry for the mistake.
Reporter | ||
Comment 14•12 years ago
|
||
Comment on attachment 676972 [details] [diff] [review]
Bug 700678 - Exit full screen mode when the app goes into the background
This is great, thanks!
Attachment #676972 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Comment 15•12 years ago
|
||
I was going to land this for you, but mozilla-inbound is closed at the moment.
To land your patch, you can also upload a new version of the patch with "r=margaret" added to the end of the commit message, then add "checkin-needed" to the keyword field, and then a kind soul should come by and land the patch for you :)
Assignee | ||
Comment 16•12 years ago
|
||
The Final Patch!
Updated•12 years ago
|
Attachment #677106 -
Flags: review?(margaret.leibovic)
Updated•12 years ago
|
Attachment #677106 -
Flags: review?(margaret.leibovic)
Updated•12 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 17•12 years ago
|
||
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/840a9b8ec6e1
This will be merged to mozilla-central later, and it will be part of Firefox 19. Thanks!
Target Milestone: --- → Firefox 19
Assignee | ||
Comment 18•12 years ago
|
||
Amazing! Thank You!
Updated•12 years ago
|
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment 19•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Comment 20•12 years ago
|
||
The app exits from full screen mode when it's moved to background. However, the app will freeze when it's reopened (bug 809055).
Closing bug as verified fixed on:
Firefox 19.0a1 (2012-11-06)
Device: Galaxy S2
OS: Android 4.0.3
Status: RESOLVED → VERIFIED
status-firefox19:
--- → verified
Comment 21•12 years ago
|
||
A TC was added in MozTrap for this on 19 phones, 19 tablets, 20 phones, 20 tablets
https://moztrap.mozilla.org/manage/case/5945
Flags: in-moztrap+
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
•