Closed Bug 640765 Opened 9 years ago Closed 9 years ago

App tab order not maintained in Tab Groups view

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 5

People

(Reporter: ashughes, Assigned: raymondlee)

References

Details

Attachments

(3 files, 8 obsolete files)

1. Open some tabs and make them App Tabs
2. Reorder them
3. Open Panorama

Result:
Order is the order they were opened, not the current arrangement

Expected:
Order in main window should be reflected in Panorama
Blocks: 603789
Mozilla/5.0 (Windows NT 6.1; rv:2.0b13pre) Gecko/20110310 Firefox/4.0b13pre
Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0

Works fine for me. After creating some app tabs and re-arranging them in a different order and opening the panaroma view, the tabs are reflected as per the main window (in the new order)
(In reply to comment #3)
> Mozilla/5.0 (Windows NT 6.1; rv:2.0b13pre) Gecko/20110310 Firefox/4.0b13pre
> Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0
> 
> Works fine for me. After creating some app tabs and re-arranging them in a
> different order and opening the panaroma view, the tabs are reflected as per
> the main window (in the new order)

The STR should be
1. Open some tabs and make them App Tabs
2. Enter and exit Panorama
2. Reorder them
3. Enter Panorama
Attached patch WIP v1 (obsolete) — Splinter Review
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → raymond
Attachment #518675 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #518741 - Flags: review?(ian)
Dupe of bug 623878.
Duplicate of this bug: 623878
(In reply to comment #7)
> Dupe of bug 623878.

Thanks, I duped to this bug since this bug already has a patch up.
Status: ASSIGNED → NEW
Comment on attachment 518741 [details] [diff] [review]
v1

I like the general direction, but can you move the existing icon rather than deleting the old one and making a new one? I think that would be cleaner.

The test looks great, but it would be more readable if you made a little helper function: 

function xulTabForAppTabIcon(index) {
	return contentWindow.iQ(contentWindow.iQ(".appTabIcon", groupItem.$appTabTray)[index]).data("xulTab");
}

>\ No newline at end of file

Fix that. :)

I figure this'll land after Fx4, but we might as well get started on that stuff!
Attachment #518741 - Flags: review?(ian) → review-
Attached patch v2 (obsolete) — Splinter Review
(In reply to comment #10)
> Comment on attachment 518741 [details] [diff] [review]
> v1
> 
> I like the general direction, but can you move the existing icon rather than
> deleting the old one and making a new one? I think that would be cleaner.
> 

I couldn't find a easy way to move the existing DOM icon to a new position in the parent node without deleting the old one and making a new one.  Any suggestion?

> The test looks great, but it would be more readable if you made a little helper
> function: 
> 
> function xulTabForAppTabIcon(index) {
>     return contentWindow.iQ(contentWindow.iQ(".appTabIcon",
> groupItem.$appTabTray)[index]).data("xulTab");
> }
> 
> >\ No newline at end of file
> 
> Fix that. :)
> 
Fixed
Attachment #518741 - Attachment is obsolete: true
Attachment #519088 - Flags: review?(ian)
(In reply to comment #11)
> I couldn't find a easy way to move the existing DOM icon to a new position in
> the parent node without deleting the old one and making a new one.  Any
> suggestion?

Can't you call iQ.remove on the appropriate element and then .insertBefore it back into the new location?
Attached patch v3 (obsolete) — Splinter Review
(In reply to comment #12)
> Can't you call iQ.remove on the appropriate element and then .insertBefore it
> back into the new location?

Fixed.
Attachment #519088 - Attachment is obsolete: true
Attachment #519088 - Flags: review?(ian)
Attachment #519334 - Flags: review?(ian)
Comment on attachment 519334 [details] [diff] [review]
v3

I believe you attached the wrong patch.
Attachment #519334 - Flags: review?(ian) → review-
Attached patch v3 (obsolete) — Splinter Review
Upload the correct patch.
Attachment #519334 - Attachment is obsolete: true
Attachment #519445 - Flags: review?(ian)
Status: NEW → ASSIGNED
Comment on attachment 519445 [details] [diff] [review]
v3

Looks beautiful! 

One thing just occurred to me, though: please make sure that the click event that's attached to the icon still functions properly after it's been removed and re-added. 

If that works, r+
Attachment #519445 - Flags: review?(ian) → review+
(In reply to comment #16)
> Comment on attachment 519445 [details] [diff] [review]
> v3
> 
> Looks beautiful! 
> 
> One thing just occurred to me, though: please make sure that the click event
> that's attached to the icon still functions properly after it's been removed
> and re-added. 
> 
> If that works, r+

Yes, the icon still functions properly after it's been removed and re-added.
Attached patch v4 (obsolete) — Splinter Review
Un-bitrot the patch.  Sent it to try and waiting for the results.
Attachment #519445 - Attachment is obsolete: true
Passed Try!
Keywords: checkin-needed
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #522282 - Attachment is obsolete: true
Comment on attachment 522347 [details] [diff] [review]
Patch for checkin

>+      let targetIndex =
>+        gBrowser.getBrowserIndexForDocument(
>+          $icon.data("xulTab").linkedBrowser.contentDocument);

You can get the index differently, there's no reason to mess with contentDocument here.
Attached patch v5 (obsolete) — Splinter Review
Attachment #522347 - Attachment is obsolete: true
Attachment #522355 - Flags: review?(dao)
Comment on attachment 522355 [details] [diff] [review]
v5

>+    elements.each(function(icon) {
>+      let $icon = iQ(icon);
>+      if ($icon.data("xulTab") != xulTab)
>+        return;
>+
>+      let targetIndex = $icon.data("xulTab")._tPos;

You can use xulTab instead of $icon.data("xulTab")
Attachment #522355 - Flags: review?(dao) → review+
bugspam
Sent it to try and waiting for results.
http://tbpl.mozilla.org/?tree=MozillaTry&rev=492aca4e0502
Attachment #522355 - Attachment is obsolete: true
Passed Try!
Keywords: checkin-needed
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/6281e1a3c11e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
bug spam
Whiteboard: [4rc]
Target Milestone: --- → Firefox 5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.