Closed Bug 654295 Opened 13 years ago Closed 13 years ago

Closing last tab of a group doesn't show Panorama

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: tabutils+bugzilla, Assigned: raymondlee)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
Version: unspecified → Trunk
Depends on: 654311
Blocks: 653099
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #531956 - Flags: feedback?(tim.taubert)
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
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.
Attached patch v2 (obsolete) — Splinter Review
Please apply patch for Bug 643392 before this patch.
Attachment #532869 - Flags: feedback?(tim.taubert)
(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.
(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.
Attachment #531956 - Attachment is obsolete: true
Attachment #531956 - Flags: feedback?(tim.taubert)
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
(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.
@Tim: could you feedback on this as well please?
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
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 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.
Attachment #532869 - Flags: feedback?(ttaubert) → feedback+
(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
Attachment #532869 - Attachment is obsolete: true
(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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8586a8f3929f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: