tab focus when resizing or moving a group in panorama

VERIFIED FIXED in Firefox 7

Status

defect
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: eyaler, Assigned: ttaubert)

Tracking

({ux-consistency})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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
Depends on: 526219
Depends on: 632294
No longer depends on: 526219
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #526965 - Flags: feedback?(raymond)
Comment on attachment 526965 [details] [diff] [review]
patch v1

Looks good
Attachment #526965 - Flags: feedback?(raymond) → feedback+
Attachment #526965 - Flags: review?(ian)
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.
Attachment #526965 - Flags: review?(ian) → review-
Posted patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #526965 - Attachment is obsolete: true
Attachment #527702 - Flags: review?(ian)
Comment on attachment 527702 [details] [diff] [review]
patch v2

Thanks!
Attachment #527702 - Flags: review?(ian) → review+
Attachment #527702 - Attachment is obsolete: true
No longer blocks: 603789
Blocks: 653099
Attachment #527796 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
http://hg.mozilla.org/mozilla-central/rev/6b26a933cad7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → Firefox 6
Backed out for possibly causing a frequent mochitest-other tabview test failure on Windows debug:
http://hg.mozilla.org/mozilla-central/rev/88fdbd974f82
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
Backed out due to mochitest-other orange.
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/cb2a5954a5b4
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: Firefox 6 → Firefox 7
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.