654295 - Closing last tab of a group doesn't show Panorama

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[ithinc]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110502 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110502 Firefox/6.0a1

See the following steps.

Reproducible: Always

Steps to Reproduce:
1. Open multiple tabs and split them into two groups
2. Restart Firefox, and ensure Panorama is not initialized
3. Close all the tabs in the current group one by one


Actual Results:  
When closing the last tab of the current group, Firefox switches to a tab in another group directly


Expected Results:  
Firefox should show the Panorama?

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[ithinc]

See https://bugzilla.mozilla.org/show_bug.cgi?id=654311#c2.

TabShow/TabSelect -> Select new group -> Close last active group if empty -> Show Panorama if last active group is closed and there is no app tabs

Comment 3

[ithinc]

Don't rely on TabClose event, it's too early. "browser.tabs.closeWindowWithLastTab" preference could be considered on TabClose. So if a new tab is added now, no TabShow/TabSelect will happen, thus no Group switching or Show Panorama.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v2

Please apply patch for Bug 643392 before this patch.

Comment 5

[Raymond Lee [:raymondlee]]

(In reply to comment #3)
> Don't rely on TabClose event, it's too early.
> "browser.tabs.closeWindowWithLastTab" preference could be considered on
> TabClose. So if a new tab is added now, no TabShow/TabSelect will happen,
> thus no Group switching or Show Panorama.

With this patch, if browser.tabs.closeWindowWithLastTab == false and the last tab is closed, a new tab would be created and Panorama would not be shown because no tab show event is fired.  If browser.tabs.closeWindowWithLastTab == true, the last tab could not be closed as the close button doesn't exist on the tab.

Comment 6

[ithinc]

(In reply to comment #5)
> With this patch, if browser.tabs.closeWindowWithLastTab == false and the
> last tab is closed, a new tab would be created and Panorama would not be
> shown because no tab show event is fired.  If
> browser.tabs.closeWindowWithLastTab == true, the last tab could not be
> closed as the close button doesn't exist on the tab.

I (and Bug 649979) meant to apply "browser.tabs.closeWindowWithLastTab" preference for each group.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> (In reply to comment #5)
> > With this patch, if browser.tabs.closeWindowWithLastTab == false and the
> > last tab is closed, a new tab would be created and Panorama would not be
> > shown because no tab show event is fired.  If
> > browser.tabs.closeWindowWithLastTab == true, the last tab could not be
> > closed as the close button doesn't exist on the tab.
> 
> I (and Bug 649979) meant to apply "browser.tabs.closeWindowWithLastTab"
> preference for each group.

That's another issue, bug 633707  should handle that.

Comment 10

[Raymond Lee [:raymondlee]]

@Tim: could you feedback on this as well please?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)

Comment 12

[ithinc]

If an add-on opens a new tab after TabClose event of the last tab of a group, the Panorama should not be shown. That's what I meant "TabClose event is too early".

Panorama should be shown only when:
1) A new group is selected, and
2) The last active group is empty, and
3) There is no app tabs
The combination of these three conditions ensures the group switching is unintentional, thus Panorama should be shown.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 532869 [details] [diff] [review]
v2

