342700 - Make undo close tab more discoverable

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Adam Guthrie]

We added this great feature, but I don't think many people are going to discover it where it currently is (in the tab context menu). 

Possible solutions:
* put it in the page's context menu
* put it in the history menu

Comment 1

[Dietrich Ayala (:dietrich)]

Another option would be to put it in the edit menu, underneath "Undo" (hidden when there is no undo action possible), as that's where users would normally look for an undo action.

Comment 2

[Jesse Ruderman]

The first place I looked was the File menu, since that's where "New Tab" and "Close Tab" are.

Comment 3

[Dietrich Ayala (:dietrich)]

Hm, I associate the File menu with creation of new things, and the Edit menu with the modification of existing things (or of things that *used* to exist in this case :)

Comment 4

[Jesse Ruderman]

I think I associate File with windows, tabs, and files, and Edit with text.

Comment 5

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

Should go in History menu. Joe had a good design in d-a-f which I modified a bit here to get rid of some of the other proposed changes which were more places-specific -- I hope he doesn't mind.

================================
... | History | Bookmarks ...
====|---------|=================
    | Back                    |
    | Forward                 |
    | Home                    |
    |-------------------------|
    | Styrofoam Pa...Art      |
    | Breakthrou...Yetis      |
    | ...                     |
    |-------------------------|------------------------|
    | Recently Closed Tabs >> | Goat Quarterly         |
    |-------------------------| Banned Cana...everages |
    | Show in Sidebar         | Goat Owners Journal    |
    |-------------------------| My Mom's Blog          |
                              | Tacos!                 |
                              |------------------------|
                              | Open All in Tabs       | <-- (bug 305910)
                              --------------------------

Comment 6

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

--> Dietrich (sorry, amigo!)

Comment 7

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

--> blocking, target at beta 1, I owe dietrich a drink

Comment 8

[Weston Carloss]

I like the idea of a trash can/recycle bin icon with a drop down list of the recently closed tabs.  I think people like clicking icons more than expanding menu items.  Also, many people are used to the behavior of undo close tab extension where the user can right click on a page to get the undo close tab functionality.  Perhaps this would be a useful thing to have in addition to an icon.

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: implements the design in comment #5

The data structure returned by getClosedTabData is, um, grody. What's the proper way to return an array of JS objects from XPCOM?

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #5)

>                               |------------------------|
>                               | Open All in Tabs       | <-- (bug 305910)
>                               --------------------------
> 

The string added for this bug is not included in places builds, so I used the places string instead. We can use the 305910 string in the branch patch.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 228068 [details] [diff] [review]
implements the design in comment #5


General: This should follow the style of the rest of the objects like this in browser.js (AugmentTabs, FeedHandler, etc)

Also, if Beltzner specs the UI, ui-review is kinda redundant... :)

>+  /*
>+  // create separator
>+  var undoSep = histMenu.appendChild(document.createElement("menuseparator"));
>+
>+  // create menu item
>+  var undoMenu = histMenu.appendChild(document.createElement("menu"));
>+  undoMenu.setAttribute("label", gNavigatorBundle.getString("tabContext.undoCloseTab"));
>+  undoMenu.setAttribute("accesskey", gNavigatorBundle.getString("tabContext.undoCloseTabAccessKey"));
>+  undoMenu.setAttribute("disabled", true);
>+  var undoPopup= undoMenu.appendChild(document.createElement("menupopup"));
>+  undoPopup.setAttribute("id", "historyUndoPopup");
>+  */

we don't need this stuff since we're in the XUL itself, so let's just kill it (death to cruft)Also

