Our tab UI doesn't expand well to different types of tabs. Example: 1) Select a message in the 3-pane tab 2) Open a new tab of a non message/folder type, e.g. about:rights, What's new or enter the following in the error console: Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("mail:3pane").document.getElementById("tabmail").openTab("contentTab", "http://www.mozillamessaging.com/", "MoMo"); 3) Dig around in the menus, notice that most options are enabled because you've got a message selected in the 3-pane tab, and items like View -> Zoom -> +/- apply to the message window not the tab you've just opened. This is going to make it very difficult for extensions to open different tabs and have a way to disable menu options they don't use or recycle menu options they want to use. Proposal -------- In mail3PaneWindowCommands.js we currently have two controllers - DefaultController - FolderPaneController These both have the functions: - supportsCommand - isCommandEnabled - doCommand - onEvent The default controller is registered using top.controllers.insertControllerAt(0, DefaultController); I think there are two ways we can do this: 1) Require different tab modes to define their own controllers, tabmail would then un-hook one controller and hook in another when switching between tabs. 2) Use just the DefaultController, but route the function calls through the tabmode. I'm not sure if option 1 could have the context of which tab is displayed passed to the controller, so option 2 may be better if we need that. In any case if a tab mode didn't want to enable any options it could just do a basic controller: var tabController = { supportsMode: function() { return false; }, isCommandEnabled: function() { return false; }, doCommand: function() {}, onEvent: function() {} }; This would effectively disable all menu options that pass through the controller. What we would also need to make sure is items such as those on the tools, window and help menus that apply to all tabs don't pass through the controller and hence are always enabled. Thoughts?

I see 2) as an elegant hack on the way to 1). I'd thought of 1), and got stuck on how to do that in 1 patch w/o a huge test suite. Me likey. (to be clear: I think long term we need to move most of the code in mail3PaneWindowCommands.js to controllers that clearly not "window" focused -- also 'mail3pane' isn't a window-centric concept anymore.)

I completely agree with comment 1. We definitely want to split apart the DefaultController into smaller, more widget-specific controllers. I may end up needing to do some of this work as part of the compact header extraction shortly; we'll see.

This is exactly the issue I've stumbled upon in bug 456385, which has been keeping calendar away from beta1 for quite some time. Menu entries are sometimes not only tab dependent, but also things-in-tab dependent (i.e the view menu options). We need to think about a good way to solve this in general. Some menu items might make sense (from a user POV) for the current tab, but not be implemented in the current tab. For example, the zoom menu items. The user may think its intuitive that zooming should be possible in the calendar views, but actually this is a feature of a different tab. Since the menuitems are just disabled, the user will wonder why he can't do this. If we hide the menuitems on the other hand, he might wonder why those menuitems went away. I see no good way to solve this dilemma. Tab-specific command controllers would definitely be a good start though. I would be delighted if this bug could enjoy very high priority, since it would make solving bug 456385 a lot easier and would avoid double implementation work.

This is a work in progress implementation of my option 2 - the tab definitions will need to implement isCommandEnabled, supportsCommand etc. I went with option 2 as we then can pass the tab element in. I don't know if anyone will ever need it, but the option is there. The implementation of the functions is optional - if they aren't implemented, we'll just disable any commands we are asking tabs about. The patch "works". A good example is to open up a new tab - either using about:rights, what's new or entering this in the error console: Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("mail:3pane").document.getElementById("tabmail").openTab("contentTab", "about:config", "about:config"); Then go back to the 3 pane tab, select a message, see that most of the message menu is enabled, then switch to the new tab and see that most of it is disabled. There are various commands that aren't disabled - I'm assuming these are not routed through the window controllers at the moment, I think these could be dealt with in follow-up patches. There are also various commands (like zoom) that don't get routed through the controllers, but would be useful if they were. I think this patch is almost usable though if we're prepared to sort out the other commands as we go. The only bits I know are broken/not working are: - standalone message window (doesn't close, easy fix) - toolbar buttons don't get disabled/enabled at the right times (not consistently anyway).

I spun the close command issue out to bug 495242 and posted a patch there. This is a reduced patch. Still need to look at the toolbar issue.

This version fixes updating the toolbar, except in one case: - Select a folder in the 3 pane message tab (but without a mail selected), switch to a tab that should have delete disabled (e.g. a content tab). Delete button doesn't get enabled until the focus goes into that tab. At this stage I'm not too fussed about that or the fact that not all the relevant menu items are disabled (or work) on non-message type tabs - we can deal with those in follow-up bugs. What this does give us is a good framework for fixing up our menus on non-message type tabs which is what I think we really need to start with.

Comment on attachment 380218 [details] [diff] [review] The fix Looks reasonable, r=mkmelin

Checked in: http://hg.mozilla.org/comm-central/rev/b7f5a15303ff I'll raise some bugs before marking this fixed though.

I've raised bug 495815 and bug 495818 on fixing up the menus further.