Last Comment Bug 608153 - stay on app tab when switching groups
: stay on app tab when switching groups
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P4 normal
: Firefox 11
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on: 600665
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-28 17:02 PDT by Ian Gilman [:iangilman]
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (4.47 KB, patch)
2011-11-30 01:17 PST, Raymond Lee [:raymondlee]
ttaubert: review-
Details | Diff | Review
v2 (5.82 KB, patch)
2011-11-30 07:44 PST, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Review
Patch for checkin (6.03 KB, patch)
2011-11-30 20:55 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Review

Description Ian Gilman [:iangilman] 2010-10-28 17:02:32 PDT
If you hit the "next group" key while in a tab group (i.e. not in the Panorama UI), it remembers which tab you were in in each group and takes you back to it as you go from group to group. With bug 600665 this even works for app tabs. 

I wonder, though, if maybe we want to provide an exception: if you're on an app tab when you hit "next group" perhaps it should keep you on that app tab but just change the group (seeing as how app tabs are present in every group).

Assigning to Aza for his thoughts.
Comment 1 Aza Raskin [:aza] 2010-12-27 17:08:02 PST
+1 on Ian's idea.
Comment 2 Michael Yoshitaka Erlewine [:mitcho] 2011-01-21 09:28:49 PST
We'll punt on this, but 600665 would be a good start if someone wants to look at that in time for beta 11.
Comment 3 Raymond Lee [:raymondlee] 2011-11-30 01:17:46 PST
Created attachment 577894 [details] [diff] [review]
v1

This fixes the issue mentioned in comment 0
Comment 4 Tim Taubert [:ttaubert] 2011-11-30 05:25:11 PST
Comment on attachment 577894 [details] [diff] [review]
v1

Review of attachment 577894 [details] [diff] [review]:
-----------------------------------------------------------------

Please address the issue mentioned below and add a regression test for this new behavior. Thanks!

::: browser/base/content/browser-tabview.js
@@ +355,5 @@
>  
> +          if (gBrowser.selectedTab.pinned) {
> +            let ui = self._window.UI;
> +            ui.setActive(tabItem.parent, {dontSetActiveTabInGroup: true});
> +            groupItems.updateTabBar();

I think it would be better to use GroupItems.updateActiveGroupItemAndTabBar() here. We should add a second parameter to pass options to UI.setActive() like 'dontSetActiveTabInGroup'.
Comment 5 Raymond Lee [:raymondlee] 2011-11-30 07:44:47 PST
Created attachment 577957 [details] [diff] [review]
v2

Updated the patch and added test.
Comment 6 Tim Taubert [:ttaubert] 2011-11-30 11:37:53 PST
Comment on attachment 577957 [details] [diff] [review]
v2

Review of attachment 577957 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Raymond, looks great!

r=me with those tiny bits fixed.

::: browser/components/tabview/groupitems.js
@@ +2454,5 @@
>    // Sets active TabItem and GroupItem, and updates tab bar appropriately.
> +  // Parameters:
> +  // tabItem - the tab item.
> +  // options -
> +  //  dontSetActiveTabInGroup bool for not setting active tab in group

Nit: it's sufficient to say that 'options' is passed to UI.setActive(). So we don't need to keep those docs synced.

@@ +2455,5 @@
> +  // Parameters:
> +  // tabItem - the tab item.
> +  // options -
> +  //  dontSetActiveTabInGroup bool for not setting active tab in group
> +  updateActiveGroupItemAndTabBar: function GroupItems_updateActiveGroupItemAndTabBar(tabItem, options) {

Nit: Please break that line before 'function'.
Comment 7 Raymond Lee [:raymondlee] 2011-11-30 20:55:00 PST
Created attachment 578170 [details] [diff] [review]
Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #6)
> r=me with those tiny bits fixed.
> 
> ::: browser/components/tabview/groupitems.js
> @@ +2454,5 @@
> >    // Sets active TabItem and GroupItem, and updates tab bar appropriately.
> > +  // Parameters:
> > +  // tabItem - the tab item.
> > +  // options -
> > +  //  dontSetActiveTabInGroup bool for not setting active tab in group
> 
> Nit: it's sufficient to say that 'options' is passed to UI.setActive(). So
> we don't need to keep those docs synced.

Updated the docs

> 
> @@ +2455,5 @@
> > +  // Parameters:
> > +  // tabItem - the tab item.
> > +  // options -
> > +  //  dontSetActiveTabInGroup bool for not setting active tab in group
> > +  updateActiveGroupItemAndTabBar: function GroupItems_updateActiveGroupItemAndTabBar(tabItem, options) {
> 
> Nit: Please break that line before 'function'.

Fixed.


Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=67f7a8ee67ce
Comment 8 Raymond Lee [:raymondlee] 2011-12-01 06:58:57 PST
There are two oranges but it seems unrelated.  
https://tbpl.mozilla.org/?tree=Try&rev=67f7a8ee67ce

Resubmitted, one known intermittence, and passed Try
https://tbpl.mozilla.org/?tree=Try&rev=26eb8984b1bb
Comment 9 Tim Taubert [:ttaubert] 2011-12-01 22:42:01 PST
https://hg.mozilla.org/integration/fx-team/rev/1938652d8c9e
Comment 10 Tim Taubert [:ttaubert] 2011-12-02 05:59:03 PST
https://hg.mozilla.org/mozilla-central/rev/1938652d8c9e

Note You need to log in before you can comment on or make changes to this bug.