I have no intention of working on those, but if they sit in the closed bug, nobody will, so here's a new bug for them: Bug 580660 comment #13 by Neil on attachment 463197 [details] [diff] [review] v2.2: in-browser UI, updated for comments >+ _updateCommandState: function BATH__updateCommandState(aTabClose) { >+ var numTabs = gBrowser.browsers.length; Nit: gBrowser.tabs.length >+ // The TabClose event is fired before the tab is removed from the DOM >+ if (aTabClose) >+ numTabs--; >+ >+ if (numTabs > 1) >+ this._command.removeAttribute("disabled"); >+ else >+ this._command.setAttribute("disabled", "true"); In theory you only need to check numTabs when closing a tab; after opening a tab I would be very surprised if there was only one! > <!-- Bookmarks Menu --> >+ <key id="addBookmarkKb" key="&addCurPageAsCmd.commandkey;" command="Browser:AddBookmark" modifiers="accel,alt"/> Unfortunately Ctrl+Alt key combos are reserved on Windows. (Also, why was Ctrl+Shift+D moved to bookmark all tabs?)

Not blocking as no reason was given why it should.

(In reply to comment #0) > > <!-- Bookmarks Menu --> > >+ <key id="addBookmarkKb" key="&addCurPageAsCmd.commandkey;" command="Browser:AddBookmark" modifiers="accel,alt"/> > Unfortunately Ctrl+Alt key combos are reserved on Windows. (Also, why was > Ctrl+Shift+D moved to bookmark all tabs?) I think is bad (I hate it when apps block reserved shortcuts!), so taking. I'll just remove the shortcut for Bookmark All Tabs, which had no shortcut before this change but stole the one from Bookmark This Page, as Neil noted. No l10n impact since addCurPageAsCmd.commandkey is used for both actions and I'll only remove one of the two users, so no need to block b3. As KaiRo said, no need to block on this, but I think we should really fix this before final because otherwise people might get used to it and then it'll be hard to justify removing it. Currently it's easy, since it should not have happened in the first place.

Comment on attachment 523059 [details] [diff] [review] patch > // The TabClose event is fired before the tab is removed from the DOM ... >+ numTabs--; > if (numTabs > 1) Could just compare against 2 instead, with the comment possibly expanded? > this._command.removeAttribute("disabled"); Don't actually need to remove the attribute because it won't be set.

So if we're rewriting half of the method, why not all of it? Note: The == 1 case is needed for the init (new window with only one tab), and the parentheses are of course not really needed but I think it's cleaner.

(In reply to comment #4) > Note: The == 1 case is needed for the init (new window with only one tab) Bah, init code strikes again. Can we just disable the command in XUL instead?

(In reply to comment #5) > (In reply to comment #4) > > Note: The == 1 case is needed for the init (new window with only one tab) > Bah, init code strikes again. Can we just disable the command in XUL instead? It already is: <!-- The command is disabled for the hidden window. Otherwise its enabled state is handled by gBookmarkAllTabsHandler. --> <command id="Browser:BookmarkAllTabs" label="&addCurTabsAsCmd.label;" accesskey="&addCurTabsAsCmd.accesskey;" oncommand="gBookmarkAllTabsHandler.doCommand();" disabled="true"/>

So, Jens pointed out that when we close the last-tab "in-place", the menuitem gets enabled with any of these patches. This is because we add a tab to replace the one that just closed, but the added tab dispatches a TabOpen event before we have really closed the tab, so there are now two tabs at this point, so we think we need to enable the "Bookmark this Group of Tabs" menuitem. The solution appears to be to dispatch the TabClose event after the TabOpen event.

Comment on attachment 523315 [details] [diff] [review] Fix tab notifications [Checkin: comment 15] f=me for the result, which is all I checked (review needs to ensure this does not regress anything else).

Comment on attachment 523295 [details] [diff] [review] Possible patch f=me, seems to work as expected (together with "Fix tab notifications"). Note that this does not fully replace my patch; I'll need to merge both.

Comment on attachment 523315 [details] [diff] [review] Fix tab notifications [Checkin: comment 15] FTR: "close the last-tab "in-place"" is closing the last tab with either: a) tab middle click and middlemouse.contentLoadURL = false b) close tab/window and browser.tabs.closeWindowWithLastTab = false c) using the tab bar close button

Comment on attachment 523326 [details] [diff] [review] merged patch (w/o tab notifications fix) [Checkin: comment 13] http://hg.mozilla.org/comm-central/rev/a51304869f50

Leaving open for part 2.

Comment on attachment 523315 [details] [diff] [review] Fix tab notifications [Checkin: comment 15] http://hg.mozilla.org/comm-central/rev/80c1b0c04bd2