608153 - stay on app tab when switching groups

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Ian Gilman [:iangilman]]

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]]

+1 on Ian's idea.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

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]]

Attachment: v1

This fixes the issue mentioned in comment 0

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: v2

Updated the patch and added test.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: 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]]

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] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/1938652d8c9e

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1938652d8c9e