Last Comment Bug 649319 - tab focus when resizing or moving a group in panorama
: tab focus when resizing or moving a group in panorama
Status: VERIFIED FIXED
: ux-consistency
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 7
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 632294
Blocks: 660175
  Show dependency treegraph
 
Reported: 2011-04-12 06:41 PDT by eyal gruss (eyaler)
Modified: 2016-04-12 14:00 PDT (History)
8 users (show)
ehsan: in‑testsuite+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (8.74 KB, patch)
2011-04-19 05:55 PDT, Tim Taubert [:ttaubert]
ian: review-
raymond: feedback+
Details | Diff | Splinter Review
patch v2 (7.92 KB, patch)
2011-04-21 17:45 PDT, Tim Taubert [:ttaubert]
ian: review+
Details | Diff | Splinter Review
patch for checkin (do not push before bug 632294) (6.51 KB, patch)
2011-04-22 09:35 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch for checkin (6.41 KB, patch)
2011-05-04 16:56 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review

Description eyal gruss (eyaler) 2011-04-12 06:41:54 PDT
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] 2011-04-19 05:55:35 PDT
Created attachment 526965 [details] [diff] [review]
patch v1
Comment 2 Raymond Lee [:raymondlee] 2011-04-19 06:58:10 PDT
Comment on attachment 526965 [details] [diff] [review]
patch v1

Looks good
Comment 4 Ian Gilman [:iangilman] 2011-04-21 10:14:22 PDT
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] 2011-04-21 17:45:25 PDT
Created attachment 527702 [details] [diff] [review]
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 7 Ian Gilman [:iangilman] 2011-04-22 09:29:33 PDT
Comment on attachment 527702 [details] [diff] [review]
patch v2

Thanks!
Comment 8 Tim Taubert [:ttaubert] 2011-04-22 09:35:43 PDT
Created attachment 527796 [details] [diff] [review]
patch for checkin (do not push before bug 632294)
Comment 9 Tim Taubert [:ttaubert] 2011-05-04 16:56:56 PDT
Created attachment 530197 [details] [diff] [review]
patch for checkin
Comment 10 Boris Zbarsky [:bz] 2011-05-05 13:34:22 PDT
http://hg.mozilla.org/mozilla-central/rev/6b26a933cad7
Comment 11 Matt Brubeck (:mbrubeck) 2011-05-05 14:58:34 PDT
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] 2011-05-27 02:21:45 PDT
bugspam
Comment 13 Tim Taubert [:ttaubert] 2011-05-27 02:26:46 PDT
bugspam
Comment 15 Boris Zbarsky [:bz] 2011-06-09 11:57:16 PDT
Backed out due to mochitest-other orange.
Comment 18 George Carstoiu 2011-06-16 05:11:13 PDT
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.

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