User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100814 Minefield/4.0b4pre Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100814 Minefield/4.0b4pre If users accidentally closes a Tab Group in Tab View of Tab Candy, they cannot undo that action. All they can do now is to undo close all the tabs(in the closed group) back and regroup them back into a group, in order to get that group back. This is not user-friendly and caused user a bad experience in Tab Candy. Implementing Undo Close Tab Group should solve this problem. Reproducible: Always

Should we take the idea further and put a recently closed Tab Groups menu item in History sub-menu of the Firefox Menu?

Please see the full specification for this feature here: https://wiki.mozilla.org/Firefox/Projects/TabCandy/Design/UndoCloseGroup

Aza, If the user closes 2 group quickly one after the other, will 2 reopen group come out, or just 1(for the last closed group) will appear?

(In reply to comment #5) > Aza, If the user closes 2 group quickly one after the other, will 2 reopen > group come out, or just 1(for the last closed group) will appear? I propose there be one for each.

In terms of implementation, I see two possibilities: 1. We modify sessionstore to keep track of closed groups like it does closed tabs. 2. We hide the associated TabItems when the user closes a group, but don't actually close the tabs until either the "undo close group" button fades away or the user exits Panorama mode. Given that we're not doing "all windows" (bug 578512) for this version, and therefore won't be able to do a clean sessionstore integration, I propose we go with option 2, with the intention of moving to option 1 in a future version. Going with option 2 will also allow us to no longer depend on bug 587874. It would likely mean deferring the history menu addition (bug 587676), but I'm going to propose punting that as well.

Spec: https://wiki.mozilla.org/Firefox/Projects/TabCandy/Design/UndoCloseGroup

This implements the UI-side of the undo feature. It does not attempt to actually do the undoing.

Same as before, but with the right mime-type

Note: one problem with this patch is I haven't separated out the CSS to the appropriate files.

This fixes the CSS — it is now divided into structural components. Will need theming work for Windows and Linux.

Comment on attachment 472562 [details] Undo patch v1.1 feedback+. request review when you consider the patch checkin-ready.

Ian: please give this patch a try. Tests are missing, will add them soon.

Comment on attachment 472683 [details] [diff] [review] v2 Looks great! Once it's got a unit test, it'll be ready to go.

However, this patch doesn't include updating the browser_tabview_group.js since it's being updated in bug 594213

Forget to add the test file.

Comment on attachment 472992 [details] [diff] [review] v3 with tests + let padding = 200; + pageBounds.inset(padding, padding); + + let box = new contentWindow.Rect(padding); + box.width = 300; + box.height = 300; Creating a Rect with a single number is a bit odd. Also you're not using pageBounds after creating it. Did you mean to pass it in to the Rect constructor? f+ with that. Please make the change and then r? dietrich.

Pushed to try server and waiting for the result. rev 326e94d59cd7

Made some changes in the dropdrop test. Pushed another commit to the try server. rev 324f7fb6fefd

Results in the try server look good. Please review this patch. Thanks!

Comment on attachment 473179 [details] [diff] [review] v4 > groupItems.forEach(function(groupItem) { >- if (groupItem.getTitle().length > 0 && >+ if (groupItem.getTitle().length > 0 && !groupItem.canUndo && > (!activeGroup || activeGroup.id != groupItem.id)) { a lot of conditions here. add a comment explaining what's happening? >@@ -1558,17 +1692,16 @@ window.GroupItems = { > // Function: setActiveGroupItem > // Sets the active groupItem, thereby showing only the relevent tabs, and > // setting the groupItem which will receive new tabs. > // > // Paramaters: nit: fix this while you're here? >+ // Function: removeCanUndoGroups >+ // Removes all can undo groups' data and its children browser tabs >+ removeCanUndoGroups: function() { nit: extra whitespace >+ this.groupItems.forEach(function(groupItem) { >+ if (groupItem.canUndo) { >+ let toClose = groupItem._children.concat(); >+ toClose.forEach(function(child) { >+ child.close(); >+ }); seems like this should be encapsulated inside the groupItem somehow? (doesn't need to be fixed in this bug) >diff --git a/browser/base/content/test/tabview/browser_tabview_undo_group.js b/browser/base/content/test/tabview/browser_tabview_undo_group.js for the test, i'd like to see it cover the full-circle of actions: close -> undo -> close -> undo. a lot of bugs have been generated over the years by not fully testing the entire cycle. patch looks good for the most part. with these fixed and the naming changes we talked about on irc i could r+ this.

Fixed the test for bug 591706

Comment on attachment 473932 [details] [diff] [review] v6 Just added a fix for the browser_tabview_bug591706.js in this patch.

Please replace -moz-border-radius with border-radius.

Replaced -moz-border-radius with border-radius.

posthumous blocking+ since people might not immediately understand that closing a group means closing all of that group's tabs.

Want a test case in Litmus for this. Flagging it to pick up during Bug Week (9-13 to 9-17) Verified with nightly build of 20100913

Added test 12843 in litmus.