Closed
Bug 559453
Opened 14 years ago
Closed 14 years ago
Fennec should handle Back button on Android
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: alexp, Assigned: alexp)
References
Details
Attachments
(1 file, 4 obsolete files)
2.11 KB,
patch
|
alexp
:
review+
|
Details | Diff | Splinter Review |
Android devices have a Back button, which returns back in the stack of activities. It works as Back in the Android browser, we should do the same in Fennec.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → alexp
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
Comment on attachment 439114 [details] [diff] [review] [Work in Progress] Go back in browser history I don't think it's this simple. We have other situations where "BACK" can be used that is not browsing history: * Awesomebar visible * Bookmark manager visible * Browser tools visible ?
Assignee | ||
Comment 3•14 years ago
|
||
Thank you for a comment, Mark. I should have mentioned it's still work in progress. Back in browsing history is just the most obvious action, there are definitely more. In addition to the listed ones, we should also handle it like other Android applications - when the stack of the activities is empty, Fennec should hide and return control to the Home screen. Awesomebar is already handled by this simple patch, but the dialogs and pop-ups are not yet.
Assignee | ||
Updated•14 years ago
|
Attachment #439114 -
Attachment description: Fix → [Work in Progress] Go back in browser history
Assignee | ||
Comment 4•14 years ago
|
||
Prevent further handling of Escape button if it has closed a dialog. This version of the patch makes the BACK work without interference both for browsing and for the dialogs, which already close on Escape (like Awesomebar and Bookmark manager).
Attachment #439114 -
Attachment is obsolete: true
Comment 5•14 years ago
|
||
Comment on attachment 439435 [details] [diff] [review] [WIP] Fix v2 Does this pass some manual testing? * Closes awesomebar and bookmark manager * Goes back in session history * Does not close panel _and_ go back in session history I worry a bit because dialog boxes can also be closed using ESCAPE. In those cases I think the dialog would close _and_ the session history would go back. Maybe we should not use the cmd_back and just add some logic into the "keypress" handler.
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #5) > Does this pass some manual testing? > * Closes awesomebar and bookmark manager > * Goes back in session history > * Does not close panel _and_ go back in session history These tests pass with the second version of the patch. If the awesomebar or bookmark manager are opened, Back button closes them, but the browser stays on the same page. Another press of the Back, and it goes back in the session history. > I worry a bit because dialog boxes can also be closed using ESCAPE. In those > cases I think the dialog would close _and_ the session history would go back. That's what the update to the patch prevents - if a dialog was closed using Escape (what the Android-Back button is now mapped to), call to aEvent.preventDefault() (as you recommended to try) stops further processing of the event. I also tried to use stopPropagation(), but it didn't work as expected. > Maybe we should not use the cmd_back and just add some logic into the > "keypress" handler. I guess we could try to do that if it's better than the current approach.
Comment 7•14 years ago
|
||
Late to the party (my earlier comment got eaten by a restarting browser). Things the back button should do - back out of the awesomescreen, leaving the user in the content area - back out of the bookmarks manager, leaving the user in the awesomescreen - back out the tools area, leaving the user in the content area - dismiss a cancellable modal dialog, leaving the user on whatever was underneath - dismiss the site menu - dismiss a context menu - _probably_ back up a page, up to the beginning of the history trail, at which point it would back out of the app
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7) Agree with that list. > - back out of the bookmarks manager, leaving the user in the awesomescreen By the way, this is what I'd expect, but when the bookmark manager is closed (even with a screen "Back" button on the top-right), we do not return back to the awesomescreen, but directly to the web page. They don't seem implemented as a stack currently.
Comment 9•14 years ago
|
||
(In reply to comment #7) > - back out of the bookmarks manager, leaving the user in the awesomescreen We don't work this way. Bookmark Manager goes to content, not Awesomebar Screen
Comment 10•14 years ago
|
||
(In reply to comment #6) > > I worry a bit because dialog boxes can also be closed using ESCAPE. In those > > cases I think the dialog would close _and_ the session history would go back. > > That's what the update to the patch prevents - if a dialog was closed using > Escape (what the Android-Back button is now mapped to), call to > aEvent.preventDefault() (as you recommended to try) stops further processing of > the event. I also tried to use stopPropagation(), but it didn't work as > expected. When I say "dialog", I mean things like alerts and confirm dialogs. Your patch doesn't handle them yet. > > Maybe we should not use the cmd_back and just add some logic into the > > "keypress" handler. > > I guess we could try to do that if it's better than the current approach. I think we'll need to to handle all the stuff Madhava listed.
Assignee | ||
Comment 11•14 years ago
|
||
With this fix the BACK button closes: - Open dialogs - Open popups - Open modal elements - Open panel or goes back in the session history if nothing from this list is open. This version does not yet hide Fennec if the history is empty. I'd recommend to implement that as a separate fix for a follow-up bug, as it is more complex than this one and will probably involve changes in several components.
Attachment #439435 -
Attachment is obsolete: true
Attachment #441670 -
Flags: review?(mark.finkle)
Comment 12•14 years ago
|
||
Comment on attachment 441670 [details] [diff] [review] Fix v3 >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js > if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE) { >+ // Check open dialogs > let dialog = this.activeDialog; > if (dialog) > dialog.close(); >+ else { >+ // Check open popups >+ if (BrowserUI._popup) >+ BrowserUI._hidePopup(); >+ else { >+ // Check open modal elements >+ let modalElementsLength = document.getElementsByClassName("modal-block").length; >+ if (modalElementsLength == 0) { >+ // Check open panel >+ if (BrowserUI.isPanelVisible()) >+ this.hidePanel(); >+ else { >+ // Only if there are no dialogs, popups, or panels open >+ var browser = getBrowser(); >+ browser.goBack(); >+ } >+ } >+ } >+ } nits: * if one side of an if/else needs braces, then both sides gets braces. * we are cuddling the "else" (not my personal style, but I have learned to accept it) if () { } else { } HOWEVER! I think we should change this code to use early returns. The previous code was very simple, so the if/else was OK, but now things are too complex. It would be cleaner to use: let dialog = this.activeDialog; if (dialog) { dialog.close(); return; } // Check open popups if (BrowserUI._popup) { BrowserUI._hidePopup(); return; } // Check open modal elements let modalElementsLength = document.getElementsByClassName("modal-block").length; if (modalElementsLength > 0) return; // Check open panel if (BrowserUI.isPanelVisible()) { this.hidePanel(); return; } // Only if there are no dialogs, popups, or panels open Browser.selectedBrowser.goBack();
Attachment #441670 -
Flags: review?(mark.finkle) → review-
Assignee | ||
Comment 13•14 years ago
|
||
Moved the Escape handler code to a separate function.
Attachment #441670 -
Attachment is obsolete: true
Attachment #441723 -
Flags: review?(mark.finkle)
Comment 14•14 years ago
|
||
Comment on attachment 441723 [details] [diff] [review] Fix v4 One last nit: Any "BrowserUI" the handleEscape method should become "this"
Attachment #441723 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 15•14 years ago
|
||
Attachment #441723 -
Attachment is obsolete: true
Attachment #442109 -
Flags: review+
Assignee | ||
Comment 16•14 years ago
|
||
One note: current implementation does Back action when the key is down. Android 2.0 guidelines recommend to do that on a key-up and track the key-down/key-up events to prevent accidental key presses. This will be done on a lower level and should not affect this patch for the BrowserUI though.
Comment 17•14 years ago
|
||
http://hg.mozilla.org/mobile-browser/rev/df6647e20f84
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Flags: in-litmus?
Comment 19•14 years ago
|
||
These are the litmus test cases for this bug(and its associated dependency bugs) **https://litmus.mozilla.org/show_test.cgi?id=12331 **https://litmus.mozilla.org/show_test.cgi?id=12333 **https://litmus.mozilla.org/show_test.cgi?id=12322 **https://litmus.mozilla.org/show_test.cgi?id=12747 **https://litmus.mozilla.org/show_test.cgi?id=12748 **https://litmus.mozilla.org/show_test.cgi?id=12749 **https://litmus.mozilla.org/show_test.cgi?id=12750
Flags: in-litmus? → in-litmus+
Comment 20•13 years ago
|
||
Fennec now handles back button on Android. Retested bug with: Build id : Mozilla/5.0 (Android;Linux armv7l; rv:9.0a1)Gecko/20110927 Firefox/9.0a1 Fennec /9.0a1 Device: Motorola DROID 2 OS: Android 2.3 Back button is used for: - browser history - goes back in the session history - closing dialogs and popups - dismiss a context menu - dismiss the site menu - back out the tools area, leaving the user in the content area - back out of the Awesome screen, leaving the user in the content area Verifying bug.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•