595374 - Panorama doesn't go back to the right tab if it was an app tab

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Ian Gilman [:iangilman]]

If you use "exit" from the Panorama UI (such as hitting the escape key), and
the last tab you had been on was an app tab, it should go there (with the group
you had been using).

Comment 1

[juan becerra [:juanb]]

This seems to work for me on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre

Comment 2

[Jürgen Kreileder]

I see the issue with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100914 Firefox/4.0b7pre

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Ian, is bug 597358 a duplicate of this?

Comment 4

[Ian Gilman [:iangilman]]

(In reply to comment #3)
> Ian, is bug 597358 a duplicate of this?

It is not.

Comment 5

[Matt Brubeck (:mbrubeck)]

Nominating as a blocker.  In recent nightlies I've tested, the Panorama exit button and exit keyboard shortcuts do nothing if the last tab was an app tab.

This also means that if your only open tab is an app tab, then activating Panorama leads to an empty gray window with no apparant way to exit.  (As a workaround, you can create a new group and then "switch" to that group.)

I'm seeing these issues in nightly build:
Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to comment #5)
> This also means that if your only open tab is an app tab, then activating
> Panorama leads to an empty gray window with no apparant way to exit.  (As a
> workaround, you can create a new group and then "switch" to that group.)

This is bug 595943 which was fixed yesterday.  Yay!

> In recent nightlies I've tested, the Panorama exit button and exit
> keyboard shortcuts do nothing if the last tab was an app tab.

This part is still true in the latest nightly.

Comment 7

[Ian Gilman [:iangilman]]

Attachment: patch v2 (requires 595371 patch)

Note that the unit test breaks if 595371 hasn't been applied (well, it breaks anyway, but that's a different story; see below).

Done: 

+ Cleaned up the button html
+ When in the Panorama UI, the escape key, option/ctrl+space, and the exit button all now exit back to the last tab you were on (regardless of what is selected in the UI), whether it's an app tab or not.
+ Removed some code that was making the exit keys not work if no tab was selected in the UI
+ The arrow keys now work if no tab is selected in the UI
+ Refactored the various code paths to the exit function
+ Added a setTimeout for the exit path from option/ctrl+space to deal with an issue with hiding the UI (bug 599157) 
+ Updated the exit button unit test for this new scenario

TODO:

+ The unit test runs clean by itself, but screws up when run with the other tabview tests

Comment 8

[Ian Gilman [:iangilman]]

Another TODO: 

