624939 - Closing all tabs while viewing an app tab makes Panorama go to the wrong group

Status: RESOLVED DUPLICATE of bug 612470

(Firefox Graveyard :: Panorama, defect)

Description

[Patrick Walton (:pcwalton)]

STR:

1. Open two or more Panorama groups with at least one tab in each.
2. View an app tab.
3. Close all tabs from the bar.
4. Open a tab.

Panorama switches back to the wrong group.

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

This patch fixes the bug by simply making groups not close by default if they have app tabs in them. Seemed to be the simplest solution, but possibly unexpected.

Comment 2

[Luke Iliffe (Harlequin99)]

This sounds like a dup of bug 596781 which also has a patch.

Comment 3

[Kevin Hanes]

(In reply to comment #1)
> Created attachment 503027 [details] [diff] [review]
> Proposed patch.
> 
> This patch fixes the bug by simply making groups not close by default if they
> have app tabs in them. Seemed to be the simplest solution, but possibly
> unexpected.

We're pretty explicit about closing empty groups (groups with no non-app tabs). To do otherwise would make it irritating to have to explicitly close empty groups and would clutter the ui.

Instead the fix for this bug should keep the group alive only until you either return to the Panorama UI or switch to a different group (via key command or menu item). 

This keeps you from jumping into a different group as well as preserving our group behaviors.

Comment 4

[Ian Gilman [:iangilman]]

(In reply to comment #2)
> This sounds like a dup of bug 596781 which also has a patch.

It is not; that bug applies only to the last group, whereas this could happen with any number of groups.

Comment 5

[Ian Gilman [:iangilman]]

Comment on attachment 503027 [details] [diff] [review]
Proposed patch.

I agree with Kevin's proposed fix.

Comment 6

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

Patch version 2 makes the suggested change, as I understand it.

Comment 7

[Raymond Lee [:raymondlee]]

I think this is a dup. of bug 612470.  If you think this patch fixes bug 612470, please make that as duplicate

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 503719 [details] [diff] [review]
Proposed patch, version 2.

Looks good, with a couple of exceptions: 

* You need to also cover the "switch to next group" key command (and delete the empty group); that's in TabView._setBrowserKeyHandlers()

* Seems like this needs a test.

Comment 9

[Ian Gilman [:iangilman]]

(In reply to comment #7)
> I think this is a dup. of bug 612470.  If you think this patch fixes bug
> 612470, please make that as duplicate

Yes, I'd say this is a dupe of bug 612470, which is a blocker. So, is it better to move the patch to the blocker, or move the blocking status to this bug?

Comment 10

[Raymond Lee [:raymondlee]]

Please submit patch to bug 612470 (a dupe of this bug) which has blocking2.0betaN+