533125 - Implement browser.tabs.closeWindowWithLastTab functionality on Seamonkey to avoid closing browser with last tab

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, enhancement)

Description

[Marcelo Bastos]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4) Gecko/20091017 SeaMonkey/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4) Gecko/20091017 SeaMonkey/2.0

In Seamonkey, if you Ctrl-W the last tab, the whole window closes. Furthermore, if you have no other Seamonkey windows opened, the whole application shuts down -- that is, a moment's mistake may need almost half a minute to recover. This is certainly by design, because the "file" menu changes accordingly.
However, this doesn't happen if you close tabs using the mouse (if you
disable the "hide tabbar when there is only one tab showing" feature, of
course) -- it just clears the tab.
Firefox has the browser.tabs.closeWindowWithLastTab preference which, when set to "false," makes the Ctrl-W shortcut behave like when closing tabs with the mouse. I think a similar option in Seamonkey would be useful.


Reproducible: Always

Steps to Reproduce:
1.Set preference "hide tabbar when there is only one tab showing" to "false"
2.Open several tabs.
2.Click on the "close tab" button (or middle-click the tab) repeatedly: the last tab doesn't close, only clears.
3.Open several tabs again.
3.Press Ctrl-W repeatedly: the last tab takes the browser window with it.
Actual Results:  
Browser window closes.

Expected Results:  
Browser remains open with a blank (or "home," depending on your settings) page.

Although it's probably years too late to change those key bindings now, I think it was a mistaken decision to use the same command to close tab and close window. Intensive tab users will close several tabs in quick sequence, but they rarely will wish to close the main window. Changing the binding now would break consistency with Firefox -- while implementing the same preference Firefox already has would keep consistency.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Something like this can hardly be implemented on the 2.0 branch, changing to Trunk and confirming valid enhancement request. On a personal note, I'd like to have this, too. :-)

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Checked with both Ctrl+W and Ctrl+F4: If the pref is false the last tab is replaced with an empty one and the window stays open.

Comment 3

[[e-mail]]

Comment on attachment 417352 [details] [diff] [review]
patch

* Don't close the tab if you're closing the window
* Need to update the menu to show that Ctrl+W only closes the tab
[Doesn't Ctrl+F4 already only always close the tab anyway?]

Comment 4

[Philip Chee]

How about:

  var browser = getBrowser();
  if (browser.tabContainer.childNodes.length > 1 ||
      !pref.getBoolPref("browser.tabs.closeWindowWithLastTab")) {
    // Just close up a tab.
    browser.removeCurrentTab();
    return;
  }

  BrowserCloseWindow();

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

Addressed nits, incorporating Philip's suggestion.

Comment 6

[[e-mail]]

Comment on attachment 417361 [details] [diff] [review]
patch v2

There's still a problem here; the menu has three states: no tabstrip, single tab in tabstrip and multiple tabs.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3

Added handling for the case where a single tab hides the tab bar, including actually doing that.

Comment 8

[[e-mail]]

Comment on attachment 417408 [details] [diff] [review]
patch v3

>+  const keepWindowWithLastTab = !pref.getBoolPref("browser.tabs.closeWindowWithLastTab");
Prefs can vary ;-) [I know const really means the same as Java's final, but it's confusing to use it like that.]

>     browser.removeCurrentTab();
>+    if (keepWindowWithLastTab && pref.getBoolPref("browser.tabs.autoHide"))
>+      browser.setStripVisibilityTo(false);
Doesn't removeCurrentTab get this right already (in particular, when there are several tabs open?)

Comment 9

[[e-mail]]

Comment on attachment 417408 [details] [diff] [review]
patch v3

>+  const keepWindowWithLastTab = !pref.getBoolPref("browser.tabs.closeWindowWithLastTab");
>   if (browser && browser.getStripVisibility()) {
>     document.getElementById('menu_closeOtherTabs').hidden = false;
>-    if (browser.tabContainer.childNodes.length > 1) {
>+    if (browser.tabContainer.childNodes.length > 1 || keepWindowWithLastTab) {
So, it would seem that you are dealing with two sorts of behaviour:
1. Should menu_closeOtherTabs be hidden or visible
2. Should menu_closeWindow be hidden or visible (and update menu_close label)
Rather than this now extensive if/else collection, you should be able to reduce duplication by separating out the two behaviours.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v4

(In reply to comment #8)
> >     browser.removeCurrentTab();
> >+    if (keepWindowWithLastTab && pref.getBoolPref("browser.tabs.autoHide"))
> >+      browser.setStripVisibilityTo(false);
> Doesn't removeCurrentTab get this right already (in particular, when there are
> several tabs open?)

For multiple tabs removeTab does this, yes, but not for the new single tab case. Since I found that my way of doing things was wrong (it also hid the tab strip for >2 tabs) and having the tab strip hiding logic in multiple places smells like bad style I moved it to removeTab. Note that I intentionally didn't merge it with the l==2 case since I think it's better understandable this way (I didn't want to mess with the "else if" that is vital to distinguish that case from the l==1 one with l++ inside) -> KISS.

(In reply to comment #9)
> So, it would seem that you are dealing with two sorts of behaviour:
> 1. Should menu_closeOtherTabs be hidden or visible
> 2. Should menu_closeWindow be hidden or visible (and update menu_close label)
> Rather than this now extensive if/else collection, you should be able to reduce
> duplication by separating out the two behaviours.

Sure, but don't blame me for making the review harder! ;-) I tried to add some sanity and consistency instead of minimizing line or Boolean logic changes.

Comment 11

[[e-mail]]

Comment on attachment 419949 [details] [diff] [review]
patch v4

>             if (l == 1) {
>               // add a new blank tab to replace the one we're about to close
>               // (this ensures that the remaining tab is as good as new)
>               this.addTab("about:blank");
>               l++;
>+              if (this.mPrefs.getBoolPref("browser.tabs.autoHide") &&
>+                  !this.mPrefs.getBoolPref("browser.tabs.closeWindowWithLastTab"))
>+                this.mStrip.collapsed = true;
Ah, I see what the issue is now: the tab strip may have been collapsed, and adding the tab will uncollapse it, in which case you need to collapse it again. I don't think you need to check for browser.tabs.closeWindowWithLastTab because if it is set then nobody will try to close a tab (except via the tabstrip close button which requires the strip to have been visible in the first place). This means that you can split the if/else if into two separate if statements, or even change it into a switch.

Comment 12

[[e-mail]]

Comment on attachment 419949 [details] [diff] [review]
patch v4

>+  if (!hideCloseOtherTabs) {
>+    if (hideCloseWindow)
>+      document.getElementById("cmd_closeOtherTabs").setAttribute("disabled", "true");
>+    else
>+      document.getElementById("cmd_closeOtherTabs").removeAttribute("disabled");
>+  }
>+
>   var recentTabsItem = document.getElementById("menu_recentTabs");
>   recentTabsItem.setAttribute("disabled", !browser || browser.getUndoList().length == 0);
>   var recentWindowsItem = document.getElementById("menu_recentWindows");
>   recentWindowsItem.setAttribute("disabled", ss.getClosedWindowCount() == 0);
Oh, and you can reuse the setAttribute("disabled", <boolean>) trick. (Fortunately false == enabled in the case of menuitems.)

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v4a [Checkin: comment 14]

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

http://hg.mozilla.org/comm-central/rev/82a29d7dead1