Closed Bug 254021 (undoclosetab) Opened 20 years ago Closed 18 years ago

Ability to open accidentally closed tabs

Categories

(Firefox :: Tabbed Browser, enhancement, P1)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 2 beta1

People

(Reporter: lalo.martins, Assigned: dietrich)

References

(
URL
)

Details

(Keywords: fixed1.8.1, Whiteboard: [swag: 1d] 181b1+)

Attachments

(1 file, 5 obsolete files)

addresses mano's review comments
11.51 KB, patch
mconnor
: review+
mconnor
: approval-branch-1.8.1+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040703 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040703 Firefox/0.9.1

I'm filing a few enhancement request for the coolest features of the wonderful
TabBrowser Extensions, so that they can be considered for post-1.0 and maybe
voted on.

This request is for an "undo close tab" or "recently closed tabs" (sub)menu.
When a tab is closed, it should be saved to a cache, and there should be a menu
somewhere in the UI to show this cache. Clicking on an entry on the menu opens a
new tab with the same contents as what used to be in the corresponding entry.

This should save the "buffer" to the cache, not just the URL, so that
"unclosing" a tab that was displaying the results of submitting a form would
still work.

An UI command that can be bound to a key should be available to "unclose" the
last closed tab; this should be bound by default to something (TBE uses
Ctrl-Shift-Z, but maybe Ctrl-Alt-W or something like that would also make sense).

Reproducible: Always
Steps to Reproduce:




http://white.sakura.ne.jp/~piro/xul/_tabextensions.html.en

*** This bug has been marked as a duplicate of 152391 ***
Status: UNCONFIRMED → RESOLVED
Closed: 20 years ago
Resolution: --- → DUPLICATE
reopend, the tabs code is forked
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
*** Bug 261738 has been marked as a duplicate of this bug. ***
*** Bug 262227 has been marked as a duplicate of this bug. ***
->NEW

leaving it unco will surely let it rot
Status: UNCONFIRMED → NEW
Ever confirmed: true
The default key binding for undo is Ctrl-Z (on MS-Windows at least).
Unless it conflicts with some already existing key binding, that would be the
logical choice.
I would be concerned about going overboard in the implementation.
In my experience, the only time I want an undo of a delete tab is 
immediately after deleting it -- it was a mistake.  As such, ^Z
or undo on the edit menu is where one expects to find it.

Any other time, I am more than willing to find what I was doing in
the history and reopen it from there.  Seems like doing much more
than this is duplicating much of the history function, something I
view as undesirable and leading to code bloat.

see: http://extensionroom.mozdev.org/more-info/undoclosetab

