Make undo close tab more discoverable

RESOLVED FIXED in Firefox 2 beta1

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Adam Guthrie, Assigned: dietrich)

Tracking

({fixed1.8.1, late-l10n})

2.0 Branch
Firefox 2 beta1
fixed1.8.1, late-l10n
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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
(Assignee)

Comment 1

12 years ago
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

12 years ago
The first place I looked was the File menu, since that's where "New Tab" and "Close Tab" are.
(Assignee)

Comment 3

12 years ago
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

12 years ago
I think I associate File with windows, tabs, and files, and Edit with text.
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)
                              --------------------------
--> Dietrich (sorry, amigo!)
Assignee: nobody → dietrich
--> blocking, target at beta 1, I owe dietrich a drink
Flags: blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED

Comment 8

12 years ago
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.
(Assignee)

Comment 9

12 years ago
Created attachment 228068 [details] [diff] [review]
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?
Attachment #228068 - Flags: ui-review?(beltzner)
Attachment #228068 - Flags: review?(mconnor)
(Assignee)

Updated

12 years ago
Keywords: late-l10n
(Assignee)

Comment 10

12 years ago
(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 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.
Attachment #228068 - Flags: ui-review?(beltzner)
Attachment #228068 - Flags: review?(mconnor)
Attachment #228068 - Flags: review+

Comment 12

12 years ago
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

12 years ago
Why isn't this just wired up to Edit->Undo?
(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.
(Assignee)

Comment 15

12 years ago
(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 :)
(Assignee)

Comment 16

12 years ago
Created attachment 228223 [details] [diff] [review]
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)
Attachment #228068 - Attachment is obsolete: true
Attachment #228223 - Flags: review?(gavin.sharp)
(Assignee)

Comment 17

12 years ago
Created attachment 228224 [details] [diff] [review]
interdiff for the bug in comment #12
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
Attachment #228223 - Flags: review?(gavin.sharp) → review+

Comment 19

12 years ago
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).
(Assignee)

Comment 20

12 years ago
(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.
Created attachment 228272 [details] [diff] [review]
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".
Attachment #228272 - Flags: review?(gavin.sharp)
(Assignee)

Comment 22

12 years ago
Created attachment 228273 [details] [diff] [review]
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?
Attachment #228273 - Flags: review?(gavin.sharp)
Attachment #228273 - Flags: approval1.8.1?
(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 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.
Attachment #228273 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 25

12 years ago
(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 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!
Attachment #228272 - Flags: review?(gavin.sharp) → review+

Updated

12 years ago
Attachment #228273 - Flags: approval1.8.1? → approval1.8.1+
(Reporter)

Updated

12 years ago
Depends on: 343801
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 343807

Updated

12 years ago
No longer blocks: 343807

Updated

12 years ago
Blocks: 345916
You need to log in before you can comment on or make changes to this bug.