>+// populate when the history menu is opened
>+HistoryMenu.populateUndoSubmenu = function PHM_populateUndoSubmenu() {
>+  var undoPopup = document.getElementById("historyUndoPopup");
>+
>+  // remove existing menu items
>+  while(undoPopup.hasChildNodes())
>+    undoPopup.removeChild(undoPopup.firstChild);

space after while (you do this a bunch, I'll stop nitpicking now)

>+  // get closed-tabs from nsSessionStore
>+  var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>+           getService(Ci.nsISessionStore);
>+  if(ss.getClosedTabCount(window) == 0) {
>+    undoPopup.parentNode.setAttribute("disabled", true);
>+    return;
>+  }

comment here why we're returning early (yes, its kinda obvious, but we need better comments all over the place)

>+  // enable menu
>+  undoPopup.parentNode.setAttribute("disabled", false);
>+

removeAttribute("disabled") is faster and does the same thing

>   getClosedTabData: function sss_getClosedTabDataAt(aWindow) {
>-    return this._windows[aWindow.__SSi]._closedTabs;
>+    try {
>+      var wrappedData = Cc["@mozilla.org/dictionary;1"].createInstance(Ci.nsIDictionary);
>+      var closedTabData = this._windows[aWindow.__SSi]._closedTabs;
>+      if (closedTabData.length == 0)
>+        return wrappedData;
>+
>+      // wrap it
>+      for (var i = 0; i < closedTabData.length; i++) {
>+        wrappedData.setValue(i, {wrappedJSObject: closedTabData[i]});
>+      }
>+      return wrappedData;
>+    } catch(ex) { debug("getClosedTabData: " + ex); }
>   },

this isn't pretty, I'm sure there's a better way to do this, but I don't remember what it is yet, and this will work well enough for b1.  Maybe comment in the IDL that this isn't the final data format.

Comment 12

[TimoT]

I'm seeing the following issue:

1) Open 2 tabs and close them
2) Try to restore the first tab from the list

The restored tab is not the one you clicked on the menu.

In my rather limited testing it seemed that only the last item in the list restores the corresponding tab, all the other entries seem to restore random tabs.

Comment 13

[Mike Schroepfer]

Why isn't this just wired up to Edit->Undo?

Comment 14

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

(In reply to comment #13)
> Why isn't this just wired up to Edit->Undo?

I've asked mconnor the same question, and in his absence, I'll give you the convincing argument he gave me: 

The Edit > Undo stack isn't an appropriate place to put this. That stack is actually maintained on a per-tab basis (so if I do something in Tab A, flip to Tab B and do something, then go back to A, the undo action acts on that tab) which means that the close action would have to be interspersed into that stack ... things would get strange pretty quickly.

Also, I don't think that I'd want to enter a world where if I hit Ctrl+Z a bunch of times and end up getting a bunch of recently closed tabs re-opened.

After some discussion, we both agreed that the History menu is a better place to put this, since really, it's in the same vein of function as Back and Forward.

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to comment #11)
> (From update of attachment 228068 [details] [diff] [review] [edit])
> 
> General: This should follow the style of the rest of the objects like this in
> browser.js (AugmentTabs, FeedHandler, etc)

I did it this way b/c it's adding to a object created in the places include, but doesn't really make sense to be in there. Also, it allows the code to be in place for both Places and non-Places builds. That said, I'll add an ifdef'd instantiation of the object for non-Places.

(In reply to comment #12)
> I'm seeing the following issue:
> 
> 1) Open 2 tabs and close them
> 2) Try to restore the first tab from the list
> 
> The restored tab is not the one you clicked on the menu.
> 
> In my rather limited testing it seemed that only the last item in the list
> restores the corresponding tab, all the other entries seem to restore random
> tabs.

Hm, I tested that scenario and it worked ok. I'll clean my tree and see if I can repro.

