Last Comment Bug 637840 - after closing a group in panorama, focus should go to the last used tab in the last used group
: after closing a group in panorama, focus should go to the last used tab in th...
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P5 normal
: Firefox 9
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on: 663421 705621
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-01 13:08 PST by eyal gruss (eyaler)
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (4.52 KB, patch)
2011-07-14 02:28 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v2 (12.49 KB, patch)
2011-09-01 03:58 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (13.10 KB, patch)
2011-09-05 22:30 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description eyal gruss (eyaler) 2011-03-01 13:08:47 PST
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] 2011-03-29 00:13:46 PDT
(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 (eyaler) 2011-03-29 01:54:51 PDT
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 2011-03-31 10:51:57 PDT
bugspam
Comment 4 Tim Taubert [:ttaubert] 2011-05-27 02:23:36 PDT
bugspam
Comment 5 Tim Taubert [:ttaubert] 2011-05-27 02:28:37 PDT
bugspam
Comment 6 Raymond Lee [:raymondlee] 2011-07-14 02:28:00 PDT
Created attachment 545864 [details] [diff] [review]
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] 2011-07-24 18:39:22 PDT
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] 2011-08-30 00:40:11 PDT
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] 2011-09-01 03:58:04 PDT
Created attachment 557456 [details] [diff] [review]
v2

Added MRUList
Comment 10 Tim Taubert [:ttaubert] 2011-09-05 11:45:02 PDT
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] 2011-09-05 22:30:16 PDT
Created attachment 558396 [details] [diff] [review]
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] 2011-09-06 20:43:04 PDT
> 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] 2011-09-07 01:08:41 PDT
http://hg.mozilla.org/integration/fx-team/rev/e4ab06518d81
Comment 14 Tim Taubert [:ttaubert] 2011-09-08 02:34:42 PDT
http://hg.mozilla.org/mozilla-central/rev/e4ab06518d81
Comment 15 eyal gruss (eyaler) 2011-11-11 02:01:38 PST
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] 2011-11-29 21:40:59 PST
(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

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