Last Comment Bug 654295 - Closing last tab of a group doesn't show Panorama
: Closing last tab of a group doesn't show Panorama
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 10
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on: 643392 654311
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-02 15:18 PDT by ithinc
Modified: 2016-04-12 14:00 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (8.54 KB, patch)
2011-05-12 09:39 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v2 (8.22 KB, patch)
2011-05-16 22:19 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch for checkin (7.71 KB, patch)
2011-10-10 02:56 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description ithinc 2011-05-02 15:18:51 PDT
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] 2011-05-12 09:39:50 PDT
Created attachment 531956 [details] [diff] [review]
v1
Comment 2 ithinc 2011-05-13 04:08:23 PDT
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 2011-05-13 04:20:39 PDT
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] 2011-05-16 22:19:43 PDT
Created attachment 532869 [details] [diff] [review]
v2

Please apply patch for Bug 643392 before this patch.
Comment 5 Raymond Lee [:raymondlee] 2011-05-16 22:33:08 PDT
(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 2011-05-17 00:28:37 PDT
(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] 2011-05-27 02:24:19 PDT
bugspam
Comment 8 Tim Taubert [:ttaubert] 2011-05-27 02:29:24 PDT
bugspam
Comment 9 Raymond Lee [:raymondlee] 2011-06-02 18:51:56 PDT
(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] 2011-06-28 02:33:52 PDT
@Tim: could you feedback on this as well please?
Comment 11 Tim Taubert [:ttaubert] 2011-07-24 18:40:20 PDT
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 2011-09-05 16:57:15 PDT
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] 2011-10-09 15:17:15 PDT
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] 2011-10-10 02:56:50 PDT
Created attachment 565886 [details] [diff] [review]
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] 2011-10-10 06:22:15 PDT
(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 17 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-11 02:27:09 PDT
https://hg.mozilla.org/mozilla-central/rev/8586a8f3929f

Note You need to log in before you can comment on or make changes to this bug.