637840 - after closing a group in panorama, focus should go to the last used tab in the last used group

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P5)

Description

[eyal gruss]

1. after closing a group or the last tab in a group in panorama - focus goes to the nearest tab in a remaining group

2. after closing the last tab in a group in the browser window - focus goes to the first tab in the first group (as far as i can tell)

expected result:
1. focus is consistent if the group is closed either in browser or in panorama
2. focus should always go to the last used tab in the last used group
3. fixing (2) will also ensure that if i close group B while focused in group A, focus will not change

Comment 1

[Raymond Lee [:raymondlee]]

(In reply to comment #0)
> 1. after closing a group or the last tab in a group in panorama - focus goes to
> the nearest tab in a remaining group
> 
> 2. after closing the last tab in a group in the browser window - focus goes to
> the first tab in the first group (as far as i can tell)
> 

In the latest trunk, the focus goes to the nearest tab in a remaining group for both 1 and 2.  Does it work the same for you?

Comment 2

[eyal gruss]

with 4.0 release i can no longer recreate the inconsistent behavior (2) ... going to the first tab.

i am morphing this bug to address only the MRU issue (expected results 2 and 3)

Comment 3

[Kevin Hanes]

bugspam

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v1

This patch creates an array to hold the last active group items.  When the last item a group is removed and the group is closed or a group gets hidden, it would look for for the last active group item in the array and set the group to active.  If the array is empty, it would set the focus on the closest tab item.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 545864 [details] [diff] [review]
v1

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

Looks good, but maybe we should create a MRUList class that provides an API like:

MRUList
  -> update(entry) - updates/inserts the given entry and sorts it as the most recently used
  -> remove(entry) - removes an entry from the MRU list
  -> peek() - returns the most recently used entry

That way we have a nice self-describing MRU list that handles everything internally and doesn't clutter GroupItem(s) itself (and is re-usable). Feel free to change the MRUList API according to your needs, this is just a quick sketch :)

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v2

Added MRUList

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 557456 [details] [diff] [review]
v2

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

Looks good, thanks! r=me with those issues addressed/questions answered.

::: browser/base/content/tabview/groupitems.js
@@ +715,5 @@
> +   let groupItem = GroupItems.getLastActiveGroupItem();
> +   if (groupItem)
> +     UI.setActive(groupItem);
> +   else
> +     this._makeClosestTabActive();

Can we move those lines into a separate function? We can call that function from below (where the same lines appear again).

@@ +1157,5 @@
> +        let groupItem = GroupItems.getLastActiveGroupItem();
> +        if (groupItem)
> +          UI.setActive(groupItem);
> +        else
> +          this._makeClosestTabActive();

(see above)

@@ +2434,5 @@
>        iQ(this._activeGroupItem.container).removeClass('activeGroupItem');
>  
>      iQ(groupItem.container).addClass('activeGroupItem');
>  
> +    this._lastActiveList.update(groupItem.id);

Is there a reason we use the groupItem ID here? Can't we use the whole groupItem object?

@@ +2457,5 @@
> +        return false;
> +      }
> +    });
> +
> +    return lastActiveGroupItem;

We don't need the variable "lastActiveGroupItem" here because we can just return the return value of .peek().

::: browser/base/content/tabview/modules/utils.jsm
@@ +796,5 @@
> +  // ----------
> +  // Function: toString
> +  // Prints [List (entry1, entry2, ...)] for debug use
> +  toString: function MRUList_toString() {
> +    return "[List (" + this._list.join(",") + ")]";

Nit: Let's make that .join(", ") for a nicer format.

@@ +804,5 @@
> +  // Function: update
> +  // Updates/inserts the given entry as the most recently used one in the list.
> +  update: function MRUList_update(entry) {
> +    this.remove(entry);
> +    this._list.splice(0, 0, entry);

Please use "this._list.unshift(entry)" here (so it's a bit more obvious what we do here).

::: browser/base/content/test/tabview/browser_tabview_bug637840.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// create three group two groups with two items and one group with on item

Please fix that comment.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

> ::: browser/base/content/tabview/groupitems.js
> @@ +715,5 @@
> > +   let groupItem = GroupItems.getLastActiveGroupItem();
> > +   if (groupItem)
> > +     UI.setActive(groupItem);
> > +   else
> > +     this._makeClosestTabActive();
> 
> Can we move those lines into a separate function? We can call that function
> from below (where the same lines appear again).
Fixed

> 
> @@ +1157,5 @@
> > +        let groupItem = GroupItems.getLastActiveGroupItem();
> > +        if (groupItem)
> > +          UI.setActive(groupItem);
> > +        else
> > +          this._makeClosestTabActive();
> 
> (see above)
Fixed

> 
> @@ +2434,5 @@
> >        iQ(this._activeGroupItem.container).removeClass('activeGroupItem');
> >  
> >      iQ(groupItem.container).addClass('activeGroupItem');
> >  
> > +    this._lastActiveList.update(groupItem.id);
> 
> Is there a reason we use the groupItem ID here? Can't we use the whole
> groupItem object?
Replaced id with groupItem

> 
> @@ +2457,5 @@
> > +        return false;
> > +      }
> > +    });
> > +
> > +    return lastActiveGroupItem;
> 
> We don't need the variable "lastActiveGroupItem" here because we can just
> return the return value of .peek().
> 
Fixed

> ::: browser/base/content/tabview/modules/utils.jsm
> @@ +796,5 @@
> > +  // ----------
> > +  // Function: toString
> > +  // Prints [List (entry1, entry2, ...)] for debug use
> > +  toString: function MRUList_toString() {
> > +    return "[List (" + this._list.join(",") + ")]";
> 
> Nit: Let's make that .join(", ") for a nicer format.
> 
> @@ +804,5 @@
> > +  // Function: update
> > +  // Updates/inserts the given entry as the most recently used one in the list.
> > +  update: function MRUList_update(entry) {
> > +    this.remove(entry);
> > +    this._list.splice(0, 0, entry);
> 
> Please use "this._list.unshift(entry)" here (so it's a bit more obvious what
> we do here).
Fixed

> 
> ::: browser/base/content/test/tabview/browser_tabview_bug637840.js
> @@ +1,4 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +// create three group two groups with two items and one group with on item
> 
> Please fix that comment.
Removed.  We don't actually need this comment

Push to try and waiting for the results.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&tree=Try&rev=387f42f23f88

Comment 12

[Raymond Lee [:raymondlee]]

> Push to try and waiting for the results.
> https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&tree=Try&rev=387f42f23f88

The results look good  but Win64 opt is still waiting to be built.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/e4ab06518d81

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/e4ab06518d81

Comment 15

[eyal gruss]

just checked this on 9.0b1
item 2 in comment 0 is broken
after closing the last tab in a group from the browser window - panorama shows an empty group which apparently also takes the focus (no tab has focus).
note that the same action i.e. closing the last tab in a group done in panorama itself: correctly closes the empty group and gives focus to the last used tab in the last used group

Comment 16

[Raymond Lee [:raymondlee]]

(In reply to eyal gruss (eyaler) from comment #15)
> just checked this on 9.0b1
> item 2 in comment 0 is broken
> after closing the last tab in a group from the browser window - panorama
> shows an empty group which apparently also takes the focus (no tab has
> focus).

bug 705621 would address this.

> note that the same action i.e. closing the last tab in a group done in
> panorama itself: correctly closes the empty group and gives focus to the
> last used tab in the last used group

Empty group would not be closed automatically. See bug 663421