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)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: alexp, Assigned: alexp)

References

Details

Attachments

(1 file, 4 obsolete files)

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: nobody → alexp
Status: NEW → ASSIGNED
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 ?
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.
Attachment #439114 - Attachment description: Fix → [Work in Progress] Go back in browser history
Attached patch [WIP] Fix v2 (obsolete) — Splinter Review
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 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.
(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.
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
(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.
(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
(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.
Attached patch Fix v3 (obsolete) — Splinter Review
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 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-
Attached patch Fix v4 (obsolete) — Splinter Review
Moved the Escape handler code to a separate function.
Attachment #441670 - Attachment is obsolete: true
Attachment #441723 - Flags: review?(mark.finkle)
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+
Attachment #441723 - Attachment is obsolete: true
Attachment #442109 - Flags: review+
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.
http://hg.mozilla.org/mobile-browser/rev/df6647e20f84
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 568927
Flags: in-litmus?
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.