Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0
Description:
When creating a group containing multiple tabs in Panorama and minimizing it in order to obtain a stack, the active tab is not displayed on the top of it.
Reproduceble always.
Steps to reproduce:
1. Create 3 or more tabs
2. Select last tab
3. Go into Panorama and minimize the group in order to create a stack.
Actual result:
The first tab created in step 1 is on top of the stack.
Expected result:
The last selected tab (the focused one) is on top of the stack.

This is an interesting question: can we get some utility out of the visual presentation of our stacks. It's one that we've wrestled with since we introduced them.
In any case, this seems like a good idea.

This patch only keeps the tab on top of the stack within the session.
If there's a desire to keep it between sessions, the |topChild| would need to be stored. If that's needed, it should be a separate bug because it would impact other use cases (eg, on session restore using ctrl+` to switch to another group currently switches to the first tab in the group).

Comment on attachment 524784[details][diff][review]
Falls back on UI.getActiveTab when available. Adds test case.
Thanks for your patches, Adam! Let me first give you some feedback on the test :)
Some nits:
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
Please use our new public domain header (see bug 629514).
>+ showTabView(function () {
>+ let cw = TabView.getContentWindow();
>+ let groupItem = cw.GroupItems.getActiveGroupItem();
>+ ok(!groupItem._isStacked, 'groupItem is not stacked');
groupItem.isStacked() is much safer to use because its behavior might change someday.
>+ groupItem.setSize(150, 150);
>+ groupItem.setUserSize();
>+ ok(groupItem._isStacked, 'groupItem is now stacked');
Same as above.
The test approach looks really good but there's one problem: if you run all tests it'll fail here:
>+ ok(!groupItem._isStacked, 'groupItem is not stacked')
That's because other tests that run before modify the group size. We might set the group size at the beginning of the test but that might affect following tests. The best way is to use newWindowWithTabView() (from head.js) here. That opens a new window that you can close at the end of the test. So no test env pollution and no other tests affected :)

Comment on attachment 524784[details][diff][review]
Falls back on UI.getActiveTab when available. Adds test case.
>+ if (!this.topChild) {
>+ var activeTab = UI.getActiveTab();
>+ if (activeTab) {
>+ this.topChild = activeTab;
>+ }
>+ }
>+
While this is working and a solution for this problem we should seize the chance to do some cleanup and take another approach :) So we figured out the rule to determine the item at the top of the stack: it's the group's active tab or (if there's none) it's the first child (if there's one). So in code this looks like:
> getTopChild: function () {
> if (!this.isStacked() || !this.getChildren().length)
> return null;
>
> return this.getActiveTab() || this.getChild(0);
> }
We should remove all occurences of "GroupItem.topChild" and replace them with "GroupItem.getTopChild()". When we did this there is no need for "GroupItem.setTopChild()" anymore. Let's remove this and the code that calls it. Everything should be ok because:
UI.setActiveTab() -> tab.makeActive() -> groupItem.setActiveTab()
That is the call chain that makes sure that every time a tab is activated it becomes the active tab in its group. So the top of the stack is always up-to-date. "setTopChild()" is called by "TabItem.zoomIn()/TabItem.zoomOut()" only to make sure the tab we're zooming in/out is visible. Both of these call "UI.setActiveTab()".
While we're at it we could also simplify "GroupItem.isTopOfStack()" to be like:
> isTopOfStack: function GroupItem_isTopOfStack(item) {
> return item == this.getTopChild();
> }
@Ian: would you concur with this cleanup/solution? The benefit we get from this is that we can remove a "private" property and let it automatically be determined without having to make sure it's always in the right state :)

Comment on attachment 527055[details][diff][review]
Fixes issues raised in previous feedback.
Review of attachment 527055[details][diff][review]:
Otherwise looking good!
::: browser/base/content/tabview/groupitems.js
@@ +347,5 @@
// Returns true if the item is showing on top of this group's stack,
// determined by whether the tab is this group's topChild, or
// if it doesn't have one, its first child.
isTopOfStack: function GroupItem_isTopOfStack(item) {
+ return item == this.getTopChild();
This function should still check this.isStacked(); See comment below on getTopChild.
@@ +1858,5 @@
+ // Gets the <Item> that should be displayed on top when in stack mode.
+ getTopChild: function GroupItem_getTopChild() {
+ if (!this.isStacked() || !this.getChildren().length) {
+ return null;
+ }
This function shouldn't check this.isStacked; it should instead return what would be the top child regardless of whether it's stacked or not.
Note that in _stackArrange(), getTopChild() is called before _isStacked is set to true. Being stacked shouldn't be a requirement for calling getTopChild.