639334 - Back button doesn't go back from pages opened by Fennec chrome

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Android users have been complaining that sometimes the "back" button exits Fennec when they don't expect it to.

One case where this happens is when a link is opened in a new tab by the Fennec chrome, including:
* "Browse site" and "Open page" buttons in the add-on manager.
* "Learn more" button in "What is an add-on?" row.
* Feedback buttons in the Beta Tester Tools add-on.

This patch makes the Android back button return to the previous page after these actions, instead of hiding Fennec and returning to the launcher screen.

This is a very simple low-risk polish fix that addresses user feedback, so I think we should take it for Fennec 4.0.

Comment 1

[Wesley Johnston (:wesj)]

Comment on attachment 517276 [details] [diff] [review]
patch

diff --git a/chrome/content/ContextCommands.js b/chrome/content/ContextCommands.js
--- a/chrome/content/ContextCommands.js
+++ b/chrome/content/ContextCommands.js
@@ -61,7 +61,7 @@ var ContextCommands = {
     ContentAreaUtils.internalSave(popupState.mediaURL, null, null,
                                   popupState.contentDisposition,
                                   popupState.contentType, false, "SaveImageTitle",
-                                  null, browser.documentURI, false, null);
+                                  null, browser.documentURI, true, null);
   },
 
   shareLink: function cc_shareLink() {

Wrong patch.

There are a few more of these that I saw:

JPAKE Dialog:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/sync.js#539

GeoLocation Prompt:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/notification.xml#96

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

(In reply to comment #1)
> -                                  null, browser.documentURI, false, null);
> +                                  null, browser.documentURI, true, null);

> Wrong patch.

Fixed.

> There are a few more of these that I saw:

Added these, thanks.

Comment 3

[Matt Brubeck (:mbrubeck)]

Related user feedback:
http://input.mozilla.com/en-US/beta/opinion/1620092
http://input.mozilla.com/en-US/beta/opinion/1568247
http://input.mozilla.com/en-US/beta/opinion/1535700
http://input.mozilla.com/en-US/beta/opinion/1237292
http://input.mozilla.com/en-US/beta/opinion/1164110

Comment 4

[Wesley Johnston (:wesj)]

Just ran into another one of these from prefs:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.xul#420

It might be nice to use newOrSelectTab here as well.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to comment #4)
> Just ran into another one of these from prefs:

Fixed this one locally, and one last one in downloads.js.

Is there any case where we definitely do *not* want to set the opener.  Perhaps we should just make this the default when aOpener is undefined?

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: alternate patch

This makes newTab(uri) default to using Browser.selectedTab as the opener.  This is a simpler patch (touches less code), but represents a slight change in behavior for callers of this API.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 517463 [details] [diff] [review]
alternate patch

The alternate patch may have some unwanted side effects.  For Fennec 4.0 we should focus on the original patch, which is entirely localized to specific links in the UI.

Comment 8

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

Comment on attachment 517307 [details] [diff] [review]
patch v2

Can you file a new bug to make this more robust? More like the alt patch.

Comment 9

[Matt Brubeck (:mbrubeck)]

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

Filed followup bug 639965.

Comment 10

[Anna (Waverley)]

VERIFIED FIXED on:
Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110315 Firefox/4.0b13pre Fennec /4.0b6pre 

Device: Motorola Droid 2 (Android 2.2)