685692 - "Move to Group" should always insert the moved tab into the same place

Status: VERIFIED FIXED

Description

[Tim Taubert [:ttaubert]]

When using the tab context menu item "Move to Group" we should insert the moved tab always into the same place. I'd say we append it but the UX team might oppose that. Asking the UX team where to insert those tabs.

Comment 1

[Raymond Lee [:raymondlee]]

At the moment, it moves tab to the first position of target group.  WIll make it add to the group as last element.

Comment 2

[Raymond Lee [:raymondlee]]

Created attachment 561153 [details] [diff] [review]
v1

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 561153 [details] [diff] [review]
v1

Review of attachment 561153 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/groupitems.js
@@ +2604,5 @@
>      // add tab item to a groupItem
>      if (groupItemId) {
>        groupItem = GroupItems.groupItem(groupItemId);
>        groupItem.add(tab._tabViewTabItem);
> +      groupItem.reorderTabsBasedOnTabItemOrder()

looks fine, r=me. and btw, i totally dig how readable panorama tests are.

one question though: should these two methods result in a different order? what's the use-case for a reordering that puts an appended tab at the front of a group?

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to Dietrich Ayala (:dietrich) from comment #3)
> one question though: should these two methods result in a different order?
> -      UI.setReorderTabItemsOnShow(groupItem);
This method is for reordering tab items in Panorama based on the tab order on the tab bar.  The issue of this bug is that the tabs on tab bar is not reordered before entering Panorama, therefore, the moved tab appears in the new group in Panorama in the order of tabs on tabbar.

> +      groupItem.reorderTabsBasedOnTabItemOrder()
To solve this, we use the above method to keep the order of tabs on tab bar and tab items in Panorama in sync.  When a tab is moved to another group  using "Move to Group" menu item, the moved tab item in Panorama would be appended to the new group list. we call the above method so when user enters next time, the moved tab would appear at the end of list.


> what's the use-case for a reordering that puts an appended tab at the front
> of a group?
For example, 

In Panorama
Tab 1, Tab 2 in Group A
Tab 3, Tab 4 in Group B

Group A is being displayed on tab bar.  Here is the tab order on tab bar
Tab 1, Tab 2, Tab 3 (hidden), Tab 4 (hidden)

Move Tab 1 to Group B and then enter Panorama. The items in Group B is reordered based on tab order on tab bar.  Tab 1 appears as the first item in Group B 
Tab 1 in Group A
Tab 1, Tab 3, Tab 4 in Group B

The patch solves this problem.

Comment 5

[Raymond Lee [:raymondlee]]

Created attachment 561372 [details] [diff] [review]
Patch for checkin

Comment 6

[Raymond Lee [:raymondlee]]

Comment on attachment 561372 [details] [diff] [review]
Patch for checkin

Passed Try!
https://tbpl.mozilla.org/?tree=Try&rev=73691b051cf8

Comment 7

[Tim Taubert [:ttaubert]]

https://hg.mozilla.org/integration/fx-team/rev/a9edf67c8954

Comment 8

[Tim Taubert [:ttaubert]]

https://hg.mozilla.org/mozilla-central/rev/a9edf67c8954

Comment 9

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:9.0a2) Gecko/20111026 Firefox/9.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111026 Firefox/10.0a1

When using move to group from the tab context menu, the moved tab is now always inserted in the last position. (as specified in comment 2). Verified on Windows XP, 7, Ubuntu 11.10, Mac OS 10.6 on Firefox 9 and 10 (Aurora and Nightly)