stay on app tab when switching groups

RESOLVED FIXED in Firefox 11

Status

Firefox Graveyard
Panorama
P4
normal
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: iangilman, Assigned: raymondlee)

Tracking

Trunk
Firefox 11

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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.
Priority: -- → P4

Comment 1

6 years ago
+1 on Ian's idea.
Assignee: aza → ian
(Reporter)

Updated

6 years ago
Depends on: 600665
We'll punt on this, but 600665 would be a good start if someone wants to look at that in time for beta 11.
Assignee: ian → nobody
Target Milestone: --- → Future
(Assignee)

Comment 3

6 years ago
Created attachment 577894 [details] [diff] [review]
v1

This fixes the issue mentioned in comment 0
Attachment #577894 - Flags: review?(ttaubert)
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'.
Attachment #577894 - Flags: review?(ttaubert) → review-
(Assignee)

Comment 5

6 years ago
Created attachment 577957 [details] [diff] [review]
v2

Updated the patch and added test.
Attachment #577894 - Attachment is obsolete: true
Attachment #577957 - Flags: review?(ttaubert)
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'.
Attachment #577957 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 7

6 years ago
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
Assignee: nobody → raymond
Attachment #577957 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

6 years ago
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
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1938652d8c9e
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Target Milestone: Future → Firefox 11
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/1938652d8c9e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.