Closed Bug 595943 Opened 9 years ago Closed 9 years ago

Closing last tab when app tabs are present causes weird state in Panorama

Categories

(Firefox Graveyard :: Panorama, defect, P1)

x86
All
defect

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: limi, Assigned: iangilman)

References

Details

Attachments

(1 file, 1 obsolete file)

How to reproduce:

1. Create some app tabs
2. Close all the tabs that are not app tabs — when the last "normal" tab is closed, you're suddenly in Panorama mode with no groups, and no obvious way to get back to your app tabs

A couple of things that could fix this:

* Make sure that an app tab is selected when the last normal tab is closed

* If there are no groups defined, show app tabs in a container somewhere in Panorama view, or always keep them in a "dummy group"

It seems a bit weird that you can ever end up in a state with no groups in Panorama in the first place — there should probably always be at least one.
Assigning to Aza for design.
Assignee: nobody → aza
Should probably block beta6, since this looks like data loss (even if you can technically get your stuff back if you know how).
blocking2.0: --- → ?
I'm constantly finding myself in this state.  Ctrl+clicking on the minefield taskbar doesn't always work to get me out of this state either.
Agreed that this should block beta 6. Expect more on the design solution soon.
Proposed design specification:

* Closing all non-app tabs causes the last-selected app-tab to become focused.
* This is represented in Panorama as a group with no elements, but app-tab icons on the side
* We do not allow you to close the last group if there are app-tabs (and we remove the close icon for that last group). If there is a group and an orphaned tab we also do not let you close the last group. (Alternatively, we can let you close the group and instantly put the orphaned tab into a group.)
Blocks: 594094
It looks like we will not have time to get this in for beta 6, which is too bad (as limi says, it feels like data loss). Unless someone feels strongly enough to write a patch with tests for tomorrow, this will be scheduled for beta 7.
Assignee: aza → ian
Marking this betaN+ but would *really* like to see it fixed asap.
blocking2.0: ? → betaN+
Priority: -- → P1
Attached patch patch v1 (obsolete) — Splinter Review
There are two issues in this bug (closing the last normal tab takes you to Panorama when it shouldn't, and there should always be a group in Panorama if there are app tabs). This patch addresses issue one. Issue two has now been filed as bug 596781.
Attachment #475693 - Flags: feedback?
Attachment #475693 - Flags: feedback? → feedback?(seanedunn)
Comment on attachment 475693 [details] [diff] [review]
patch v1

Nitpicks:
Multiple lines > 80 columns. Needs more flare.
Attachment #475693 - Flags: feedback?(seanedunn) → feedback+
Attachment #475693 - Flags: review?(dietrich)
Pushed to try.
Comment on attachment 475693 [details] [diff] [review]
patch v1

>+++ b/browser/base/content/tabview/ui.js
...
>+          // Don't return to TabView if there are any app tabs
>+          for (let a = 0; a < gBrowser.tabs.length; a++) {
>+            let theTab = gBrowser.tabs[a]; 
>+            if (theTab.pinned && gBrowser._removingTabs.indexOf(theTab) == -1) 
>+              return;
>+          }

I don't think you need to iterate over the whole list of tabs... AFAIK AppTabs are always at the beginning, so you could break early.

        for (let a = 0; a < gBrowser.tabs.length; a++) {
          let theTab = gBrowser.tabs[a];
          if (!theTab.pinned)
              break;
          if (gBrowser._removingTabs.indexOf(theTab) == -1) 
            return;
        }

r+ with thar+ with t
Attachment #475693 - Flags: review?(dietrich) → review+
Attachment #475693 - Attachment is obsolete: true
Follow-up filed for dolske's feedback:

bug 596833
OS: Mac OS X → All
Pushed http://hg.mozilla.org/mozilla-central/rev/5b88f6330deb
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
is Bug 596591 a dupe?
Duplicate of this bug: 596591
verified on recent nightly builds of minefield
Status: RESOLVED → VERIFIED
So in my initial checks using named groups, this was OK. However, using a single unnamed group still displays this problem.  closing the last non-App tab closes the unnamed group and makes it impossible.

STR:

1) create an app tab
2) organize all your tabs into a single group and close all but that group.
3) close all normal tabs

Tested results:
the last groups disappears an the Group View toggle button (and keyboard short cut) become unusable.  Only way out is to close the Window, which shuts down the browser, then restart.

Expected results: As long as there is even a single app tab, there should be a group remaining for it to live in Group View.


Note: it's also possible to break the Group View toggle button in similar scenario but with named or multiple empty groups. Though in those cases it is possible to exit Group view by new tab or clicking the app tab
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 597043
(In reply to comment #18)

> Expected results: As long as there is even a single app tab, there should be a
> group remaining for it to live in Group View.

This is now bug 596781 (see comment 8).

> Note: it's also possible to break the Group View toggle button in similar
> scenario but with named or multiple empty groups. Though in those cases it is
> possible to exit Group view by new tab or clicking the app tab

The Tab View toggle button not working when there's no non-app tab is fixed by the patch for bug 595374.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.