586198 - Incorrect placement of tab after Undo Close Tab in Tab Candy

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Aza Raskin [:aza]]

In a group with multiple tabs (say three):

(1) Close a tab while inside zoomed into the group
(2) Using the context-menu or the keyboard shortcut to undo the close tab
(3) The tab will appear in a group by itself (i.e., the other tabs will go away) and the tab in the Tab Candy interface will appear directly underneath the former group (so hidden until the group is moved).

Comment 1

[Christophe Calvez]

Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100812 Minefield/4.0b4pre ID:20100813015949

I think this is the same bug, except that the behaviour I see on step (3) is the following :
- the restored tab is the only tab
- when going to TabView, it works as it should, the restored tab is zoomed-out to its group.

Comment 2

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 4

[Ed Lee :Mardak]

Undo close tab is a pretty major feature. Technically not a regession as undo close tab does still reopen the closed tab. I suppose not quite my call if it should be fixed in beta 4.

What's the expected behavior though? Restore the tab into the group it was in before? Restore into old group and switch to that group? Restore it into the current group?

Comment 5

[IU]

(In reply to comment #4)
>Restore the tab into the group it was in before?

To me, this makes the most sense and also generally covers option 3 ("Restore it into the current group"), when undo closed tab is bounded by group, instead of operating globally.

Here are my thoughts.  Assume I have Group A, Group B, and Group C:


SCENARIO I:  While browsing Group A, I close a tab.

If:
a) while still browsing Group A, I undo closed tab, the tab should be restore to my current group, which is Group A
b) I switch to Group B or C and choose to undo closed tab.  The action should be ignored, because I have no closed tabs in Group B or C.
c) having switched to Group B or C, I switch back to Group A and choose to undo closed tab.  The action should succeed and the tab should be restored, because it lived in Group A.


SCENARIO II: I am in "Group Your Tabs" view and close one or more tabs, in one or more groups, therein.

Note 1: Since tabs can be closed from "Group Your Tabs" view, it would make sense to have undo closed tab buttons included in each tab group window.

If I browse any of the groups, from which I closed tabs while in "Group Your Tabs" view, undo closed tab should still be bound to only tabs closed within the respective group -- should not act beyond the current group's scope to undo tabs in groups not in view.

If "undo closed tab" and "undo closed group (or window)" buttons are added in the global scope of the "Group Your Tabs" view, then I believe that is the only time undo closed tab/window should act globally.

Note 2: Session restore should continue to work per normal and not be bound by these conditions.

Of course, these are just my thoughts, which carry no weight, and I should don my flame retardant suit immediately upon hitting the "Save Changes" button. :-)

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Yes, we should restore to the previous group - this ends up looking a lot like dataloss to users, even though it isn't.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This is a regression, for sure.  To users who don't use tab candy (and I suspect there will be many) it's the only part of tab candy that can affect them, and really really feels like dataloss.  (I couldn't figure out how to get those other tabs back when it happened to me.)

If we can jam a fix in, even at the expense of some tab candy functionality, I think we should.

Comment 8

[Ed Lee :Mardak]

Attachment: wip

Kinda fixes it but not really. With the fix in bug 586693, the undo-close-tab tab will appear in the tab strip but disappear on switch because it thinks it has no parent.

With this patch, it'll remember what parent it belongs to but it still shows up in the current tabstrip and disappears when switching away.

The patch works by saving the parent before the tabitem sends "close" to its subscribers -- one of them being the groupitem which will then tabitem.setParent(null) which then calls the Item.setParent -> Item.save parent implementations and saves no group id.

Raymond, Ian, any suggestions for better fixes?

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

After thinking about this more, I believe it needs to block beta4. (Sorry; thought I did this earlier, looks like there was a bugzilla timeout)

Comment 10

[Ed Lee :Mardak]

Attachment: v1

Temporary fix to just keep visible tabs visible when undo close tab instead of hiding the rest of the tabs.

I'll file a followup bug to implement the desired ux of undo close tab should be group-specific and only undo close tabs of tabs in that group.

Comment 11

[Ed Lee :Mardak]

Attachment: v1

Apparently hg treats "null" as a special tag.. renamed my mq patch to something else to export :p

Comment 12

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/f61b0b275541
Explicitly wipe out tab storage data on close so that undo close tab acts like a brand new tab.

Comment 15

[tabmix.onemen]

tab position in current group after restore is wrong.

if the tab was originally closed in another group then it should reopen as the last tab in the current group

Comment 17

[Ed Lee :Mardak]

(In reply to comment #15)
> tab position in current group after restore is wrong.
Yes. That was known for this temporary fix. The desired full fix is bug       587874 where only tabs of the current group will get restored, so it will end up in the expected position.

Comment 18

[tabmix.onemen]

if my browser.sessionstore.max_tabs_undo is 10.
and i have group A,B and C each with 8 tabs , how many closed tab will be saved in the closed tab list for each group.

lest say i closed 3 tabs from group A, then open group D from some news site open 15 tabs then close group D.

questions:
How many closed tabs are in the closed tabs list and in what order?
if i switch to group C and i like to restore one of the closed tabs can i do it?
if only tabs from current group can restored, how can i restore tabs from closed groups?

Comment 19

[Cameron Dawson [:camd]]

Added to Litmus:
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=12844

Comment 20

[Teodosia Pop]

I am able to reproduce this issue using Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110106 Firefox/4.0b9pre.

Comment 21

[Michael Yoshitaka Erlewine [:mitcho]]

I also just reproduced this on trunk, on Mac.

Comment 24

[Ian Gilman [:iangilman]]

It's likely this is due to my recent sessionstore-related patches:

2011-01-05 12:54 -0800	Ian Gilman - Bug 617511 - bug fix for test breakage by 594644 r=dietrich, a=johnath

2010-11-03 11:14 -0700	Ian Gilman - Bug 605935 - Use private-browsing-transition-complete instead of sessionstore-browser-state-restored r=ehsan a=dietrich

2010-11-22 16:28 -0800	Ian Gilman - Bug 594644 part 2 r=dietrich a=blocking

2010-10-08 15:59 -0700	Ian Gilman - Bug 594644 - Return from Private Browsing Mode, Panorama forgets tab groups, until Panorama is manually re-launched r=dietrich a=blocking

2011-01-05 12:54 -0800	Ian Gilman - Bug 598795 - Clean up reconnect code once we have instant access to sessionstore r=dietrich, a=blocking

Comment 25

[Alice0775 White]

(In reply to comment #24)

It happens after using Panonama once.

In local build:
build from f453924d5fe1 : fails
build from 8bb78383bdb6 : works
build from ccfc9d214703 : works
build from b06a94065ef0 : works
build from 2d849e2d302e : works
build from bddc89a2200c : works

And related to Bug 623792.

Comment 27

[Nickolay_Ponomarev]

mitcho, ian, tim, since this is a new regression, it should really be in a separate bug, otherwise all the flags and dependencies are getting messed up. (For example, it might get lost from the blocking radar because it's set to block an old beta).

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

regression of this bug is now handled in bug 624265

Comment 30

[eyal gruss]

i am seeing this on  Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9) Gecko/20100101 Firefox/4.0b9

Comment 31

[Josh Matthews [:jdm]]

Nope.  This bug is resolved, and bug 624265 handles the regression with a fix that didn't make it into 4.0b9.

Comment 32

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11) Gecko/20100101 Firefox/4.0b11

Comment 33

[Steve Scott (pxbugz)]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre