608158 - Exiting Panorama when nothing but hidden groups and no app tabs, should create new tab

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Ian Gilman [:iangilman]]

It's possible to hit the Panorama exit (either the key or the button) when there are no tabs except those in hidden (closed but with option to undo) groups. If you do so, those hidden tabs will get flushed and the browser will close (since you have no more tabs in it).

Instead, when this happens, I feel it should create a new tab and drop you into that.

Comment 1

[Raymond Lee [:raymondlee]]

Once patch in bug 595521 lands, it wouldn't be possible to exit Panorama.  Do we still want to create a new tab and drop user into that?

Comment 2

[Kevin Hanes]

(In reply to comment #1)
> Once patch in bug 595521 lands, it wouldn't be possible to exit Panorama.  Do
> we still want to create a new tab and drop user into that?

Nothing in that bug appears to keep Panorama from closing, unless you've explicitly added something to prevent that. In the case of bug 595521, Panorama should still close if you hit the Panorama button after you've closed the last group (with the undo close group button showing) unless this bug gets implemented.

That being the case, I do think we want the behavior Ian describes.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1 (Required patch for 595521)

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 487335 [details] [diff] [review]
v1 (Required patch for 595521)

Looks good. Comments: 

* Seems like it would be good to count tabs in the test; make sure you start out and end up with the right number. 

>     let activeTabItem = this.getActiveTab();
>-    if (!activeTabItem)
>-      activeTabItem = gBrowser.selectedTab.tabItem;
>-      
>+    if (!activeTabItem) {
>+      let tabItem = gBrowser.selectedTab.tabItem;
>+      if (tabItem && (!tabItem.parent || !tabItem.parent.hidden))
>+        activeTabItem = tabItem;
>+    }
>+
>     if (activeTabItem)
>-      activeTabItem.zoomIn(); 
>+      activeTabItem.zoomIn();
>     else
>       self.goToTab(gBrowser.selectedTab);

The check to see if gBrowser.selectedTab is in a hidden group is good, but unfortunately it doesn't really cover it, as we'll fall through to the last line above which tries to go to it anyway. 

Instead, if the parent is hidden, pick another tab that's not hidden.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1 (Required patch for 595521)

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 487522 [details] [diff] [review]
v1 (Required patch for 595521)

>+        if (!tabItem.parent || !tabItem.parent.hidden) {
>+          activeTabItem = tabItem;
>+        } else { // has parent and it is not hidden

That should read "has parent and it is hidden", right?

Otherwise looks good.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v1

Updated the comment to make it more clear.

+        } else { // set active tab item if there is at least one unhidden group
+          if (unhiddenGroups.length > 0)
+            activeTabItem = unhiddenGroups[0].getActiveTab();
+        }

f=ian

Comment 8

[Raymond Lee [:raymondlee]]

Sent it to try b455d061e8d6 and waiting for the result.

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to comment #8)
> Sent it to try b455d061e8d6 and waiting for the result.

Passed Try

Comment 10

[Raymond Lee [:raymondlee]]

Could someone give approval2.0 to this please?

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 487794 [details] [diff] [review]
v1

a+uir=beltzner

Comment 12

[Ian Gilman [:iangilman]]

Raymond, what's the status of this bug?

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Sent it to try and waiting for the rssult

Comment 14

[Raymond Lee [:raymondlee]]

Passed try

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d0cbb80b60e8

Comment 16

[Tracy Walker [:tracy]]

Verified in recent nightly minefield build