640765 - App tab order not maintained in Tab Groups view

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[u279076]

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

Comment 1

[u279076]

Attachment: Screenshot: Order in main window

Comment 2

[u279076]

Attachment: Screenshot: Order in panorama

Comment 3

[aravindm]

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)

Comment 4

[Raymond Lee [:raymondlee]]

(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

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: WIP v1

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 7

[B.J. Herbison]

Dupe of bug 623878.

Comment 9

[u279076]

(In reply to comment #7)
> Dupe of bug 623878.

Thanks, I duped to this bug since this bug already has a patch up.

Comment 10

[Ian Gilman [:iangilman]]

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!

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v2

(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

Comment 12

[Ian Gilman [:iangilman]]

(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?

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: v3

(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.

Comment 14

[Ian Gilman [:iangilman]]

Comment on attachment 519334 [details] [diff] [review]
v3

I believe you attached the wrong patch.

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: v3

Upload the correct patch.

Comment 16

[Ian Gilman [:iangilman]]

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+

Comment 17

[Raymond Lee [:raymondlee]]

(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.

Comment 18

[Raymond Lee [:raymondlee]]

Attachment: v4

Un-bitrot the patch.  Sent it to try and waiting for the results.

Comment 19

[Raymond Lee [:raymondlee]]

Passed Try!

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 21

[Dão Gottwald [:dao]]

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.

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: v5

Comment 23

[Dão Gottwald [: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")

Comment 24

[Kevin Hanes]

bugspam

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Sent it to try and waiting for results.
http://tbpl.mozilla.org/?tree=MozillaTry&rev=492aca4e0502

Comment 26

[Raymond Lee [:raymondlee]]

Passed Try!

Comment 27

[Benjamin Stover (:stechz)]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/6281e1a3c11e

Comment 28

[George Carstoiu]

Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

bug spam