Closed Bug 639334 Opened 9 years ago Closed 9 years ago

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


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: mbrubeck, Assigned: mbrubeck)


(Keywords: polish, ux-mode-error, Whiteboard: [has patch][has review][needs approval])


(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #517276 - Flags: review?(wjohnston)
Comment on attachment 517276 [details] [diff] [review]

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.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:

GeoLocation Prompt:
Attachment #517276 - Flags: review?(wjohnston) → review+
Attached patch patch v2Splinter Review
(In reply to comment #1)
> -                                  null, browser.documentURI, false, null);
> +                                  null, browser.documentURI, true, null);

> Wrong patch.


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

Added these, thanks.
Attachment #517307 - Flags: review+
Attachment #517276 - Attachment is obsolete: true
Whiteboard: [has patch][needs review] → [has patch][has review][needs approval]
Just ran into another one of these from prefs:

It might be nice to use newOrSelectTab here as well.
(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?
Attached patch alternate patchSplinter Review
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.
Attachment #517463 - Flags: review?(mark.finkle)
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.
Attachment #517463 - Flags: review?(mark.finkle)
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.
Attachment #517307 - Flags: approval2.0+

Filed followup bug 639965.
Closed: 9 years ago
Resolution: --- → FIXED
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)
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.