it has multiple levels of undo (if i close 4 tabs, i can bring all four back),
but it's bound to the weird ctrl+shift+f12. ctrl+z would make it perfect, but
might interfer with ctrl+z on pages with forms...
The way Tabbrowser Extensions handled it was by middle-clicking on a blank
section of the tab bar.  It worked really well for me, except that if the tab
bar was hidden (because only one tab was open after closing the other) I would
have to open a new tab with Ctrl-T and then undo the closed tab.  Regardless of
what the shotcut for it is, though, I think it deserves a place in the menus. 
That is really the only feature of TBE that I miss now that I don't use it.
(In reply to comment #9)
The other issue with the middle-clicking option, I suppose, is that it only
works for people who have a wheel-mouse (or perhaps a 3-button mouse), which
would exclude a lot of people.
*** Bug 275820 has been marked as a duplicate of this bug. ***
*** Bug 280181 has been marked as a duplicate of this bug. ***
*** Bug 287535 has been marked as a duplicate of this bug. ***
^Z interferes with undo of text editing -- you don't want to hit Ctrl+W, W, or
something like that, and lose the ability to undo close tabs. Similarly, you
don't want to do Ctrl+W, then come back and look at each of your tabs and think
"wait a minute, which was the one that I closed?", and try to undo-close-tab
just to find it undoes whatever edit you did on that page. But then you don't
want the opposite either -- closing a page, changing tab, undoing what you
edited there previously and finding that puts your tab back.

So they should be separate.
Assignee: bugs → nobody
Using Ctrl-Z is a good idea but it covers only a HALF of that issue.

So, if you have one tab open, it is useful to use Ctrl-Z. Just press it and your
state is rolled back (text edition, window close, etc). The same is when you use
multiple tabs/windows but remember what tab/window initiated new window/tab
opening - open that tab/window and press buttons.

BUT: if you have a lot of tabs opened, and you don't remember which one has
opened needed window, or you closed that opener window/tab (so there is no
windows/tabs that initiated opening window that you'd like to recover) - using
Ctrl-Z wouldn't help you. In that case there must be points in menu.

TOTAL: Ctrl-Z really useful, but nodes in menu is needed too.
(In reply to comment #8)
> see: http://extensionroom.mozdev.org/more-info/undoclosetab
> 
> it has multiple levels of undo (if i close 4 tabs, i can bring all four back),
> but it's bound to the weird ctrl+shift+f12. ctrl+z would make it perfect, but
> might interfer with ctrl+z on pages with forms...

This is the one I had in mind when voting on this bug. Since the dragging tabs
function was rolled in to the core code, I think this is next. 

Is this a duplicate to Bug 152391 or vice versa?
>>Is this a duplicate to Bug 152391 or vice versa?

I think that's Mozilla (Seamonkey)'s tabs are different
This should be marked to block bug 308396.
Marking blocking for investigation. Resummarizing because the "undo" model doesn't necessarily naturally apply to window management - it's more of an editing thing.
Flags: blocking-firefox2+
Summary: request: undo close tab → Ability to open accidentally closed tabs
Alias: undoclosetab
So I looked at the various ways of trying to store the closed tab, and reached the conclusion that the method that best satisfied the accidentally closed case is to simply hide the tab (selecting the next unhidden tab via the same pattern as we currently do (tab owner, right, left)) and really close it after X seconds (my in-flight first-cut implementation defaulted to 60).  The big advantage here is that plugins/AJAX state are exactly as the user left it, without lots of random magic to re-layout and restore states.  It also should be fairly immune from leaks if done right (I'll have a patch early next week when I'm back in my own office).

Adding this into the Undo stack isn't obvious, certainly, so the goal is to provide some means of opening one/all of the "closed" tabs, which I'm waffling a bit on.  One option is to add a submenu to the tab context menu to show the available tabs and a Reopen All option.  Ideas/feedback would be most excellent.
Assignee: nobody → mconnor
A nice idea. I don't think hiding the tab is enough - consider the edge case of content popping an alert every 10 seconds or a more realistic case of annoying ad that eats 100% of CPU. Instead closing a tab should freeze scripts just like navigating from a page does (and restoring is a matter of going back using bfcache).

As for 60 seconds timeout, if you mean the tab would disappear from the closed tabs list (whatever the UI will be) after that timeout, I think it's wrong. Always keeping last X closed tabs in the hidden state and relying on other mechanism (possibly browser.sessionhistory.max_viewers?) for memory management makes more sense to me.

Mike - yeah hiding the tab seems like the most complete solution to addressing state, entered data etc. One problem I can think of though: are there consequences for keeping a document loaded in a tab around after the user has requested that it go away? Is this similar at all to bfcache and thus not worth worrying about?
Can't we leverage the bfcache work to keep the "document state" around even after we close the tab? i.e. when the user closed the tab we would push the document into "bfcached" mode (firing appropriate events), and if we need to undo we can restore that document into a new tab (not sure whether bfcache could handle restoring into a different docshell than the document started in).
I tried hiding tabs before as a premise for an extension. While I didn't ever finish it (due to lack of personal interest), I turned up some problems. 

Firstly, the tab still exists. I know that's kinda obvious, but it becomes a problem when you want to close the window - you think you only have one tab open (i.e. tab bar is hidden) and you get told that you still have many open tabs.

I also found (although possibly due to my implementation) that there were a lot of nasty issues that need to be double-checked, such as CTRL + Tab and CTRL + number getting the tab / browser order out of sync. Closing visible tabs also had a tendency to focus a hidden tab.

These are both problems that can be overcome, but in general it would seem better to find some other way of doing this. 

If touching bfcache isn't an issue, how about cloning the whole tab node (and presumably it's browser contents), holding it as an object in memory and appending it back if necessary? 
s/issue/option

Sorry for bugspam.

Also, I have to agree with comment 21, 100% CPU usage and high memory usage in a hidden tab would definitely be a nasty thing. I would say that even more likely is the annoyance factor of dynamic content such as Flash or Shockwave playing music or messing with focus in the background. 

Closing a tab should mean that these things go away, rather than hide in the background.
Blocks: 322898
Attached patch in progress (obsolete) — — Splinter Review 
There's some thinking around reusing some of the session saver capabilities for this, but I'm not sure that's going to pan out in time.  bfcache is incomplete in what it caches, and in the case of an interrupted load/form submission won't really do what people expect in this case.  The 60 second timeout doesn't completely solve the lockup issue, but most of the 100% CPU bugs I notice are js, which hogs the UI thread anyway.

includes bryner's tab select/open/close/move events patch from bug 322898.  not done yet, still need to do the following:

- implement pref for tab close delay (right now we default to 60 seconds)
- ensure keyboard compat
- fix bookmarks open in tabs to work with this properly, to some definition of properly
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 2 alpha1
mconnor, you probably want to also reset the type to and from content-targetable (as in, "not-really-closed" tabs should not be targetable).  I guess we can do that after bug 326009 is fixed?
Attached patch pretty much done (obsolete) — — Splinter Review 
This doesn't have bz's changes for content-targetable, I'll change that in sync with bug 326009
Attachment #212558 - Attachment is obsolete: true
Attachment #212756 - Flags: ui-review?(beltzner)
Attachment #212756 - Flags: review?(robert.bugzilla)
Comment on attachment 212756 [details] [diff] [review]
pretty much done

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

>@@ -1281,21 +1282,21 @@ function ctrlNumberTabSelection(event)

>-  if (index >= gBrowser.tabContainer.childNodes.length)
>+  if (index >= gBrowser.mTabs.length)
>     return;

Shouldn't this be openTabsCount to match below?

>   var oldTab = gBrowser.selectedTab;
>-  var newTab = gBrowser.tabContainer.childNodes[index];
>+  var newTab = gBrowser.openTabs[index];

>Index: browser/components/bookmarks/content/bookmarks.js

>@@ -699,38 +699,41 @@ var BookmarksCommand = {
>     var resource = RDF.GetResource(aURI);
>     var urlArc   = RDF.GetResource(gNC_NS+"URL");
>     RDFC.Init(BMDS, resource);
>     var containerChildren = RDFC.GetElements();
> 
>     if (aTargetBrowser == "current" || aTargetBrowser == "tab") {
>       var browser  = w.document.getElementById("content");
>       var tabPanels = browser.browsers;
>-      var tabs = browser.mTabContainer.childNodes;
>-      var tabCount  = tabPanels.length;
>+      var tabs = browser.openTabs;
>+      var tabCount  = tabs.length;
>       var doReplace = PREF.getBoolPref("browser.tabs.loadFolderAndReplace");
>       var loadInBackground = PREF.getBoolPref("browser.tabs.loadBookmarksInBackground");
>       var index0;
>       if (doReplace)
>         index0 = 0;
>       else {
>-        for (index0=tabCount-1; index0>=0; --index0)
>-          if (tabPanels[index0].webNavigation.currentURI.spec != "about:blank")
>+        for (index0 = tabCount - 1; index0 >= 0; --index0) {
>+          if (tabs[index0] && browser.getBrowserForTab(tabs[index0]).webNavigation.currentURI.spec == "about:blank")

Could just be getBrowserForTab.currentURI.spec without the ".webNavigation.". Also, why would tabs[index0] be null? Guess this code is all going away soon so it doesn't matter.

>Index: toolkit/content/widgets/tabbox.xml

>+            // Support both the old "select" event and the new, better-named
>+            // "TabSelect" event.
>+            var event = document.createEvent("Events");
>+            event.initEvent("select", false, true);
...
>+            event.initEvent("TabSelect", true, false);

Why the discrepancy in the canBubble and cancelable args?

>Index: toolkit/content/widgets/tabbrowser.xml

>+            <xul:menuseparator tbattr="tabbrowser-restore"/>
>+            <xul:menuitem label="&restoreClosedTabs.label;" accesskey="&restoreClosedTabs.accesskey;"
nit: put all attributes on their own line?

>@@ -1261,51 +1282,179 @@
>       <method name="removeTab">
>         <parameter name="aTab"/>
>         <body>
>           <![CDATA[
>-            if (aTab.localName != "tab")
>-              aTab = this.mCurrentTab;
>+            if (this.openTabsCount == 1) {
>+              this.loadURI("about:blank");
>+              return;
>+            }

Hmm, should this clear session history too? I seem to have some recollection of code that used to do that, but maybe I'm just imagining things.

>+              // Find and locate the tab in our list.
>+              for (var i = 0; i < this.mTabs.length; i++) {
>+                if (this.mTabContainer.childNodes[i] == aTab)
>+                  index = i;
>               }

XPFE added a getTabIndex(aTab) method, might be worth adding here too (for open tabs only, I guess)?

>+            // we're closing the current tab, need to select a new index.
>+            var newIndex = -1;
>+            var currentIndex = this.mTabContainer.selectedIndex;
>+            if (currentIndex == index) {
>+              if ("owner" in aTab && aTab.owner &&
>+                  this.mPrefs.getBoolPref("browser.tabs.selectOwnerOnClose")) {
>+                for (i = 0; i < this.mTabContainer.childNodes.length; ++i) {
>+                  tab = this.mTabContainer.childNodes[i];
>+                  if (tab == aTab.owner) {
>+                    newIndex = i;
>+                    break;
>+                  }

Could use getTabIndex(aTab.owner) here.

>+                }
>+              }
>+              else {
>+                // close to right, unless there are no tabs to the right that aren't "closed"
>+                for (i = index + 1;i < this.mTabs.length;i++) {
nit: spaces after ";"s

>-            var ds = this.getBrowserForTab(aTab).docShell;
...
>-            if (ds.contentViewer && !ds.contentViewer.permitUnload())
>-              return;

Moving this to the "really close tab" stage doesn't look right -  if a page has a onbeforeunload handler that's an alert, for example, won't the user get the alert 60 seconds after he closes the tab? I haven't tested this but seems like funny things could happen then. Is there any harm in leaving this here? I guess there's the risk that pages will do something desctructive onbeforeunload which would result in unexpected changes when re-opening, or that multiple onbeforeunloads could be fired (by closing and reopening), but I'm not sure how to avoid that. Given that onbeforeunload is mostly used for "do you want to save changes" features, I think leaving it here is probably least risky, though I may be missing something.

>+            // Remove this tab as the owner of any other tabs, since it's going away.
>+            for (i = 0; i < this.mTabContainer.childNodes.length; ++i) {

Guess this could just loop through openTabs? If a tab is closed and then re-opened, it's already lost it's owner, right (due to the change of selectedTab)? I don't think it's worth trying to make the parenting thing work with reopened tabs, though I'm not sure I've thought through all the possible cases.

>+            // set up a timer to close the tab for real after a delay
>+            var callback = {
>+              tabbrowser: this,
>+              tab:        aTab,
>+              notify: function(timer) {
>+                this.tabbrowser._reallyRemoveTab(this.tab);
>+              }
>+            }

Hmm, should you null out tabbrowser and tab after calling _reallyRemoveTab?

>+            aTab.timer = Components.classes["@mozilla.org/timer;1"]
>+                                  .createInstance(Components.interfaces.nsITimer);
nit: alignment

>+      <method name="restoreAllTabs">
>+        <body>
>+          <![CDATA[
>+            for (var i = 0;i < this.mTabs.length;i++) {
nit: space after ";"s

>+      <method name="_reallyRemoveTab">

>+            if (aTab.localName != "tab")
>+              aTab = this.mCurrentTab;

Doesn't seem like this should happen. Why default to the current tab?

>+            var l = this.mTabs.length;
>+
>+            var ds = this.getBrowserForTab(aTab).docShell;
>+
>+            if (ds.contentViewer && !ds.contentViewer.permitUnload())
>+              return;

See comment above about moving this.

>               // Find and locate the tab in our list.
>               for (var i = 0; i < l; i++)
>-                if (this.mTabContainer.childNodes[i] == aTab)
>+                if (this.mTabs[i] == aTab)
>                   index = i;

Could use getTabIndex here too.

>+            this.mTabBox.selectedPanel = this.getBrowserForTab(this.mCurrentTab).parentNode;

Hmm, why is this still here? The tab is already unselected, right?

>+      <!-- open in the sense of not being in the not-really-closed state -->
>+      <property name="openTabs" readonly="true">
>+        <getter>
>+          <![CDATA[
>+            var openTabs = new Array();
nit: var openTabs = []; ?

>+            for (var i = 0; i < this.mTabs.length; i++) {
>+              if (!this.mTabs[i].hasAttribute("closed"))
>+                openTabs[openTabs.length] = this.mTabs[i];
nit: openTabs.push(this.mTabs[i]) ?

>@@ -2166,16 +2350,20 @@
>+          
>+          try {
>+            this.mTabCloseDelay = this.mPrefs.getIntPref("browser.tabs.closeDelay");
>+          } catch(e) {}

Maybe better to have the pref be in seconds as opposed to milliseconds?


Still haven't tested this out, but it's late so I'll probably do that tomorrow.
Mike, have you summarized the overall behavior/implementation somewhere on the wiki?
Comment on attachment 212756 [details] [diff] [review]
pretty much done

I don't have much to add to Gavin's review in comment #29 - I haven't compiled this yet

>Index: browser/components/bookmarks/content/bookmarks.js
...
>@@ -699,38 +699,41 @@ var BookmarksCommand = {
>     var resource = RDF.GetResource(aURI);
>     var urlArc   = RDF.GetResource(gNC_NS+"URL");
>     RDFC.Init(BMDS, resource);
>     var containerChildren = RDFC.GetElements();
> 
>     if (aTargetBrowser == "current" || aTargetBrowser == "tab") {
>       var browser  = w.document.getElementById("content");
>       var tabPanels = browser.browsers;
>-      var tabs = browser.mTabContainer.childNodes;
>-      var tabCount  = tabPanels.length;
>+      var tabs = browser.openTabs;
>+      var tabCount  = tabs.length;
>       var doReplace = PREF.getBoolPref("browser.tabs.loadFolderAndReplace");
>       var loadInBackground = PREF.getBoolPref("browser.tabs.loadBookmarksInBackground");
>       var index0;
>       if (doReplace)
>         index0 = 0;
>       else {
>-        for (index0=tabCount-1; index0>=0; --index0)
>-          if (tabPanels[index0].webNavigation.currentURI.spec != "about:blank")
tabPanels is no longer used and you didn't remove its declaration:

>Index: toolkit/content/widgets/tabbox.xml
...
>@@ -287,20 +287,25 @@
...
>-            // Fire an onselect event for the tabs element.
>-            var event = document.createEvent('Events');
>-            event.initEvent('select', false, true);
>+
>+            // Support both the old "select" event and the new, better-named
>+            // "TabSelect" event.
What is the purpose of the TabSelect event and shouldn't it state the purpose here instead of it being new and better named?

Index: toolkit/content/widgets/tabbrowser.xml
...
>@@ -600,20 +609,25 @@
...
>-            var disabled = this.mPanelContainer.childNodes.length == 1;
>+            var hidden = this.openTabsCount == 1;
>             var menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
>             for (var i = 0; i < menuItems.length; i++)
>-              menuItems[i].disabled = disabled;
>+              menuItems[i].hidden = hidden;
>+              
>+            hidden = this.closedTabsCount == 0;
>+            menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-restore");
>+            for (i = 0; i < menuItems.length; i++)
>+              menuItems[i].hidden = hidden;
Glad to see hiding of commands that aren't applicable... especially the ones that were enabled (e.g. close other tabs with one tab open, etc.)

>@@ -1261,51 +1282,179 @@
...
>+            var i;
>+            var index = -1;
>+
>+            if (this.mCurrentTab == aTab)
>+              index = this.mTabContainer.selectedIndex;
>+            else {
>+              // Find and locate the tab in our list.
>+              for (var i = 0; i < this.mTabs.length; i++) {
>+                if (this.mTabContainer.childNodes[i] == aTab)
>+                  index = i;
>               }
you declare i twice
Can this be rewritten as follows?
>+            if (this.mCurrentTab == aTab)
>+              var index = this.mTabContainer.selectedIndex;
>+            else {
>+              index = -1;
>+              // Find and locate the tab in our list.
>+              for (var i = 0; i < this.mTabs.length; i++) {
>+                if (this.mTabContainer.childNodes[i] == aTab)
>+                  index = i;
>               }


>+            // Remove this tab as the owner of any other tabs, since it's going away.
>+            for (i = 0; i < this.mTabContainer.childNodes.length; ++i) {
>+              var tab = this.mTabContainer.childNodes[i];
>+              if ("owner" in tab && tab.owner == aTab)
>+                // |tab| is a child of the tab we're removing, make it an orphan
>+                tab.owner = null;
>+            }
What are the implications of removing the owner for a tab and then the tab being restored?

>+
>+            if (this.openTabsCount == 2) {
>               var autohide = this.mPrefs.getBoolPref("browser.tabs.autoHide");
>               var tabStripHide = !window.toolbar.visible;
>               if (autohide || tabStripHide)
>                 this.setStripVisibilityTo(false);
>             }
> 
>+
nit: remove the extra line

Besides addressing / responding to these and Gavin's review details as to what the behaviors are for a hidden tab especially in regards to scripts, etc. (e.g. I open a page that is taking forever to load so I close it - does the page continue to load and thereby continue to utilize my available bandwidth for up to 60 seconds?) should be documented before this is +'d.
Attachment #212756 - Flags: review?(robert.bugzilla) → review-
Just compiled and the first thing I tried was loading http://www.badgerbadgerbadger.com/ - when I closed the tab it of course continued to play the flash movie which includes sound.
In answer to the example of behaviors I provided (e.g. I open a page that is taking forever to load so I close it - does the page continue to load and thereby continue to utilize my available bandwidth for up to 60 seconds?) it does continue to load in the background. This isn't so bad with a page with a small amount of content but any page that has media (e.g. video, sound, flash, etc.) will continue to load after the page is closed and once loaded play that media including sound if available. Am I missing something here because not doing this - especially in regards to sound - seems to me to be a base requirement for this type of functionality.
Presumably this happens because the tab's equivalent browser still exists in the background. 

Assuming that the goal of doing this is to restore the tab (and it's loaded content) exactly as things were when it was closed, would it be possible to implement something closer to the bfcache? 

If bfcache is taking things too far, how viable would it be to implement this in sync with the session save/restore feature that is planned - both of these features will need ways to restore tabs and the tab content and cope with form data, etc.
(In reply to comment #34)
> In answer to the example of behaviors I provided (e.g. I open a page that is
> taking forever to load so I close it - does the page continue to load and
> thereby continue to utilize my available bandwidth for up to 60 seconds?) it
> does continue to load in the background. 


Also, don't some people get charged by bandwidth rather than time? This could be a real bad thing for them financially. 


It seems to me that we ought to put the closed tab into fast-backed mode.
I'm not saying the "leave it intact and hide the tab" method is perfect.  We could use bfcache, but the tradeoffs there are:

- interrupts the pageload, so you'd have to reload the tab anyway (dangerous if you're midway through a form submission)
- doesn't cache everything, so it only partially addresses dataloss
- we haven't solved the POSTDATA bug such that you can go back to the results of a POST request.

To fix those, we would need to:
- let the pageload complete in the background, and then dump to bfcache
- allow bfcache to optionally store everything regardless of headers
- allow going back to the displayed results of a POST submission

The tradeoffs for the current approach are:
- annoying or large things don't go away and stop loading immediately.

To fix this, we would need to:
- Find a heuristic (and possibly a user action) to decide when to permanently close the tab (i.e. determining when tabs are closed deliberately) instead of doing the delayed close.  For example, if a tab is opened in the background, and closed by middle-click without ever being focused, there's no point in storing the tab (that's an easy one).
Another tradeoff is that when you have finished with a page with media on it and close the page before the media has completed or if it is looped it doesn't go away as well. So, closing a page before any media on the page has finished - especially with sounds - continues to play until the timer fires... not to mention other things like java applets, scripts, etc. - I could be wrong in thinking that people sometimes close web pages that contain media before the media has finished but I don't believe that I am. :/
Ultimately, the choice is between making closing tabs absolute and immediate and making reopening the closed tab 100% reliable.  We could use bfcache, and warn before close in the cases that we won't restore properly, but that sucks too.

Or we could just pass on this until such time as we can do this perfectly and without compromise, and just do confirm-on-data-in-form to prevent the most common dataloss case.  It's highly imperfect as well, but if neither solution is acceptable enough, then we can't fix this for this release.
I've voiced my concerns and if you truly think this solution is acceptable then write out the behaviors (e.g. on the wiki) that this will provide and see if others find it to be acceptable. For myself, I am more than willing to consider / compromise regarding bandwidth utilization, applets running, plugins running, CPU utilization, etc. but closing a tab and having sound from media on a page continue without anyway to stop it except by exiting the application or waiting for the timer to fire is a deal breaker for me... perhaps no one else sees it the same way as I do and making it clear what the behaviors are would be a good start.
I think that the dataloss of closing a posted form is acceptable.  I feel that if a tab is closed, whatever was loading should cease immediately.  If the page was fully loaded, then use bfcache.

What happens to bfcache if you click a link on a page before its fully loaded?
> What happens to bfcache if you click a link on a page before its fully loaded?

We don't put that page in bfcache.
Maybe I've missed something in the technical discussion of this. Why cannot closing a tab be the same as pressing the stop button on the toolbar? This should stop page-loading and media-playing. A user can then pull that page back and if they really require whatever was going on on the page they can reload it. Importantly, they won't lose the URL.

As an aside, maybe what would be more useful for the user is having the URL stored in the history in a way that's easy to get hold of.
The reason I wanted this enhancement is that my normal behaviour is to read a page and load any interesting links into additional tabs. They load while I keep reading and then I go take a look at them. As I follow the tree of my interest I prune behind myself. Sometimes I accidentally close the wrong tab and I may not even know which tab I accidentally closed.
> Maybe I've missed something in the technical discussion of this.

You missed the fact that a URL is not enough to identify a page, apparently?
Let me see if I have the situations right:
1) Page is fully loaded
2) Page is still loading
3) Page has form data and is fully loaded
4) Page has form data and is still loading

1 - bfcache should work, right?
2 - we need to hit "stop" and then bfcache whatever it is
3&4 - What do we do for bfcache for these?
Target Milestone: Firefox 2 alpha1 → Firefox 2 alpha2
Attachment #212756 - Attachment is obsolete: true
Attachment #212756 - Flags: ui-review?(beltzner)
Flipping to Dietrich, may be able to leverage the session restore work for a better solution.
Assignee: mconnor → dietrich
Status: ASSIGNED → NEW
Adding SWAG for leveraging session-restore (328159) to implement this.
Status: NEW → ASSIGNED
Whiteboard: 4d

        Attachment #221000 -
        Flags: review?(mconnor)

    
    

    
  
Comment on attachment 221000 [details] [diff] [review]
undo-close-tab via session-restore

missing a file, sorry for bugspam

        Attachment #221000 -
        Attachment is obsolete: true

        Attachment #221000 -
        Flags: review?(mconnor)

    
    

    
  
Retargetted to beta1
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch against trunk (obsolete)
        — 
          — Splinter Review
      

    


  
This patch is against trunk. It can be applied to branch once branch has been synced to trunk (bug 337320, patch 222637).

        Attachment #223205 -
        Flags: review?(mconnor)

    
  

        Attachment #223205 -
        Attachment is obsolete: true

        Attachment #223205 -
        Flags: review?(mconnor)

    
    

    
  


  
implemented some changes talked about in #developers

        Attachment #223260 -
        Flags: review?(mconnor)

    
    

    
  
Comment on attachment 223260 [details] [diff] [review]
sans tabs, moved into obj, moved into browser.js

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


>   // Initialize the microsummary service by retrieving it, prompting its factory
>   // to create its singleton, whose constructor initializes the service.
>   Cc["@mozilla.org/microsummary/service;1"].getService(Ci.nsIMicrosummaryService);
>+
>+  // browser-specific tab augmentation
>+  AugmentTabs.init();

Why should it be an object (i.e. what is the advantage of scoping here). Note we're already have
addBookmarkMenuitems in the global scope...

I'm wondering if we should just move the context menu from tabbrowser.xml to browser.xul
(should be straightforward).

> }
> 
> function BrowserShutdown()
> {
>   var os = Components.classes["@mozilla.org/observer-service;1"]
>     .getService(Components.interfaces.nsIObserverService);
>   os.removeObserver(gSessionHistoryObserver, "browser:purge-session-history");
>   os.removeObserver(gXPInstallObserver, "xpinstall-install-blocked");
>@@ -6494,8 +6497,52 @@ var BrowserController = {
>   }
> };
> window.controllers.appendController(BrowserController);
> 
> #include browser-places.js
> #include ../../../toolkit/content/debug.js
> 
> #endif
>+
>+/**
>+ * This object is for augmenting tabs
>+ */
>+ var AugmentTabs = {
>+  /**
>+   * Called in delayedStartup
>+   */
>+  init: function at_init() {
>+    // add the tab context menu for undo-close-tab (bz254021)
>+    this.addUndoCloseTabContextMenu();
>+  },
>+
>+  /**
>+   * Add undo-close-tab to tab context menu
>+   */
>+  addUndoCloseTabContextMenu: function at_addUndoCloseTabContextMenu() {

If we do keep this scoped, let's name this _addUndoCloseTabContextMenu.

>+    // get tab context menu
>+    var tabbrowser = getBrowser();

gBrowser would do.

>+    var tabMenu = document.getAnonymousElementByAttribute(tabbrowser,"anonid","tabContextMenu");
>+
>+    // get menu label
>+    var menuLabel = gNavigatorBundle.getString("tabContextUndo");
>+
>+    // create new menu item
>+    var undoCloseTabItem = document.createElement("menuitem");
>+    undoCloseTabItem.setAttribute("label", menuLabel);

Please add an accesskey.

>+    undoCloseTabItem.setAttribute("oncommand", "AugmentTabs.undoCloseTab();");
>+

Might be cleaner to use addEventListener here (I assume that's why it's scoped?).

>+    // add to tab context menu
>+    var insertPos = tabMenu.lastChild.previousSibling;
>+    tabMenu.insertBefore(undoCloseTabItem, insertPos);
>+  },
>+
>+  /**
>+   * Re-open the most-recently-closed tab
>+   */
>+  undoCloseTab: function at_undoCloseTab() {
>+    // get session-store service
>+    var ss = Components.classes["@mozilla.org/browser/sessionstore;1"].
>+                        getService(Components.interfaces.nsISessionStore);

Use Cc and Ci here (they're defined earlier).


>Index: browser/components/sessionstore/nsISessionStore.idl
>===================================================================

> 
> #include "nsISupports.idl"
> 
>+interface nsIDOMWindow;
>+
> /**
>  * nsISessionStore keeps track of the current browsing state - i.e.
>  * tab history, cookies, scroll state, form data, POSTDATA and window features
>  * - and allows to restore everything into one window.
>  */
> 
> [scriptable, uuid(07fada1e-c3a6-11da-95a7-00e08161165f)]

rev the uuid please.

> interface nsISessionStore : nsISupports
> {
>   /**
>    * Initialize the service
>    */
>   void init();
>+
>+	/**
>+	 * @param aWindow is the window to reopen a closed tab in.
>+	 * @param aIndex  indicates the window to be restored (FIFO ordered).
>+	 */

make that
>+	/**
>+	 * @param aWindow
>+                the window to reopen a closed tab in.
>+	 * @param aIndex
>+                indicates the window to be restored (FIFO ordered).
>+	 */

>+	void undoCloseTab(in nsIDOMWindow aWindow, in unsigned long aIndex);

no hard tabs please.

> };
>Index: browser/components/sessionstore/src/nsSessionStore.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v
>retrieving revision 1.12
>diff -u -8 -p -r1.12 nsSessionStore.js
>--- browser/components/sessionstore/src/nsSessionStore.js	21 May 2006 02:56:20 -0000	1.12
>+++ browser/components/sessionstore/src/nsSessionStore.js	24 May 2006 22:06:14 -0000
>@@ -435,17 +435,17 @@ SessionStoreService.prototype = {
>       _this.onTabAdd(aEvent.currentTarget.ownerDocument.defaultView, aEvent.target);
>       }, false);
>     tabpanels.addEventListener("DOMNodeRemoved", function(aEvent) {
>       _this.onTabRemove(aEvent.currentTarget.ownerDocument.defaultView, aEvent.target);
>       }, false);
>     tabpanels.addEventListener("select", function(aEvent) {
>       _this.onTabSelect(aEvent.currentTarget.ownerDocument.defaultView, aEvent.currentTarget);
>       }, false);
>-    tabbrowser.addEventListener("TabClose", function(aEvent) {
>+    tabbrowser.addEventListener("DOMNodeRemoved", function(aEvent) {
>       _this.onTabClose(aEvent.currentTarget.ownerDocument.defaultView, aEvent.originalTarget);
>       }, false);

Er, a DOMNodeRemoved event on tabbrowser does not mean a tab was closed, that is also why you're receiving it
it for more than one target.

The patch on https://bugzilla.mozilla.org/show_bug.cgi?id=318168 adds the TabClose event.


>   undoCloseTab: function sss_undoCloseWindow(aWindow, aIx) {
>-    var tabs = this._windows[aWindow.__SSi]._closedTabs;
>+    var closedTabs = this._windows[aWindow.__SSi]._closedTabs;
>+
>+    // default to the most-recently closed tab
>+    aIx = aIx || 0;
>     
>-    if (aIx in tabs) {
>+    if (aIx in closedTabs) {
>       var browser = aWindow.getBrowser();
>-      var tabData = tabs.splice(aIx, 1);
>-      tabData._tab = browser.addTab();
>+
>+      // fetch the data of closed tab, while removing it from the array
>+      var closedTab = closedTabs.splice(aIx, 1).shift();
>+      var closedTabState = closedTab.state;
>+
>+      // create a new tab
>+      closedTabState._tab = browser.addTab();
>       
>       // restore the tab's position
>-      browser.moveTabTo(tabData._tab, tabData[0].pos);
>+      browser.moveTabTo(closedTabState._tab, closedTab.pos);
> 
>       // restore the tab's state
>-      aWindow.setTimeout(this.restoreHistory_proxy, 0, tabData, 1, 0, 0);
>+      var doWindowRestore = function(aSessionService) {
>+        aSessionService.restoreHistoryPrecursor(this, [closedTabState], 1, 0, 0);
>+      }
>+      aWindow.setTimeout(doWindowRestore, 0, this);


Any reason for asyncing this (comment on it here if so)?

>     }
>     else {
>-      Components.returnCode = -1; //zeniko: or should we rather fail silently?
>+      Components.returnCode = -1;

Cr.NS_ERROR_INVALID_ARG

        Attachment #223260 -
        Flags: review?(mconnor) → review-

    
    

    
  
(In reply to comment #54)
> >Index: browser/base/content/browser.js

> Why should it be an object (i.e. what is the advantage of scoping here). Note
> we're already have
> addBookmarkMenuitems in the global scope...

This was discussed in #developers. Browser.js is messy and hard to read, and having so many objects and functions in the global scope is confusing and leads to trouble. Object scoping makes things easier to read, and reduces the number of global variables. I think addBookmarkMenuitems should be moved to this new Object (not necessarily as part of this patch).

> >+    // get tab context menu
> >+    var tabbrowser = getBrowser();
> 
> gBrowser would do.

The point of getBrowser() was that it should be used to not rely on the fact that the browser element is stored in a global variable, and to allow changing that in the future without requiring tree-wide changes. It also provides an "unbreakable" extension API that gBrowser was not garanteed to be, but unfortunately some code (including extension code) uses gBrowser directly, so I guess that's kind of a pointless battle to fight now...

    
    

    
  


  
(In reply to comment #54)
> (From update of attachment 223260 [details] [diff] [review] [edit])
> >Index: browser/base/content/browser.js
> >===================================================================
> 
> 
> >   // Initialize the microsummary service by retrieving it, prompting its factory
> >   // to create its singleton, whose constructor initializes the service.
> >   Cc["@mozilla.org/microsummary/service;1"].getService(Ci.nsIMicrosummaryService);
> >+
> >+  // browser-specific tab augmentation
> >+  AugmentTabs.init();
> 
> Why should it be an object (i.e. what is the advantage of scoping here). Note
> we're already have
> addBookmarkMenuitems in the global scope...

Check Gavin's previous comment about this.

> I'm wondering if we should just move the context menu from tabbrowser.xml to
> browser.xul
> (should be straightforward).

Not as part of this patch, right? :)

semi-related discussion of moving all of tabbrowser over:

http://steelgryphon.com/blog/?p=74

> >+
> >+  /**
> >+   * Add undo-close-tab to tab context menu
> >+   */
> >+  addUndoCloseTabContextMenu: function at_addUndoCloseTabContextMenu() {
> 
> If we do keep this scoped, let's name this _addUndoCloseTabContextMenu.

fixed

> >+    // get tab context menu
> >+    var tabbrowser = getBrowser();
> 
> gBrowser would do.

again, note Gavin's comment about this. although, i don't see how the same logic wouldn't apply to gNavigatorBundle, et al...

> >+    var tabMenu = document.getAnonymousElementByAttribute(tabbrowser,"anonid","tabContextMenu");
> >+
> >+    // get menu label
> >+    var menuLabel = gNavigatorBundle.getString("tabContextUndo");
> >+
> >+    // create new menu item
> >+    var undoCloseTabItem = document.createElement("menuitem");
> >+    undoCloseTabItem.setAttribute("label", menuLabel);
> 
> Please add an accesskey.

added

> >+    undoCloseTabItem.setAttribute("oncommand", "AugmentTabs.undoCloseTab();");
> >+
> 
> Might be cleaner to use addEventListener here (I assume that's why it's
> scoped?).

fixed

> >+    // add to tab context menu
> >+    var insertPos = tabMenu.lastChild.previousSibling;
> >+    tabMenu.insertBefore(undoCloseTabItem, insertPos);
> >+  },
> >+
> >+  /**
> >+   * Re-open the most-recently-closed tab
> >+   */
> >+  undoCloseTab: function at_undoCloseTab() {
> >+    // get session-store service
> >+    var ss = Components.classes["@mozilla.org/browser/sessionstore;1"].
> >+                        getService(Components.interfaces.nsISessionStore);
> 
> Use Cc and Ci here (they're defined earlier).

fixed

> >Index: browser/components/sessionstore/nsISessionStore.idl
> >===================================================================
> 
> > 
> > #include "nsISupports.idl"
> > 
> >+interface nsIDOMWindow;
> >+
> > /**
> >  * nsISessionStore keeps track of the current browsing state - i.e.
> >  * tab history, cookies, scroll state, form data, POSTDATA and window features
> >  * - and allows to restore everything into one window.
> >  */
> > 
> > [scriptable, uuid(07fada1e-c3a6-11da-95a7-00e08161165f)]
> 
> rev the uuid please.

done

> > interface nsISessionStore : nsISupports
> > {
> >   /**
> >    * Initialize the service
> >    */
> >   void init();
> >+
> >+	/**
> >+	 * @param aWindow is the window to reopen a closed tab in.
> >+	 * @param aIndex  indicates the window to be restored (FIFO ordered).
> >+	 */
> 
> make that
> >+	/**
> >+	 * @param aWindow
> >+                the window to reopen a closed tab in.
> >+	 * @param aIndex
> >+                indicates the window to be restored (FIFO ordered).
> >+	 */

fixed

> >+	void undoCloseTab(in nsIDOMWindow aWindow, in unsigned long aIndex);
> 
> no hard tabs please.

fixed

> > };
> >Index: browser/components/sessionstore/src/nsSessionStore.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v
> >retrieving revision 1.12
> >diff -u -8 -p -r1.12 nsSessionStore.js
> >--- browser/components/sessionstore/src/nsSessionStore.js	21 May 2006 02:56:20 -0000	1.12
> >+++ browser/components/sessionstore/src/nsSessionStore.js	24 May 2006 22:06:14 -0000
> >@@ -435,17 +435,17 @@ SessionStoreService.prototype = {
> >       _this.onTabAdd(aEvent.currentTarget.ownerDocument.defaultView, aEvent.target);
> >       }, false);
> >     tabpanels.addEventListener("DOMNodeRemoved", function(aEvent) {
> >       _this.onTabRemove(aEvent.currentTarget.ownerDocument.defaultView, aEvent.target);
> >       }, false);
> >     tabpanels.addEventListener("select", function(aEvent) {
> >       _this.onTabSelect(aEvent.currentTarget.ownerDocument.defaultView, aEvent.currentTarget);
> >       }, false);
> >-    tabbrowser.addEventListener("TabClose", function(aEvent) {
> >+    tabbrowser.addEventListener("DOMNodeRemoved", function(aEvent) {
> >       _this.onTabClose(aEvent.currentTarget.ownerDocument.defaultView, aEvent.originalTarget);
> >       }, false);
> 
> Er, a DOMNodeRemoved event on tabbrowser does not mean a tab was closed, that
> is also why you're receiving it
> it for more than one target.
> 
> The patch on https://bugzilla.mozilla.org/show_bug.cgi?id=318168 adds the
> TabClose event.

Yep. But it works as-is, and would be hard to test this patch if it didn't do anything at all. This code can be updated once that patch lands, or this patch can block if it's ok for this feature to land in Beta 1.

> >   undoCloseTab: function sss_undoCloseWindow(aWindow, aIx) {
> >-    var tabs = this._windows[aWindow.__SSi]._closedTabs;
> >+    var closedTabs = this._windows[aWindow.__SSi]._closedTabs;
> >+
> >+    // default to the most-recently closed tab
> >+    aIx = aIx || 0;
> >     
> >-    if (aIx in tabs) {
> >+    if (aIx in closedTabs) {
> >       var browser = aWindow.getBrowser();
> >-      var tabData = tabs.splice(aIx, 1);
> >-      tabData._tab = browser.addTab();
> >+
> >+      // fetch the data of closed tab, while removing it from the array
> >+      var closedTab = closedTabs.splice(aIx, 1).shift();
> >+      var closedTabState = closedTab.state;
> >+
> >+      // create a new tab
> >+      closedTabState._tab = browser.addTab();
> >       
> >       // restore the tab's position
> >-      browser.moveTabTo(tabData._tab, tabData[0].pos);
> >+      browser.moveTabTo(closedTabState._tab, closedTab.pos);
> > 
> >       // restore the tab's state
> >-      aWindow.setTimeout(this.restoreHistory_proxy, 0, tabData, 1, 0, 0);
> >+      var doWindowRestore = function(aSessionService) {
> >+        aSessionService.restoreHistoryPrecursor(this, [closedTabState], 1, 0, 0);
> >+      }
> >+      aWindow.setTimeout(doWindowRestore, 0, this);
> 
> 
> Any reason for asyncing this (comment on it here if so)?

At some point in time there was a reason for it :)

fixed.

> >     }
> >     else {
> >-      Components.returnCode = -1; //zeniko: or should we rather fail silently?
> >+      Components.returnCode = -1;
> 
> Cr.NS_ERROR_INVALID_ARG

fixed

        Attachment #223260 -
        Attachment is obsolete: true

        Attachment #223361 -
        Flags: review?(mconnor)

    
    

    
  
Thanks dietrich,

I'm still wondering though why you can't rely on a more specfic target for DOMNodeRemoved, as things stand you would also get notfied on stuff like the contextmenu item you're adding...

Also note you're already adding a DOMNodeRemoved listener on the tabpannels element, is this broken by now?

    
    

    
  
(In reply to comment #57)
> Thanks dietrich,
> 
> I'm still wondering though why you can't rely on a more specfic target for
> DOMNodeRemoved, as things stand you would also get notfied on stuff like the
> contextmenu item you're adding...

If you can suggest a more specific target, I'd be glad to use it. As far as I knew, this was the way to do it until the new events are added:

http://developer.mozilla.org/en/docs/Extension_Code_Snippets:Tabbed_Browser#Notification_when_a_tab_is_added_or_removed

> Also note you're already adding a DOMNodeRemoved listener on the tabpannels
> element, is this broken by now?

Nothing is broken AFAICT. See that there's a note on the onTabRemove method that says "unify w/ onTabClose once the proper events are available". I *could* do that now, but it doesn't really buy us anything until the new events are available.



    
    

    
  
Point was you're adding it on the tabbrowser (getBrowser()) instead of the panel container, which is the suggested target in the wiki document  *and* in the code above your handler.

    
    

    
  
(In reply to comment #56)
> > Cr.NS_ERROR_INVALID_ARG
> fixed

One issue not yet handled by your patch is what happens when there are no tabs to be restored. I'd expect the menu item to be grayed out. Current behavior is nothing happening except the INVALID_ARG error in the console.

For this to work, you'd probably need a "canUndoCloseTab" or a "undoCloseTabCount" method on nsISessionStore and then update the menu item's disabled attribute whenever a tab is closed or reopened.

Another thing to consider: do we really want all closed tabs to be reopenable, or only tabs actually containing something (i.e. sessionHistory.count > 1 || currentURI.spec != "about:blank")? I'd vote for treating blank tabs specially by ignoring them (since there's no value in saving them anyway).

    
    

    
  
(In reply to comment #60)
> For this to work, you'd probably need a "canUndoCloseTab" or a
> "undoCloseTabCount" method on nsISessionStore and then update the menu item's
> disabled attribute whenever a tab is closed or reopened.

I obviously meant to say that the menu item's disabled state should be updated onpopupshowing.

    
  
Whiteboard: 4d → [swag: 1d]

    
    

    
  
Comment on attachment 223361 [details] [diff] [review]
addresses mano's review comments


>+  /**
>+   * Called in delayedStartup
>+   */
>+  init: function at_init() {
>+    // add the tab context menu for undo-close-tab (bz254021)
>+    this._addUndoCloseTabContextMenu();
>+  },

Init needs to gate on whether system restore is disabled.

>+  /**
>+   * Add undo-close-tab to tab context menu
>+   */
>+  _addUndoCloseTabContextMenu: function at_addUndoCloseTabContextMenu() {
>+    // get tab context menu
>+    var tabbrowser = getBrowser();
>+    var tabMenu = document.getAnonymousElementByAttribute(tabbrowser,"anonid","tabContextMenu");
>+
>+    // get strings 
>+    var menuLabel = gNavigatorBundle.getString("tabContext.undoCloseTab");
>+    var menuAccessKey = gNavigatorBundle.getString("tabContext.undoCloseTabAccessKey");
>+
>+    // create new menu item
>+    var undoCloseTabItem = document.createElement("menuitem");
>+    undoCloseTabItem.setAttribute("label", menuLabel);
>+    undoCloseTabItem.setAttribute("accesskey", menuAccessKey);
>+    undoCloseTabItem.addEventListener("command", this.undoCloseTab, false);
>+
>+    // add to tab context menu
>+    var insertPos = tabMenu.lastChild.previousSibling;
>+    tabMenu.insertBefore(undoCloseTabItem, insertPos);
>+  },

is there a reason we're not doing this in sync with the bookmark menu appending?

Simon's right, if there's no tabs to restore, we should hide this menuitem (not disable it, context menus are supposed to show only relevant items).  Do that as a followup though.

>+  /**
>+   * Re-open the most-recently-closed tab
>+   */
>+  undoCloseTab: function at_undoCloseTab() {
>+    // get session-store service
>+    var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>+               getService(Ci.nsISessionStore);

I do believe the style of record aligns getService with the Cc bit

>+    ss.undoCloseTab(window, 0);
>+  }
>+ };

nit: misaligned bracket

>+      });
>+      this._windows[aWindow.__SSi]._closedTabs.splice(this._getPref("sessionstore.max_tabs_undo", DEFAULT_MAX_TABS_UNDO));

this line seems excessively long, get the pref and then splice, lot easier to read


 
>-  undoCloseTab: function sss_undoCloseWindow(aWindow, aIx) {
>-    var tabs = this._windows[aWindow.__SSi]._closedTabs;
>+  undoCloseTab: function sss_undoCloseTab(aWindow, aIx) {
>+    var closedTabs = this._windows[aWindow.__SSi]._closedTabs;
>+
>+    // default to the most-recently closed tab
>+    aIx = aIx || 0;

just use aIndex please, no bonus points for saving characters here

r=me, with the comments above.

Please file a followup to handle menu hiding/disabling

        Attachment #223361 -
        Flags: review?(mconnor)

        Attachment #223361 -
        Flags: review+

        Attachment #223361 -
        Flags: approval-branch-1.8.1+

    
    

    
  
(In reply to comment #62)
> Simon's right, if there's no tabs to restore, we should hide this menuitem (not
> disable it, context menus are supposed to show only relevant items).  Do that
> as a followup though.

I'd rather go with disabling than hiding in this case: This makes the function better discoverable and ensures muscle memory. IMO context menus should hide menu items only based on context, not on function availability (see e.g. the context menu of an empty <input type="text">).

    
    

    
  
nits are fixed, and this is checked in on trunk. However, caused a regression (342155), which i'm working on now.

(In reply to comment #62)
> (From update of attachment 223361 [details] [diff] [review] [edit]) 
> >+  /** 
> >+   * Add undo-close-tab to tab context menu
> >+   */ 
> >+  _addUndoCloseTabContextMenu: function at_addUndoCloseTabContextMenu() {
> >+    // get tab context menu
> >+    var tabbrowser = getBrowser();
> >+    var tabMenu = document.getAnonymousElementByAttribute(tabbrowser,"anonid","tabContextMenu");
> >+
> >+    // get strings 
> >+    var menuLabel = gNavigatorBundle.getString("tabContext.undoCloseTab");
> >+    var menuAccessKey = gNavigatorBundle.getString("tabContext.undoCloseTabAccessKey");
> >+
> >+    // create new menu item
> >+    var undoCloseTabItem = document.createElement("menuitem");
> >+    undoCloseTabItem.setAttribute("label", menuLabel);
> >+    undoCloseTabItem.setAttribute("accesskey", menuAccessKey);
> >+    undoCloseTabItem.addEventListener("command", this.undoCloseTab, false); 
> >+
> >+    // add to tab context menu
> >+    var insertPos = tabMenu.lastChild.previousSibling;
> >+    tabMenu.insertBefore(undoCloseTabItem, insertPos);
> >+  },
> 
> is there a reason we're not doing this in sync with the bookmark menu
> appending?

This feature is not related to bookmarks, so I thought it quite out of place in the bookmarks code. It would make more sense to group the tab context-menu code here than to put this in the bookmark functions.
 
> Simon's right, if there's no tabs to restore, we should hide this menuitem (not
> disable it, context menus are supposed to show only relevant items).  Do that 
> as a followup though. 

https://bugzilla.mozilla.org/show_bug.cgi?id=342169
Depends on: 342155
Depends on: 342179

    
    

    
  
This should not block bug 322898.
No longer blocks: 322898

    
  
Depends on: 342432

    
  
Depends on: 342700

    
    

    
  
Note: I'm holding off on checking this in on the branch until bug 318168 lands on branch, b/c there are 3 bugs (bug 342155, bug 342179, bug 334839) that are related to this, that depend on the patch in bug 318168.
Depends on: tabbedbrowsing

    
  
Whiteboard: [swag: 1d] → [swag: 1d] 181b1+

    
    

    
  
(In reply to comment #66)
> Note: I'm holding off on checking this in on the branch until bug 318168 lands
> on branch, b/c there are 3 bugs (bug 342155, bug 342179, bug 334839) that are
> related to this, that depend on the patch in bug 318168.
> 
bug 318168 was just checked in, so this should be checked in too.


    
    

    
  
(In reply to comment #66)
> Note: I'm holding off on checking this in on the branch until bug 318168 lands
> on branch, b/c there are 3 bugs (bug 342155, bug 342179, bug 334839) that are
> related to this, that depend on the patch in bug 318168.
> 
bug 318168 was just checked in, so this should be checked in too.


    
    

    
  
ugh. sorry. double comment.

    
    

    
  
Created Bug 343212 'Clear private data...>Browsing History should also clear the 'Undo Close Tab' history'

    
    

    
  
One more thing: when there is no closed tab to undo, the menu item should be disabled otherwise you might confuse the user as to what this menu is doing.

Open a new firefox window, right click on the tab bar and notice the 'undo closed tab' item can be clicked, and does nothing. Someone might conclude it's a broken function...

    
    

    
  
(In reply to comment #71)
> One more thing: when there is no closed tab to undo, the menu item should be
> disabled otherwise you might confuse the user as to what this menu is doing.
> 
> Open a new firefox window, right click on the tab bar and notice the 'undo
> closed tab' item can be clicked, and does nothing. Someone might conclude it's
> a broken function...
> 
Bug 342169 covers this.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago → 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

    
    

    
  
The menuitem currently only has links to its label and to the access key. Normally menuitems are linked either to an id or to a command. Hence

menuitem[oncommand="gBrowser.undoRemoveTab();"] {

would normally allow a themer to skin this menuitem. Leaving only a label means that if you have a different access key (e.g.--U for Underline instead of UndoCloseTab) or if you use a different language for your label, the menuitem goes unskinned.

Please attach the command directly. This is better than attaching an id because then the above css line skins the menuitem wherever it appears in the future, and not just where you put it now. 

This is not academic because ATM you have no provision for this menuitem showing up in the main context menu (i.e.--no "Undo Close Tab" in the content window). So when a user chooses to recover the tabbar space when there is only one tab,  and sets up his browser to show no tabbar when there is only one tab, there are no tabs left to have a context menu when the next-to-last tab is closed. So the user then has no way to recover the lost tab.

Ideally, you will put an "Undo Close Tab" menuitem in the main context window. If that also links directly to the command, then a happy themer will only have to write code once to skin the "Undo Close Tab" menuitem.

    
    

    
  
. . . and what do you know?

When I skinned the menuitem for Undo Close Tab using 

menuitem[accesskey="U"]

it worked - but:

it disabled the skinning of the menuitem for uninstall theme or extension in the Addons page, which also uses the U accesskey.

So it's not academic now.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: