515409 - closing a tab should go back to opening tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Axel Hecht [:Pike]]

I had tree tabs open on fennec, and was on the mail tab in the middle. I clicked on links in mail (bugmail), closed the bug, ended up in facebook, and had to open the tab strip and click to get back. I didn't figure out if I could reorder tabs, which didn't make it easy to work around that.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Did that mean, opening the new tab right after the opening one?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

s/opening one/opener

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

After thinking, (In reply to comment #1)
> Did that mean, opening the new tab right after the opener?

This also mean we need to have an indication about new opened tab in the tabs area.

Comment 4

[Axel Hecht [:Pike]]

In Firefox, the tab is opened at the end, but if you close the tab, you get back to the opener.

I don't really mind that much where the new tab is inserted in tab order, as long as you can easily follow links with target="_blank" and close the new tabs to get back to where you were before.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #4)
> In Firefox, the tab is opened at the end, but if you close the tab, you get
> back to the opener.

Not since 3 days (bug 465673) :)

Actually in a Firefox build Gecko/20090909 Namoroka/3.6a2pre the tab appears right after the opener, so I'm wondering if we should do the same?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Madhava, can you give us any advice?

Comment 7

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

Firefox (tabbrowser) has the concept of a tab "owner". Some types of tabs are considered to be owned by an existing tab:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1155

When a tab is closed, any tabs that it "owns" have their .owner propterty set to null. They are orphaned:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1475

Also, when a tab is closed, the "owner" tab (if one exists) becomes selected:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1584

I think we could implement a similar system.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip

Thanks for the direction Mark!

This WIP implement the 'owner' part. It missed the 'browser.tabs.selectOwnerOnClose' check thought.

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This is an updated version of Vivien's patch, with some minor changes and fixes.

There's still a problem when add-ons call newTab or addTab from chrome pages.  They may need to be updated, depending on whether the new tab should be considered a child of an existing tab.  For example, the new behavior does not work for Firefox Sync's "Tabs from your other computers" link on about:home.

This patch *also* makes the "back" key (backspace, escape, or the Android "back" button) return to the parent tab when possible, if you are at the start of the current tab's history.  This is exactly what the stock Android browser does, and I find it extremely useful to prevent the "back" button from breaking mysteriously when sites open links in new tabs.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

tiny formatting fix - misplaced a curly brace

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

>     // Only if there are no dialogs, popups, or panels open
>-    Browser.selectedBrowser.goBack();
>+    Browser.goBack();

I'm not a fan of the goBack function encapsuling a browser.xml#goBack function encapsuling the webNavigation#goBack function, ...
Maybe we could add a return value to the goBack/goForward methods of browser.xml to know if goBack has been done or not and use that instead and only for Android.

Or better we could add this to the browser.xml binding itself :)

> 
>-  addTab: function(uri, bringFront) {
>+  addTab: function(uri, bringFront, aOwner) {
>     let newTab = new Tab();
>+    newTab.owner = aOwner;

nit: maybe use newTab.owner = null || aOwner just to avoid having an undefined value. I think extensions developers will sometimes try to call it and avoid passing the last arg.


Code looks ok otherwise, my only problem is having a Browser.goBack function (not its behavior thought)

Comment 12

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

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

>diff -r 74295e05a514 chrome/content/aboutHome.xhtml
>     function openTabs(aURLs) {

>+      let owner = getChromeWin().Browser.selectedTab;
>+      for (let i=0; i < aURLs.length; i++) {
>+        BrowserUI.newTab(aURLs[i], owner);

I'm not sure this is a good idea. Let's try it and see how it feels.

>diff -r 74295e05a514 chrome/content/browser-ui.js

>     // Only if there are no dialogs, popups, or panels open
>-    Browser.selectedBrowser.goBack();
>+    Browser.goBack();

I'd rather you move the implementation here. I do not want Browser.goBack

>       case "cmd_back":
>-        browser.goBack();
>+        Browser.goBack();

Leave alone. Let's not substitute anywhere but handleEscape

>+  goBack: function goBack() {
>+    let tab = this._selectedTab;
>+    let browser = tab.browser;
>+
>+    if (browser.canGoBack)
>+      browser.goBack();
>+    else if (tab.owner)
>+      this.closeTab(tab);
>+  },

Move this impl to handleEscape

Also, please add a Tab.owner default. I like to see members explicitly defined in the code whenever possible.

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Updated based on feedback from Mark and Vivien.

Comment 14

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

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/feadb07ae869

I think we could add some tests (browser_tabs.js) fairly easily.

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: tests

Comment 16

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

Comment on attachment 451425 [details] [diff] [review]
tests

I had to add a check for closing a closed tab in Browser.closeTab since the test tries to close new_tab_03 twice.

It's a good thing! Test found a bug!

Comment 17

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

pushed tests:
http://hg.mozilla.org/mobile-browser/rev/1135faf54be6

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified :
Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110506 Firefox/6.0a1 Fennec/6.0a1
Device: Thunderbolt
OS: Android 2.2