tab focus when resizing or moving a group in panorama

VERIFIED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: eyal gruss (eyaler), Assigned: ttaubert)

Tracking

({ux-consistency})

Trunk
Firefox 7
ux-consistency
Dependency tree / graph
Bug Flags:
in-testsuite +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Depends on: 526219
(Assignee)

Updated

6 years ago
Depends on: 632294
No longer depends on: 526219
(Assignee)

Updated

6 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 526965 [details] [diff] [review]
patch v1
Attachment #526965 - Flags: feedback?(raymond)
Comment on attachment 526965 [details] [diff] [review]
patch v1

Looks good
Attachment #526965 - Flags: feedback?(raymond) → feedback+
(Assignee)

Updated

6 years ago
Attachment #526965 - Flags: review?(ian)
(Assignee)

Comment 3

6 years ago
Comment on attachment 526965 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=099e605403c5
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-
(Assignee)

Comment 5

6 years ago
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.
Attachment #526965 - Attachment is obsolete: true
Attachment #527702 - Flags: review?(ian)
(Assignee)

Comment 6

6 years ago
Comment on attachment 527702 [details] [diff] [review]
patch v2

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=5cb519a7d5e2
Comment on attachment 527702 [details] [diff] [review]
patch v2

Thanks!
Attachment #527702 - Flags: review?(ian) → review+
(Assignee)

Comment 8

6 years ago
Created attachment 527796 [details] [diff] [review]
patch for checkin (do not push before bug 632294)
Attachment #527702 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
No longer blocks: 603789
(Assignee)

Updated

6 years ago
Blocks: 653099
(Assignee)

Comment 9

6 years ago
Created attachment 530197 [details] [diff] [review]
patch for checkin
Attachment #527796 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
http://hg.mozilla.org/mozilla-central/rev/6b26a933cad7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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 → ---
(Assignee)

Comment 12

6 years ago
bugspam
No longer blocks: 653099
(Assignee)

Comment 13

6 years ago
bugspam
Blocks: 660175
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/d580181e5434
Whiteboard: [inbound]
Backed out due to mochitest-other orange.
Whiteboard: [inbound]
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/cb2a5954a5b4
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/cb2a5954a5b4
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: Firefox 6 → Firefox 7

Comment 18

6 years ago
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.