The default bug view has changed. See this FAQ.

Closing last tab of a group doesn't show Panorama

RESOLVED FIXED in Firefox 10

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ithinc, Assigned: raymondlee)

Tracking

Trunk
Firefox 10
Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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?
(Reporter)

Updated

6 years ago
Version: unspecified → Trunk
(Reporter)

Updated

6 years ago
Depends on: 654311
Blocks: 653099
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

6 years ago
Created attachment 531956 [details] [diff] [review]
v1
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #531956 - Flags: feedback?(tim.taubert)
(Reporter)

Comment 2

6 years ago
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
(Reporter)

Comment 3

6 years ago
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.
Depends on: 643392
(Assignee)

Comment 4

6 years ago
Created attachment 532869 [details] [diff] [review]
v2

Please apply patch for Bug 643392 before this patch.
Attachment #532869 - Flags: feedback?(tim.taubert)
(Assignee)

Comment 5

6 years ago
(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.
(Reporter)

Comment 6

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #531956 - Attachment is obsolete: true
Attachment #531956 - Flags: feedback?(tim.taubert)
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
@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
(Reporter)

Comment 12

6 years ago
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+
(Assignee)

Comment 14

6 years ago
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
Attachment #532869 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
(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/integration/mozilla-inbound/rev/8586a8f3929f
Keywords: checkin-needed
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/8586a8f3929f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.