Closed Bug 595374 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P2)

defect

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: iangilman, Assigned: iangilman)

References

Details

Attachments

(1 file, 5 obsolete files)

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).
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
Depends on: 578553
I see the issue with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100914 Firefox/4.0b7pre
Priority: -- → P2
Ian, is bug 597358 a duplicate of this?
(In reply to comment #3)
> Ian, is bug 597358 a duplicate of this?

It is not.
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
blocking2.0: --- → ?
(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.
Assignee: nobody → ian
Blocks: 597043
blocking2.0: ? → betaN+
Status: NEW → ASSIGNED
Attached patch patch v2 (requires 595371 patch) (obsolete) — Splinter Review
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
Attachment #478106 - Flags: feedback?(mitcho)
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)
Attached patch patch v3 (requires 595371 patch) (obsolete) — Splinter Review
Addressed all of the TODO items above.
Attachment #478106 - Attachment is obsolete: true
Attachment #478397 - Flags: feedback?(mitcho)
Attachment #478106 - Flags: feedback?(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.
Attachment #478397 - Flags: feedback?(mitcho) → feedback+
Blocks: 600665
Attached patch patch v5 (requires 595371 patch) (obsolete) — Splinter Review
(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?
Attachment #478397 - Attachment is obsolete: true
Attachment #479561 - Flags: feedback?(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.
Attachment #479561 - Flags: feedback?(mitcho) → feedback+
(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.
Attachment #479561 - Flags: review?(dolske)
Duplicate of this bug: 601502
Attached patch patch v6 (requires 595371 patch) (obsolete) — Splinter Review
Now that we're using command+e instead of option+space, I can remove a setTimeout; done.
Attachment #479561 - Attachment is obsolete: true
Attachment #480785 - Flags: review?(dolske)
Attachment #479561 - Flags: review?(dolske)
Blocks: 597358
Blocks: 601915
Blocks: 601923
Attached patch patch v7 (requires 595371 patch) (obsolete) — Splinter Review
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.
Attachment #480785 - Attachment is obsolete: true
Attachment #481717 - Flags: review?(dietrich)
Attachment #480785 - Flags: review?(dolske)
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!
Attachment #481717 - Flags: review?(dietrich) → review+
(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.
Sending to try
Attachment #481717 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/380c7bd4eb25
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
verified with 20101013 build of minefield
Status: RESOLVED → VERIFIED
Duplicate of this bug: 606822
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.