Last Comment Bug 559453 - Fennec should handle Back button on Android
: Fennec should handle Back button on Android
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: ---
Assigned To: Alex Pakhotin (:alexp)
:
Mentors:
: 460088 (view as bug list)
Depends on:
Blocks: 568927
  Show dependency treegraph
 
Reported: 2010-04-14 15:28 PDT by Alex Pakhotin (:alexp)
Modified: 2011-09-27 09:12 PDT (History)
8 users (show)
ayanshah62: in‑litmus+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
[Work in Progress] Go back in browser history (1.15 KB, patch)
2010-04-14 15:30 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
[WIP] Fix v2 (1.91 KB, patch)
2010-04-15 19:38 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Fix v3 (1.62 KB, patch)
2010-04-26 18:08 PDT, Alex Pakhotin (:alexp)
mark.finkle: review-
Details | Diff | Splinter Review
Fix v4 (2.12 KB, patch)
2010-04-27 00:02 PDT, Alex Pakhotin (:alexp)
mark.finkle: review+
Details | Diff | Splinter Review
Fixed the last nit (2.11 KB, patch)
2010-04-28 09:33 PDT, Alex Pakhotin (:alexp)
alex.mozilla: review+
Details | Diff | Splinter Review

Description Alex Pakhotin (:alexp) 2010-04-14 15:28:21 PDT
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.
Comment 1 Alex Pakhotin (:alexp) 2010-04-14 15:30:37 PDT
Created attachment 439114 [details] [diff] [review]
[Work in Progress] Go back in browser history
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2010-04-15 12:32:40 PDT
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 ?
Comment 3 Alex Pakhotin (:alexp) 2010-04-15 12:57:46 PDT
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.
Comment 4 Alex Pakhotin (:alexp) 2010-04-15 19:38:04 PDT
Created attachment 439435 [details] [diff] [review]
[WIP] Fix v2

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).
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2010-04-16 13:43:44 PDT
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.
Comment 6 Alex Pakhotin (:alexp) 2010-04-16 13:57:29 PDT
(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 Madhava Enros [:madhava] 2010-04-16 14:01:41 PDT
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
Comment 8 Alex Pakhotin (:alexp) 2010-04-16 14:09:45 PDT
(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 Mark Finkle (:mfinkle) (use needinfo?) 2010-04-16 14:14:36 PDT
(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 Mark Finkle (:mfinkle) (use needinfo?) 2010-04-16 14:16:41 PDT
(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.
Comment 11 Alex Pakhotin (:alexp) 2010-04-26 18:08:01 PDT
Created attachment 441670 [details] [diff] [review]
Fix v3

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.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2010-04-26 23:01:11 PDT
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();
Comment 13 Alex Pakhotin (:alexp) 2010-04-27 00:02:27 PDT
Created attachment 441723 [details] [diff] [review]
Fix v4

Moved the Escape handler code to a separate function.
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2010-04-28 09:01:28 PDT
Comment on attachment 441723 [details] [diff] [review]
Fix v4

One last nit: Any "BrowserUI" the handleEscape method should become "this"
Comment 15 Alex Pakhotin (:alexp) 2010-04-28 09:33:36 PDT
Created attachment 442109 [details] [diff] [review]
Fixed the last nit
Comment 16 Alex Pakhotin (:alexp) 2010-04-28 10:02:24 PDT
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 Michael Wu [:mwu] 2010-04-28 11:34:41 PDT
http://hg.mozilla.org/mobile-browser/rev/df6647e20f84
Comment 18 Michael Wu [:mwu] 2010-05-13 10:30:36 PDT
*** Bug 460088 has been marked as a duplicate of this bug. ***
Comment 20 Carla Nadastean 2011-09-27 09:12:28 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.