moveTabToGroupItem fails if move the only tab in a group to the same group

VERIFIED FIXED in Future

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: ithinc, Assigned: ttaubert)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101022 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101022 Firefox/4.0b8pre

When implementing a "Move Tabs to New Group" feature, I need to move the context tab to a new group first, then move all needed tabs to that group. If the context tab happens to be the first tab in the tabs, it leads to a failure.

Reproducible: Always

Steps to Reproduce:
Supposing you have not opened an app tab:
TabView.moveTabTo(gBrowser.mTabs[0], null);
TabView.moveTabTo(gBrowser.mTabs[0], gBrowser.mTabs[0].tabItem.parent.id);
(Reporter)

Updated

8 years ago
Component: General → TabCandy
QA Contact: general → tabcandy
(Reporter)

Updated

8 years ago
Blocks: 597043
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk

Comment 1

8 years ago
This would be great to have for beta 8, though I'm not sure we'll get to it. We currently plan to do our most intensive API work post-ff4.0.

I'll leave this bug blocking beta8 for the time being to keep it on our minds.
Target Milestone: --- → Future
No longer blocks: 597043
(Assignee)

Comment 2

7 years ago
Created attachment 500626 [details] [diff] [review]
patch v1

This should be as simple as comparing the tab's current group id with the target group's id. If they're equal do nothing.
Assignee: nobody → tim.taubert
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #500626 - Flags: review?(Olli.Pettay)

Comment 3

7 years ago
Comment on attachment 500626 [details] [diff] [review]
patch v1

I'm not familiar with this code.
Attachment #500626 - Flags: review?(Olli.Pettay) → review?(ian)
(Reporter)

Comment 4

7 years ago
Yes, it's simple enough. The patch should work.
Comment on attachment 500626 [details] [diff] [review]
patch v1

>+  TabView.moveTabTo(tab, null);

This line shouldn't be necessary; entering the test we should have one tab and one group, and the tab should be in the group. Is this not what you're finding?

Otherwise looks good. R+ with that addressed.
Attachment #500626 - Flags: review?(ian) → review+
(Assignee)

Comment 6

7 years ago
Created attachment 501277 [details] [diff] [review]
patch v2 (test corrected)
Attachment #500626 - Attachment is obsolete: true
(Assignee)

Comment 7

7 years ago
Created attachment 501368 [details] [diff] [review]
patch v3 (test now correctly cleans up its environment)
Attachment #501277 - Attachment is obsolete: true
Attachment #501368 - Flags: approval2.0?
(Assignee)

Comment 8

7 years ago
Pushed to try. Passed.
Comment on attachment 501368 [details] [diff] [review]
patch v3 (test now correctly cleans up its environment)

a=beltzner
Attachment #501368 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 10

7 years ago
Created attachment 503248 [details] [diff] [review]
patch for checkin

Passed try.
Attachment #501368 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/041947ac6cbb
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified with recent minefield nightly build
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.