568927 - Back button should close Fennec activity and return to the previous application

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alex Pakhotin (:alexp)]

Back button on Android finishes the current activity and returns back in the activity stack.
Fennec should support that as well.

Comment 1

[Alex Pakhotin (:alexp)]

Attachment: [Work in progress] Close on Back

Close Fennec when cannot go back in the session history.
This is not the best approach, but with current implementation the activity cannot be closed without shutting down Gecko.

Comment 2

[Matt Brubeck (:mbrubeck)]

If bug 515409 lands then this patch will need to be updated slightly, and it should decrease the risk of triggering this behavior accidentally.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: mozilla-central patch

Add a wrapper for moveTaskToBack(), used in the following patch.

I'm not sure if nsIDOMChromeWindow.Minimize is the best place to expose this.  I tried adding it to nsIPhoneSupport instead (as a new "hideTask" method alongside the existing "switchTask"), but AndroidBridge.h can't be included in externally-linked code right now because it indirectly uses nsString.h.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: mobile-browser patch

This is the patch for mobile-browser.  It depends on the patch in bug 515409.  It the same behavior as the built-in Android browser:

http://www.google.com/codesearch/p?hl=en#vReMmdMRqmA/src/com/android/browser/BrowserActivity.java&l=2566

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: mozilla-central patch v2

This also requires launchMode="singleInstance" (set in http://hg.mozilla.org/users/mwu_mozilla.com/test-repo/file/3cd4094f01a5/fix-launch but not on mozilla-central).

Comment 6

[Alex Pakhotin (:alexp)]

Comment on attachment 451327 [details] [diff] [review]
mozilla-central patch v2

Just a nit - fix the case indent:

> nsWindow::SetSizeMode(PRInt32 aMode)
> {
>+    switch (aMode) {
>+      case nsSizeMode_Minimized :
>+          AndroidBridge::Bridge()->MoveTaskToBack(true);

Comment 7

[Alex Pakhotin (:alexp)]

Comment on attachment 451309 [details] [diff] [review]
mobile-browser patch

Fine with me if nsIDOMChromeWindow.minimize() works.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: mozilla-central patch v3

Updated based on feedback from alexp and mwu:
- Fix indentation.
- Remove unused argument and return value from MoveTaskToBack.

Comment 9

[Michael Wu [:mwu]]

mozilla-central patch pushed.

http://hg.mozilla.org/mozilla-central/rev/46261630d046

Comment 10

[Benjamin Stover (:stechz)]

Comment on attachment 451309 [details] [diff] [review]
mobile-browser patch

What about:
1) user is browsing in fennec
2) user opens new tab
3) user presses back

I think the right behavior for this case is to close the tab and keep Fennec maximized.

Comment 11

[Matt Brubeck (:mbrubeck)]

Interestingly, Android's Browser does not have the "right" behavior for the steps in comment 10.  (It hides the browser activity after step 3.)

I'm not sure what the best rule is, or whether the "right" back behavior is well defined in a non-owned tab.  I can try adding a flag to distinguish between tabs was opened within Fennec vs. by another activity, but even this doesn't seem right once you start switching tabs.  For example:

1) From the Twitter app, click a link that opens in Fennec
2) Switch to an existing tab with no previous history (canGoBack == false)
3) Press "back"

Should we stay in Fennec, or go back to Twitter?  I'm honestly not sure.  And if the user opens Fennec later, returns to the Twitter-opened tab, and presses back..?

I'm leaning towards sticking with the Android Browser behavior, since at least it is simple and (mostly) predictable.

Comment 13

[Benjamin Stover (:stechz)]

I'd argue we should stay in Fennec and go to the previous tab, but I am hesitant to suggest anything different from the stock browser behavior in general (even if we do see it as broken).

Madhava should probably define this behavior.  I'd argue the important properties are:
1) support the case for clicking a link in another application and pressing back to get to the previous app
2) do what the user expects
3) be consistent

For non-browsing applications, the user expects to go to the previous "screen" that was focused on completing some task (what Android calls an activity).  This is further enforced with a sliding animation that lets the user know the activity has changed.  Since URLs and tabs aren't going to map perfectly to activities and we don't do the reinforcing animation, we are sending mixed messages by hooking the back button into browsing history.

I'm fine with this landing as is, but let's make sure this sticky issue is on Madhava's radar.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 451309 [details] [diff] [review]
mobile-browser patch

>diff -r 90cd9106f383 chrome/content/browser-ui.js

>-    else if (tab.owner)
>+    } else if (tab.owner) {
>       this.closeTab(tab);
>+    }

I am beginning to wonder if the "close the tab" behavior should be ANDROID only too

Comment 15

[Matt Brubeck (:mbrubeck)]

(In reply to comment #14)
> I am beginning to wonder if the "close the tab" behavior should be ANDROID only
> too

Since it's in handleEscape, it won't happen on Maemo (at least with current devices).  I like having it compile on desktop for easier testing in the emulator, but I can move it into the #ifdef if necessary.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 451309 [details] [diff] [review]
mobile-browser patch

We can adjust later for other devices, if needed

Comment 17

[Michael Wu [:mwu]]

http://hg.mozilla.org/mobile-browser/rev/b87a9f5d93ae

Comment 18

[Catalin Suciu [:csuciu]]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110921
Firefox/9.0a1 Fennec/9.0a1
Device: HTC Desire
OS: Android 2.2