Closed Bug 608158 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 4.0b8

People

(Reporter: iangilman, Assigned: raymondlee)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
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?
Depends on: 595521
Status: ASSIGNED → NEW
(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.
Status: NEW → ASSIGNED
Attached patch v1 (Required patch for 595521) (obsolete) — Splinter Review
Attachment #487335 - Flags: feedback?(ian)
Attachment #487335 - Attachment description: v1 → v1 (Required patch for 595521)
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.
Attachment #487335 - Flags: feedback?(ian) → feedback-
Attached patch v1 (Required patch for 595521) (obsolete) — Splinter Review
Attachment #487335 - Attachment is obsolete: true
Attachment #487522 - Flags: feedback?(ian)
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.
Attachment #487522 - Flags: feedback?(ian) → feedback+
Attached patch v1 (obsolete) — Splinter Review
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
Attachment #487522 - Attachment is obsolete: true
Attachment #487794 - Flags: review?(dao)
Priority: -- → P3
Attachment #487794 - Flags: review?(dao)
Attachment #487794 - Flags: review+
Attachment #487794 - Flags: approval2.0?
Sent it to try b455d061e8d6 and waiting for the result.
(In reply to comment #8)
> Sent it to try b455d061e8d6 and waiting for the result.

Passed Try
Could someone give approval2.0 to this please?
Comment on attachment 487794 [details] [diff] [review]
v1

a+uir=beltzner
Attachment #487794 - Flags: approval2.0? → approval2.0+
Raymond, what's the status of this bug?
Sent it to try and waiting for the rssult
Attachment #487794 - Attachment is obsolete: true
Passed try
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/d0cbb80b60e8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Verified in recent nightly minefield build
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.