(In reply to comment #13)
> Why isn't this just wired up to Edit->Undo?

Beltzner already answered this, but I'd add that we need to be consistent with the behaviour of Edit->Undo: If we add undo-close-tab, then users would expect other actions there as well, such as undo-close-window or maybe undo-submit-form :)

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: patch with review comments fixed, and bug from comment #12 fixed

Gavin, could you please review the change that addresses the bug in comment #12? (interdiff coming)

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: interdiff for the bug in comment #12

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 228223 [details] [diff] [review]
patch with review comments fixed, and bug from comment #12 fixed

>Index: browser/base/content/browser-menubar.inc

>+#ifdef MOZ_PLACES
>+var HistoryMenu = {};
>+#endif

should be #ifndef as discussed

>Index: browser/components/sessionstore/src/nsSessionStore.js

>+      if(length > maxTabsUndo)

missing space here

>+   * Returns nsISimpleEnumerator

s/nsISimpleEnumerator/nsIDictionary/ ?

r=me on the interdiff

Comment 19

[Ed Hume]

We really ought to have an Undo Close Tab menuitem in the main context menu. Just a quick undo of the last closed tab.

And while you're rebuilding this, please think about adding a direct command reference in the menuitems. This will allow us themers to skin the menuitem no matter what the language (see bug 343703).

Comment 20

[Dietrich Ayala (:dietrich)]

(In reply to comment #19)
> We really ought to have an Undo Close Tab menuitem in the main context menu.
> Just a quick undo of the last closed tab.
> 
> And while you're rebuilding this, please think about adding a direct command
> reference in the menuitems. This will allow us themers to skin the menuitem no
> matter what the language (see bug 343703).
> 

Hi Ed,

This is already on the trunk. I'll file a separate bug for making undo-close-tab properly theme-able.

Comment 21

[Marek Stępień [:marcoos, inactive]]

Attachment: Fix wrong historyUndoMenu.accesskey

From attachment #228223 [details] [diff] [review] for Bug 342700:
+<!ENTITY historyUndoMenu.label "Recently Closed Tabs">
+<!ENTITY historyUndoMenu.accesskey "u">

There is no "u" in "Recently Closed Tabs", so it will display as "Recently Closed Tabs (u)"

This patch sets the historyUndoMenu.accesskey to "R".

Comment 22

[Dietrich Ayala (:dietrich)]

Attachment: branch patch

Asking for review on this again because I had to get the "Open in tabs" string from bookmarks.properties, which didn't seem to accessible in the scope of browser.js the way that browser.properties is. Is there a better way to get at this string?

Comment 23

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #22)
> Created an attachment (id=228273) [edit]
> branch patch

This patch suffers from the same problem I described in comment 21.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 228273 [details] [diff] [review]
branch patch

>Index: browser/base/content/browser.js

>+  // "open in tabs"
>+  var bundleService = Cc["@mozilla.org/intl/stringbundle;1"].
>+                     getService(Ci.nsIStringBundleService);
>+  var appLocale = Cc["@mozilla.org/intl/nslocaleservice;1"].
>+                 getService(Ci.nsILocaleService).getApplicationLocale();
>+  var stringBundle = bundleService.createBundle("chrome://browser/locale/bookmarks/bookmarks.properties", appLocale);

createBundle hasn't taken an appLocale param since bug 76332 was fixed. That some callers (like the ones in bookmarks.js) still pass it is bug 311672.

And what about Marek's accesskey patch, should it not also be rolled into the branch patch?

r=me with those addressed.

Comment 25

[Dietrich Ayala (:dietrich)]

(In reply to comment #24)
> (From update of attachment 228273 [details] [diff] [review] [edit])
> >Index: browser/base/content/browser.js
> 
> >+  // "open in tabs"
> >+  var bundleService = Cc["@mozilla.org/intl/stringbundle;1"].
> >+                     getService(Ci.nsIStringBundleService);
> >+  var appLocale = Cc["@mozilla.org/intl/nslocaleservice;1"].
> >+                 getService(Ci.nsILocaleService).getApplicationLocale();
> >+  var stringBundle = bundleService.createBundle("chrome://browser/locale/bookmarks/bookmarks.properties", appLocale);
> 
> createBundle hasn't taken an appLocale param since bug 76332 was fixed. That
> some callers (like the ones in bookmarks.js) still pass it is bug 311672.
> 
> And what about Marek's accesskey patch, should it not also be rolled into the
> branch patch?
> 
> r=me with those addressed.
> 

Yes, I'll add it to this patch. Can I consider this +r for adding it to trunk as well?

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 228272 [details] [diff] [review]
Fix wrong historyUndoMenu.accesskey

(In reply to comment #25)
> Yes, I'll add it to this patch. Can I consider this +r for adding it to trunk
> as well?

Yep!