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

VERIFIED FIXED in Future

Status

defect
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: tabutils+bugzilla, Assigned: ttaubert)

Tracking

Trunk
Future

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

9 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

9 years ago
Component: General → TabCandy
QA Contact: general → tabcandy
Reporter

Updated

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

Comment 1

9 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
Posted patch patch v1 (obsolete) — Splinter Review
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 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

9 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+
Posted patch patch v2 (test corrected) (obsolete) — Splinter Review
Attachment #500626 - Attachment is obsolete: true
Attachment #501277 - Attachment is obsolete: true
Attachment #501368 - Flags: approval2.0?
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+
Passed try.
Attachment #501368 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/041947ac6cbb
Status: ASSIGNED → RESOLVED
Closed: 9 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.