554300 - "close tab" context menu item should be enabled for last tab when browser.tabs.closeWindowWithLastTab=false

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[eyal gruss]

now that Bug 501714 is fixed, the "close tab" context menu item should be enabled for the last tab when browser.tabs.closeWindowWithLastTab=false

Comment 1

[Michael Kohler [:mkohler]]

Attachment: Patch v1

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 456123 [details] [diff] [review]
Patch v1

For the item to always be enabled when it should, I think you need to remove tbattr="tabbrowser-multiple" from context_closeTab in browser.xul and use this:

> document.getElementById("context_closeTab").disabled = disabled && closeWithLastTab;

instead of:

>+    if (!closeWithLastTab && disabled)
>+      document.getElementById("context_closeTab").removeAttribute("tbattr");
>+    else
>+      document.getElementById("context_closeTab").setAttribute("tbattr", "tabbrowser-multiple");

Comment 3

[Michael Kohler [:mkohler]]

Attachment: Patch v2

(In reply to comment #2)
> (From update of attachment 456123 [details] [diff] [review])
> For the item to always be enabled when it should, I think you need to remove
> tbattr="tabbrowser-multiple" from context_closeTab in browser.xul and use this:
> 
> > document.getElementById("context_closeTab").disabled = disabled && closeWithLastTab;

This is simpler, yes.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 456153 [details] [diff] [review]
Patch v2

>+    // Enable the "Close Tab" menuitem when the window doesn't close with the last tab.
>+    var closeWithLastTab = gPrefService.getBoolPref("browser.tabs.closeWindowWithLastTab");
>+    document.getElementById("context_closeTab").disabled = disabled && closeWithLastTab;

Could also be done without accessing the actual pref:

document.getElementById("context_closeTab").disabled =
  disabled && gBrowser.tabContainer._closeWindowWithLastTab;

Comment 5

[Michael Kohler [:mkohler]]

Attachment: Patch v3

Nit addressed.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 456859 [details] [diff] [review]
Patch v3

I don't see a difference between this and v2.

Comment 7

[Michael Kohler [:mkohler]]

Attachment: Patch v3.1 (now correct)

sorry, I attached the wrong patch. Now it should be the correct one.

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/9632cff74e11

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Michael, could you also write an automated test for it?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre

Comment 11

[Michael Kohler [:mkohler]]

Attachment: Test v1

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 458768 [details] [diff] [review]
Test v1

>+function test() {
>+  // Open a tab and see wether the "Close Tab" menuitem is enabled
>+  // if browser.tabs.closeWindowWithLastTab is set to false and disabled
>+  // if it is set to true.
>+  var tab = gBrowser.addTab();

This is going to be the second tab in that window, which makes the test not test what it intends to test...

>+  var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService);

Services.prefs

>+  prefs.setBoolPref("browser.tabs.closeWindowWithLastTab", false);
>+  EventUtils.synthesizeMouse(gBrowser.tabs[0], 1, 1, { type : "contextmenu", button: 0 });
>+  ok(!document.getElementById("context_closeTab").disabled, "'Close Tab' menuitem is enabled.");
>+
>+  prefs.setBoolPref("browser.tabs.closeWindowWithLastTab", true);

You should reset the pref to its original value, even if that happened to be false.