Review of attachment 532869 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-tabview.js
@@ +87,5 @@
>        if (data && data == "true") {
>          this.show();
>        } else {
> +        if (this._tabCloseEventListener)
> +          return;

I don't think we need this? TabView.init() is only executed once.

@@ +102,5 @@
> +                  // close empty group if one exists.
> +                  self._window.GroupItems.groupItems.forEach(function(groupItem) {
> +                    if (groupItem.closeIfEmpty())
> +                      return true;
> +                  });

Please don't close empty groups when Panorama is hidden (see bug 663421).

@@ +111,5 @@
> +          }
> +        };
> +        this._tabCloseEventListener = function(event) {
> +          if (!self._window && gBrowser.visibleTabs.length == 0)
> +            self._closedLastVisibleTabBeforeFrameInitialized = event.target;

We should just set this to "true".

@@ +143,5 @@
>      if (this._tabShowEventListener) {
>        gBrowser.tabContainer.removeEventListener(
> +        "TabShow", this._tabShowEventListener, false);
> +      gBrowser.tabContainer.removeEventListener(
> +        "TabClose", this._tabCloseEventListener, false);

Please wrap this in "if (self._tabCloseEventListener)".

@@ +189,5 @@
>        if (self._tabShowEventListener) {
>          gBrowser.tabContainer.removeEventListener(
> +          "TabShow", self._tabShowEventListener, false);
> +        gBrowser.tabContainer.removeEventListener(
> +          "TabClose", self._tabCloseEventListener, false);

Please wrap this in "if (self._tabCloseEventListener)".

::: browser/base/content/test/tabview/browser_tabview_bug654295.js
@@ +7,5 @@
> +  const DUMMY_PAGE_URL = "about:blank";
> +  const DUMMY_PAGE_URL_2 = "http://mochi.test:8888/";
> +
> +  let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
> +  waitForExplicitFinish();

That's a duplicate line.

@@ +10,5 @@
> +  let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
> +  waitForExplicitFinish();
> +
> +  // open a new window and setup the window state.
> +  let newWin = openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no");

Please use newWindowWithState() here.

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #13)
> Comment on attachment 532869 [details] [diff] [review] [diff] [details] [review]
> v2
> 
> Review of attachment 532869 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-tabview.js
> @@ +87,5 @@
> >        if (data && data == "true") {
> >          this.show();
> >        } else {
> > +        if (this._tabCloseEventListener)
> > +          return;
> 
> I don't think we need this? TabView.init() is only executed once.
> 

Removed

> @@ +102,5 @@
> > +                  // close empty group if one exists.
> > +                  self._window.GroupItems.groupItems.forEach(function(groupItem) {
> > +                    if (groupItem.closeIfEmpty())
> > +                      return true;
> > +                  });
> 
> Please don't close empty groups when Panorama is hidden (see bug 663421).
> 

Removed

> @@ +111,5 @@
> > +          }
> > +        };
> > +        this._tabCloseEventListener = function(event) {
> > +          if (!self._window && gBrowser.visibleTabs.length == 0)
> > +            self._closedLastVisibleTabBeforeFrameInitialized = event.target;
> 
> We should just set this to "true".
> 

Changed

> @@ +143,5 @@
> >      if (this._tabShowEventListener) {
> >        gBrowser.tabContainer.removeEventListener(
> > +        "TabShow", this._tabShowEventListener, false);
> > +      gBrowser.tabContainer.removeEventListener(
> > +        "TabClose", this._tabCloseEventListener, false);
> 
> Please wrap this in "if (self._tabCloseEventListener)".
> 

Added

> @@ +189,5 @@
> >        if (self._tabShowEventListener) {
> >          gBrowser.tabContainer.removeEventListener(
> > +          "TabShow", self._tabShowEventListener, false);
> > +        gBrowser.tabContainer.removeEventListener(
> > +          "TabClose", self._tabCloseEventListener, false);
> 
> Please wrap this in "if (self._tabCloseEventListener)".

Addded

> 
> ::: browser/base/content/test/tabview/browser_tabview_bug654295.js
> @@ +7,5 @@
> > +  const DUMMY_PAGE_URL = "about:blank";
> > +  const DUMMY_PAGE_URL_2 = "http://mochi.test:8888/";
> > +
> > +  let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
> > +  waitForExplicitFinish();
> 
> That's a duplicate line.
> 

Removed

> @@ +10,5 @@
> > +  let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
> > +  waitForExplicitFinish();
> > +
> > +  // open a new window and setup the window state.
> > +  let newWin = openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no");
> 
> Please use newWindowWithState() here.

Fixed

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=98a0778ee673

Comment 15

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #14)
> 
> Pushed to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=98a0778ee673

Passed Try!

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8586a8f3929f

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/8586a8f3929f