649319 - tab focus when resizing or moving a group in panorama

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[eyal gruss]

current behavior:
1. when a group is resized in panorama, focus does not change
2. when a group is moved in panorama, focus moves to the first tab in that group

expected behavior:
1. focus should be behave the same whether resizing a group or moving it
2. if the focus changing behavior is implemented for both actions, focus should change to the last used tab in the group (instead of the first tab)

Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Raymond Lee [:raymondlee]]

Comment on attachment 526965 [details] [diff] [review]
patch v1

Looks good

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 526965 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=099e605403c5

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 526965 [details] [diff] [review]
patch v1

I see you've based this patch on the changes in bug 632294; excellent. One thing: it's now updated so you no longer have to pass setActiveTabInGroup into UI.setActive.

>   getActiveTab: function GroupItem_getActiveTab() {
>-    return this._activeTab;
>+    return this._activeTab || this.getChild(0);
>   },

Why this change? There's already logic elsewhere to make sure that there's always an active tab as long as there's a tab. Is that logic not working?

If we absolutely need this change, then you should also change every place where _activeTab is accessed directly to now go through getActiveTab; otherwise you'll have inconsistent results. Also we'd probably want to strip out that logic elsewhere. 

I suspect this change is not necessary and the logic for ensuring there's always an active tab in the group maybe just needs updating.

>-        // if it is visually active, set it as the active tab.
>-        if (iQ(item.container).hasClass("focus"))
>-          UI.setActive(item);
>+        if (item == UI.getActiveTab())
>+          this._activeTab = item;

You should still use this.setActiveTab rather than setting _activeTab directly; as long as we have the accessor, better to route everything through it.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #4)
> I see you've based this patch on the changes in bug 632294; excellent. One
> thing: it's now updated so you no longer have to pass setActiveTabInGroup into
> UI.setActive.

Removed.

> >   getActiveTab: function GroupItem_getActiveTab() {
> >-    return this._activeTab;
> >+    return this._activeTab || this.getChild(0);
> >   },
> 
> Why this change? There's already logic elsewhere to make sure that there's
> always an active tab as long as there's a tab. Is that logic not working?

Removed. But changed the following:

> >-        // if it is visually active, set it as the active tab.
> >-        if (iQ(item.container).hasClass("focus"))
> >-          UI.setActive(item);
> >+        if (item == UI.getActiveTab())
> >+          this._activeTab = item;
> 
> You should still use this.setActiveTab rather than setting _activeTab directly;
> as long as we have the accessor, better to route everything through it.

Fixed. And changed to:

> >+        if (item == UI.getActiveTab() || !this._activeTab)
> >+          this._activeTab = item;

So the first tabItem added to an empty groupItem will automatically be active.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 527702 [details] [diff] [review]
patch v2

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=5cb519a7d5e2

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 527702 [details] [diff] [review]
patch v2

Thanks!

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin (do not push before bug 632294)

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 10

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/6b26a933cad7

Comment 11

[Matt Brubeck (:mbrubeck)]

Backed out for possibly causing a frequent mochitest-other tabview test failure on Windows debug:
http://hg.mozilla.org/mozilla-central/rev/88fdbd974f82

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d580181e5434

Comment 15

[Boris Zbarsky [:bzbarsky]]

Backed out due to mochitest-other orange.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cb2a5954a5b4

Comment 17

[(no longer active)]

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

Comment 18

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110615 Firefox/7.0a1

Verified issue on Ubuntu 11.04, Mac OS X 10.6, Win7 x86 and WinXp using the following steps to reproduce:
 1. Created two groups in Panorama populated each with two tabs.
 2. Played with resize and move between groups always checking to see whether the focus returns to the last used tab in the group.

Changing status to Verified.