+ As mitcho has rightly pointed out, we should put the setTimeout only in the non-animation path (since that's where it's needed, and otherwise it'll just slow things down)

Comment 9

[Ian Gilman [:iangilman]]

Attachment: patch v3 (requires 595371 patch)

Addressed all of the TODO items above.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 478397 [details] [diff] [review]
patch v3 (requires 595371 patch)

>       // ___ currentTab
>       this._currentTab = gBrowser.selectedTab;
>-
>+      

nit: You've added space here... also elsewhere, I believe.

>       function getClosestTabBy(norm) {
>         var centers =
>           [[item.bounds.center(), item]
>              for each(item in TabItems.getItems()) if (!item.parent || !item.parent.hidden)];
>-        var myCenter = self.getActiveTab().bounds.center();
>+        let activeTab = self.getActiveTab();
>+        var myCenter = (activeTab ? activeTab.bounds.center() : new Point(0, 0));

So, if there's no active tab (which we might get rid of elsewhere, I see) then we pretend we clicked on 0,0? Won't this just pick out the top-left-most tab? Is this desired behavior, or a desired behavior change? Is this important for this bug?

>+      // For some reason, we can't hide the TabView UI from within the 
>+      // control/option+space key handler (even though it works fine for the
>+      // escape key handler), so we add this timeout. Bug 599157.
>+      // We have the timeout here rather than in the key handler, since we 
>+      // don't need it in the zoom case (above), and it would in fact slow 
>+      // things down for that case. 
>+      setTimeout(function() {
>+        self.goToTab(gBrowser.selectedTab);
>+      }, 0);

How are we ensuring that when we do this, we go into the app tab with the last group? Perhaps I missed that part.

>+  window.addEventListener("tabviewshown", onTabViewWindowLoaded, false);

Why was this renamed onTabViewWindowLoaded? Seems to me that onTabViewShown is a more logical name. (moving it out of the test function is a fine choice, though)

(Oh, I see, it conflicts. Then call the second one onTabViewShownAgain or something, maybe?)

General test comment:
The test logic/sequence looks good. But isn't this really only a good test if we add another group which, ostensibly, could also be a fine choice for opening up with the app tab?

OVERALL:
Feedback+, though I'm slightly confused as to how (or which part of) this patch really fixes the bug.

Comment 11

[Ian Gilman [:iangilman]]

Attachment: patch v5 (requires 595371 patch)

(In reply to comment #10)
> nit: You've added space here... also elsewhere, I believe.

Fixed.

> So, if there's no active tab (which we might get rid of elsewhere, I see) then
> we pretend we clicked on 0,0? Won't this just pick out the top-left-most tab?
> Is this desired behavior, or a desired behavior change? Is this important for
> this bug?

This is the arrow key handling code, which assumed there was an active tab. To fix the bug, I had to remove the code that cancelled all key handling if there was no active tab. The arrows were the only keys that couldn't handle the "no active tab" case, so I fixed them. On the other hand, you've pointed out a flaw in my logic which will require the code to be a bit more extensive (0, 0 only works for the right and down keys), and at any rate, bug 596729 will make this obsolete anyway. Given that, I've now simplified it by adding a "don't do arrow keys if no selected tab" check.

By the way, looks like we will need to keep it possible for there to be no active tab, at least until we have more robust app tab handling (bug 600665), so we can support the behavior required for this bug; if you return to the Panorama UI from an app tab, we shouldn't automatically select some other tab, otherwise that's where you'll go to when you exit. See the updated patch. 

> How are we ensuring that when we do this, we go into the app tab with the last
> group? Perhaps I missed that part.

I was just assuming that you'd go back to how the tab bar was when you came from it, but as you point out, this doesn't work, as you might have reorganized your groups before exiting. I believe I have this fixed now. 

> Why was this renamed onTabViewWindowLoaded? Seems to me that onTabViewShown is
> a more logical name. (moving it out of the test function is a fine choice,
> though)

I was just copying Raymond's code from a different test. Anyway, I like the name, as that's what we're doing there... making sure Tab View is loaded. I suppose it could be called onTabViewLoadedAndShown; yes, I think I'll do that. Done.

> The test logic/sequence looks good. But isn't this really only a good test if
> we add another group which, ostensibly, could also be a fine choice for opening
> up with the app tab?

I've actually changed the logic a bit from the original description of the bug; at first I thought you should go back to how things were, but the fact is you could have changed stuff. Now I'm going with something more like what we already had; it takes you into the active tab/group, with the exception being that if you've come straight from an app tab, there's no active tab, so it goes back to the app tab, but it uses whatever group is active. 

So, I feel the test we have in this patch properly tests the point of the bug (being "if the last tab was an app tab, do you go back to it on exit?"). 

> Feedback+, though I'm slightly confused as to how (or which part of) this patch
> really fixes the bug.

Is it more clear with the new patch?

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 479561 [details] [diff] [review]
patch v5 (requires 595371 patch)

Moving this check into UI.hideTabView makes more sense to me. I like it. Moving the _updateTabBar call out of the group code itself also makes sense.

I do wonder whether we should move _updateTabBar to UI, though? Though that probably should be a different bug.

Comment 13

[Ian Gilman [:iangilman]]

(In reply to comment #12)

> I do wonder whether we should move _updateTabBar to UI, though? Though that
> probably should be a different bug.

I think it's fine there... It is based on the active GroupItem, after all.

Comment 15

[Ian Gilman [:iangilman]]

Attachment: patch v6 (requires 595371 patch)

Now that we're using command+e instead of option+space, I can remove a setTimeout; done.

Comment 16

[Ian Gilman [:iangilman]]

Attachment: patch v7 (requires 595371 patch)

We weren't dismissing the "zoomPrep" properly when we didn't do the zoom animation; fixed. 

Also, switching review to Dietrich to lighten Dolske's load.

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 481717 [details] [diff] [review]
patch v7 (requires 595371 patch)


>+    // If there's an active TabItem, zoom into it. If not (when the selected tab
>+    // is an app tab), just go there. 
>+    let activeTabItem = this.getActiveTab();
>+    if (!activeTabItem)
>+      activeTabItem = gBrowser.selectedTab.tabItem;
>+      
>+    if (activeTabItem)
>+      activeTabItem.zoomIn(); 
>+    else
>+      self.goToTab(gBrowser.selectedTab);
>   },

i'd prefer a more explicit tabItem.isAppTab boolean approach (EIBTI!). it's less regression-prone, easier to read, and is more future-proof. or you could go the other way and explicitly classify some tabs as "untracked" or something.

but i won't withhold review for that, so r=me!

Comment 18

[Ian Gilman [:iangilman]]

(In reply to comment #17)
> i'd prefer a more explicit tabItem.isAppTab boolean approach (EIBTI!). it's
> less regression-prone, easier to read, and is more future-proof. or you could
> go the other way and explicitly classify some tabs as "untracked" or something.

In bug 600665 we will be moving towards having app tabs properly accounted for. 

In this routine we do really want to fall through for any tab missing a tab item, though, regardless of whether it's an app tab or not (defensive coding). I'll tweak the comment a little to reflect that.

Comment 19

[Ian Gilman [:iangilman]]

Attachment: patch for checkin

Sending to try

Comment 20

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/380c7bd4eb25

Comment 21

[Tracy Walker [:tracy]]

verified with 20101013 build of minefield