642793 - The focused Tab is not set first in the stack

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P5)

Description

[Simona Badau, Desktop QA]

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.

Comment 1

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0

Issue is present ever since the first build that included panorama.

Comment 2

[Kevin Hanes]

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.

Comment 3

[Adam]

Attachment: Falls back on UI.getActiveTab when available. Adds test case.

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 4

[Ian Gilman [:iangilman]]

Comment on attachment 524784 [details] [diff] [review]
Falls back on UI.getActiveTab when available.  Adds test case.

Tim, can you take a look at this first?

Comment 5

[Ian Gilman [:iangilman]]

Thank you for all the patches, by the way, Adam! :)

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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 7

[Tim Taubert [:ttaubert] (inactive)]

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 8

[Tim Taubert [:ttaubert] (inactive)]

@Raymond: What would you say? Please give your two cents, too, if you want :)

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to comment #8)
> @Raymond: What would you say? Please give your two cents, too, if you want :)

I concur with the the cleanup/solution you suggested :)

Comment 10

[Ian Gilman [:iangilman]]

I concur with the suggested clean-up as well; thank you for bringing it up!

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 524784 [details] [diff] [review]
Falls back on UI.getActiveTab when available.  Adds test case.

Please request feedback again on your next iteration.

Comment 12

[Adam]

Attachment: Cleanup of how stack top child is handled, updates other tests, adds test for the bug itself.

Thanks for the feedback, Tim.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 526773 [details] [diff] [review]
Cleanup of how stack top child is handled, updates other tests, adds test for the bug itself.

Nice patch, thank you! Some minor issues:

* GroupItem.setActiveTab():

We need to make sure that the tab on the top of the stack is up-to-date when someone calls GroupItem.setActiveTab(). So if the group is stacked and the given tab argument != this._activeTab we need to call this.arrange({immediately: true}); to ensure the new active tab is rendered at the top of the stack.

* GroupItem.setZ():

We need to replace self.topChild with the new .getTopChild() method.

>@@ -1534,17 +1532,17 @@ GroupItem.prototype = Utils.extend(new I
>   expand: function GroupItem_expand() {
>     var self = this;
>     // ___ we're stacked, and command is held down so expand
>     GroupItems.setActiveGroupItem(self);
>-    let activeTab = this.topChild || this.getChildren()[0];
>+    let activeTab = this.getTopChild();
>     UI.setActiveTab(activeTab);

Nit: We don't need the extra variable (activeTab) here.

>+++ b/browser/base/content/test/tabview/browser_tabview_bug642793.js
>+function testTopOfStack(win) {
>+  let cw = win.TabView.getContentWindow();
>+  groupItem = cw.GroupItems.getActiveGroupItem();
>+  ok(!groupItem.isStacked(), 'groupItem is not stacked');
>+  groupItem.setSize(150, 150);
>+  groupItem.setUserSize();
>+  ok(groupItem.isStacked(), 'groupItem is now stacked');
>+  ok(groupItem.isTopOfStack(groupItem.getChild(2)),
>+    'the third tab is on top of stack');
>+  finishTest(win);
>+}
>+
>+function finishTest(win) {
>+  win.close();
>+  finish();
>+}

Let's do the following:

>+function testTopOfStack(win) {
>+  registerCleanupFunction(function () win.close());
>+  let cw = win.TabView.getContentWindow();

This makes sure that even if the test times out the window gets closed. We also don't need "finishTest()" anymore.

F+ with those changes! Please ask Ian for review once your patch is updated.

Comment 14

[Adam]

Attachment: Fixes issues raised in previous feedback.

Comment 15

[Ian Gilman [:iangilman]]

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.

Comment 16

[Adam]

Attachment: Correct isStacked() checks for getTopChild and isTopOfStack

Thanks for the feedback.

Comment 17

[Ian Gilman [:iangilman]]

Comment on attachment 528138 [details] [diff] [review]
Correct isStacked() checks for getTopChild and isTopOfStack

Review of attachment 528138 [details] [diff] [review]:

Looks great, thanks!

Comment 18

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/projects/cedar/rev/c8c3e140ebe3

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Adam, thank you for the patch!

Comment 20

[George Carstoiu]

Verified on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:6.0a1) Gecko/20110504 Firefox/6.0a1