Closed
Bug 649319
Opened 14 years ago
Closed 13 years ago
tab focus when resizing or moving a group in panorama
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 7
People
(Reporter: eyalgruss, Assigned: ttaubert)
References
Details
(Keywords: ux-consistency)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #526965 -
Flags: feedback?(raymond)
Comment 2•14 years ago
|
||
Comment on attachment 526965 [details] [diff] [review]
patch v1
Looks good
Attachment #526965 -
Flags: feedback?(raymond) → feedback+
Assignee | ||
Updated•14 years ago
|
Attachment #526965 -
Flags: review?(ian)
Assignee | ||
Comment 3•14 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 4•14 years ago
|
||
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•14 years ago
|
||
(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•14 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 7•14 years ago
|
||
Comment on attachment 527702 [details] [diff] [review]
patch v2
Thanks!
Attachment #527702 -
Flags: review?(ian) → review+
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #527702 -
Attachment is obsolete: true
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #527796 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
Comment 10•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → Firefox 6
Comment 11•13 years ago
|
||
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 14•13 years ago
|
||
Whiteboard: [inbound]
Assignee | ||
Comment 16•13 years ago
|
||
Whiteboard: [inbound]
Comment 17•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: Firefox 6 → Firefox 7
Comment 18•13 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
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•