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

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Alex Limi (:limi) — Firefox UX Team]

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.

Comment 1

[Ian Gilman [:iangilman]]

Assigning to Aza for design.

Comment 2

[Alex Limi (:limi) — Firefox UX Team]

Should probably block beta6, since this looks like data loss (even if you can technically get your stuff back if you know how).

Comment 3

[u88484]

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.

Comment 4

[Aza Raskin [:aza]]

Agreed that this should block beta 6. Expect more on the design solution soon.

Comment 5

[Aza Raskin [:aza]]

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.)

Comment 6

[Aza Raskin [:aza]]

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.

Comment 7

[Johnathan Nightingale [:johnath]]

Marking this betaN+ but would *really* like to see it fixed asap.

Comment 8

[Ian Gilman [:iangilman]]

Attachment: patch v1

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.

Comment 9

[Sean Dunn]

Comment on attachment 475693 [details] [diff] [review]
patch v1

Nitpicks:
Multiple lines > 80 columns. Needs more flare.

Comment 10

[Ian Gilman [:iangilman]]

Pushed to try.

Comment 11

[Justin Dolske [:Dolske]]

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

Comment 12

[Ian Gilman [:iangilman]]

Attachment: patch for check-in [r=dolske, a=blocking]

Comment 13

[Ian Gilman [:iangilman]]

Follow-up filed for dolske's feedback:

bug 596833

Comment 14

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/5b88f6330deb

Comment 15

[eyal gruss]

is Bug 596591 a dupe?

Comment 17

[Tracy Walker [:tracy]]

verified on recent nightly builds of minefield

Comment 18

[Tracy Walker [:tracy]]

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

Comment 19

[Ian Gilman [:iangilman]]

(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.