+++ This bug was initially created as a clone of Bug #428216 +++ Splitting off the discussion about the UI design of the toolbar context menu since stafanh has concerns about the current configuration.

Don't we already have bug 45532 about those context menus?

Well, bug 428216 will somehow fix bug 45532 since it adds a show/hide option for both toolbars ("context menu for toolbars" - note the plural).

Having the overall toolbar config in the context of each single toolbar doesn't make sense. The context menu should just show the context options for _this_ toolbar.

Is there any way we can get rid of those checkbox menuitems? I find them really counter-intuitive.

> Is there any way we can get rid of those checkbox menuitems? I find them really > counter-intuitive. Which checkbox menu items? The Toolbar toggle items? That's copied from Firefox. I added those for feature parity.

> Which checkbox menu items? The Toolbar toggle items? That's copied from > Firefox. I added those for feature parity. No, I ment the menuitems in the sub-menu. But I see that it'll be difficult to do it in some other way.

I'm wondering if this is an issue: The "Use default settings" menuitem is disabled. But the "Icons and text" menuitem is not. I don't know what is correct, since the usage of checkbox menuitems is very rare on mac (that's probably why I think they're counter-intuitive - I never know if an unchecked menuitem can be checked or not), but shouldn't both have the same state here?

You could say that it's duplicate UI, because you can only use default settings if the settings are not default, so unchecking makes no sense, which is why it's disabled, which means that we don't need to check it because the fact that it is disabled could be used to infer that the settings are default, although offhand I'm not sure how easy it is to make that inference.

Technically there should be another separator between "Use small icons" and "Show text beside icon" to show that there are actually three groups. So changing any of the {icon size OR the label position OR the button mode} from their default setting will enable the "Use default settings" item. Selecting this latter will then reset all three groups to default. The other UI I looked at was that the button mode items would be a submenu off another menu item, the latter which would be co-equal to the icon size an the label position menu items; However this would mean a two level deep menu structure and I read somewhere that this is discouraged.

I'm not sure we need that "[x] Use default settings" at all. How about [x] Icons [ ] Icons and Text (Default) [ ] Text where the "(Default)" extension would depend upon the pref setting? OTOH, if we want to support a setting where the toolbar mode is set to the same value as the default, but should not change when changing the default, then we should have a radio group with four members: [x] Icons [ ] Icons and Text [ ] Text [ ] Default We probably could label the last item depending of what the current default is, but I think that "Default (Icons)" would just provoke "huh? 'Icons' is twice?".

The default settings menuitem is also a bit cryptic, because it assumes that you know what the default is.

> The default settings menuitem is also a bit cryptic, because it assumes that > you know what the default is. Is it? I'd imagine using that setting as "I don't care for special settings, just use the default". Okay, if telling the default is probably better, than we probably need to make sure it's understood as not being fixed: [ ] Current Default (Icons)

(In reply to Comment 10) > value as the default, but should not change when changing the default, then we > should have a radio group with four members: But the "default settings" also refers to the icon size mode, and to the label align mode, so putting it as a radio item in the button mode radio group makes no sense. (In reply to Comment 11) > The default settings menuitem is also a bit cryptic, because it assumes that > you know what the default is. Well: 1. The default is the settings the first time you open that particular window without any customization. 2. As Karsten says "I don't care for special settings, just use the default". OR one use case I anticipate (from using Firefox) "I messed up, lets start again from the defaults whatever they are".

> But the "default settings" also refers to the icon size mode, and to the label > align mode, so putting it as a radio item in the button mode radio group makes > no sense. Ah, okay, true. I retract my submenu layout proposals then. But I still think that the context menu's submenu should be the main menu and the current main menu with the global toolbar config should go away.

Maybe calling it "Reset to default settings" would make its purpose clearer?

IMHO, we should remove the show/hide items from the toplevel, those feel out of place. What I'd like to see instead is "hide <this toolbar>" and "collapse <this toolbar>". The submenu could either be moved to the toplevel or renamed to "Icon/Text settings"

"Hide" or "Hide toolbar" sounds reasonable. The context is the toolbar, so it would be understandable. I also think that the sub-menu should be removed to the top-level.

IMHO, if it should be anything more than the mode/icon stuff, "Hide toolbar" is enough. That said, with the mode/icons choices you already have 6 items in the menu and I wonder if this isn't enough.

I disagree with the "Hide toolbar" menu item. If the show/hide items are removed then there should NOT be a "Hide" toolbar item as well because if you hide a toolbar with a toolbar specific context menu, you can't unhide it from that context menu either. You could use the View->Show/Hide menu to unhide the toolbar but this would be asymmetric.

I'm fine with just the mode/icon stuff :-)

OK, then we'll need to reopen bug 45532, see esp. its comment #0.

(In reply to comment #21) > OK, then we'll need to reopen bug 45532, see esp. its comment #0. Yeah, probably.

Can we address the issues that this bug was filed for?

I am thinking of what to do with show/hide when users can create custom toolbars. For Firefox compatibility (where these toolbar can be shown/hidden) where can I put these context menu items.

IMHO, View > Show/Hide should contain all toolbars, including user-generated ones and so should be where to access them. I Show/Hide submenu to the toolbar context menu that is identical to the one in the view menu would be something I could agree with, but it again breaks the actual context somewhat.

Firefox has two places in the UI where they show a dynamic show/hide toolbars menu: 1. The toolbar context menu. 2. In View->Toolbars The Suite has something similar to [2] in View->Show/Hide, except that all the menu items are static. This patch removes the dynamic list of toolbars from the toolbar context menu and moves it to View->Show/Hide. Two of the static items (Navigation Toolbar and Personal Toolbar) are removed since they are now generated dynamically. Since the main Messenger window also has a View->Show/Hide menu I can leverage on this patch when it comes to turning on toolbar customization for MailNews.

Comment on attachment 367782 [details] [diff] [review] [obsolete] Patch v1.0 (Draft for comments) >+ <menu label="&linkToolbar.label;" >+ insertbefore="menuitem_showhide_tabbar" > accesskey="&linkToolbar.accesskey;" The accesskey should follow the label directly. r=me with that.

Do we need the sub-menu?

> Do we need the sub-menu? No. I'll just collapse it all into one level then.

There's another thing. We now have: 1) One set of menuitems dealing with icon/text/default mode in the toolbar the user have right-clicked on. 2) One menuitem that opens the customize toolbar sheet/dialog. And that deals with all toolbars in the window. I think it's worth discussing if this is optional. I do understand that having the menuitem in 2) makes the feature more discoverable, but wouldn't you as a user be surprised that the menuitem in 2) brings up a dialog that deals with all toolbars (and you can actually set icon/text modes for all toolbars in that dialog)? I'd like more people to comment here, especially Karsten - this is basically comment #3, except that the menuitem in 2) wasn't there when this bug was filed.

Maybe if we renamed the menuitem "Customise Toolbars..." ?

I think the "Customise Toolbar(s)..." menuitem should definitely be in the View menu (maybe just below the show/hide menu). But should it be in the context menu as well?

> I think the "Customise Toolbar(s)..." menuitem should definitely be in the View > menu (maybe just below the show/hide menu). But should it be in the context > menu as well? Firefox and Thunderbird has a Customize... menu item in the View menu. Firefox, Thunderbird, Sunbird (and I suspect most other toolkit apps have copied Firefox in this respect) all have a Customize... menu item in the toolbar context menu. For better or worse there are now all over the intertubes in knowledgebases and forum threads the advice to "Right click on any toolbar to get the Customize menu". I think we should follow suit to be consistent. At the very least, it would make it easier to persuade Firefox/Thunderbird users to move to SeaMonkey.

Interestingly, Safari has a slightly similar problem - they have "Remove Item", "Keep Item Visible" combined with "Customize Toolbar..." (cocoa apps doesn't have multiple toolbars) in the context menu. (In reply to comment #33) > I think we should follow suit to be consistent. At the very least, it > would make it easier to persuade Firefox/Thunderbird users to move to > SeaMonkey. Consistent in that we have this specific menuitem in the same place as the others, yes. Consistent in terms of UI, I'd say no, because the context menu should only deal with one context (in this case, either the "target" toolbar or the global-window toolbar(s)). I guess we could summarize this to: Should we have a context menu with a 100% consistent UI or should we sacrifice that a bit because the feature would be easier to discover (because all other apps have the menuitem there)?

> Should we have a context menu with a 100% consistent UI or should we sacrifice > that a bit because the feature would be easier to discover (because all other > apps have the menuitem there)? Well I would personally go for the latter, but this is a UI Policy question so over to Neil & KaiRo.

(In reply to comment #32) > ... the "Customise Toolbar(s)..." menuitem ... Should definitely be Toolbars plural, so that it's more obvious that it applies to all the toolbars, and not the one being right-clicked.

(In reply to comment #34) > because all other apps "All" meaning about "three", where two just copied the first without thinking? ;-)

(In reply to comment #37) > (In reply to comment #34) > > because all other apps > > "All" meaning about "three", where two just copied the first without thinking? > ;-) Well, FF/TB doesn't have any problem with the contextual meaning - their context is the global-window toolbar(s).

> Well, FF/TB doesn't have any problem with the contextual meaning - their > context is the global-window toolbar(s). You do know that in Firefox you can create additional toolbars via the customize toolbar window, right? And recently Thunderbird added that when they unforked their customize code. So "Toolbars" is correct.

(In reply to comment #39) > > Well, FF/TB doesn't have any problem with the contextual meaning - their > > context is the global-window toolbar(s). > > You do know that in Firefox you can create additional toolbars via the > customize toolbar window, right? And recently Thunderbird added that when they > unforked their customize code. So "Toolbars" is correct. Yes. My point was just that their menuitems all have the same context.

"Customize Toolbars..." sounds good for me, and I don't really see a context clash, as all those are relevant to the context of the place where the right-click was made, where "context" doesn't strictly have to be a single layer of context, IMHO. I still would love to have "Hide $toolbarname" and maybe "Collapse $toolbarname" (with a "Open $toolbarname" context menu item on the collapsed grippy) in the context menu. I don't see why the asychronousness of the would hurt, as we then would need to remove "Delete Folder" or "Delete Message" from mailnews as well, right? And for what reason do we need "toolbarmode-context-menu" to be a separate foldout and not part of the main context menu?

(In reply to comment #41) > I don't see why the asychronousness of the would > hurt, as we then would need to remove "Delete Folder" or "Delete Message" from > mailnews as well, right? Having both "Delete Folder" and "Delete Message" in the same context menu sounds confusing. That would be one reason to really think/discuss the UI before it's implemented ;-) (once it's there it's hard to remove) > And for what reason do we need "toolbarmode-context-menu" to be a separate > foldout and not part of the main context menu? We don't (see comment #29). The down-side with it (when combined with "Customize Toolbars...") is that we don't explicitly tell the user that those menuitems only deals with the right-clicked toolbar.

I don't think that calling the menu item "Customize Toolbars..." (plural) is a good idea. Although it's correct technically (you _can_ create new toolbars there, you _can_ alter other toolbars from there), it strongly hurts the "context" meme. The context of a right click is "the" toolbar - the user expects to get a context menu for _this_ toolbar. And in fact he does get that - and more, but that's NOT the right place to tell him. Thus, the menuitem should just be labelled "Customize...". OTOH, we should have a normal menuitem, probably in the Show/Hide menu, named "Customize Toolbars...", which would basically just open the very same toolbar customization dialog.

Comment on attachment 367782 [details] [diff] [review] [obsolete] Patch v1.0 (Draft for comments) Karsten's comment convinced me that if we really need a context menuitem for bringing up the customize sheet/dialog we should name it "Customize...". And also agree that we should have a menuitem in the View menu named "Customize Toolbars...". (and no sub-menu in the context menu). We also need a bug filed to remove the icon/text mode options in the customize toolbar sheet/dialog.

I forgot to mention that I get a "phantom" menuitem in MailNews that doesn't do anything and disappears when I expand the sub-menu (but since this a step towards the mailNews implementation of customize toolbars it might be something that is known/expected?).

> OK. So: 1. No submenu in the context menu. 2. context menu item label is "Customize..." 3. View menu label is "Customize Toolbars..." > We also need a bug filed > to remove the icon/text mode options in the customize toolbar sheet/dialog. This is toolkit code. I don't think the Firefox/toolkit peers will agree. > I forgot to mention that I get a "phantom" menuitem in MailNews that doesn't do > anything and disappears when I expand the sub-menu Ick. I forgot that this code in utilityOverlay.js is used by other windows. I need to make sure my refactoring doesn't affect windows that are not ready for this.

(In reply to comment #46) > > We also need a bug filed > > to remove the icon/text mode options in the customize toolbar sheet/dialog. > This is toolkit code. I don't think the Firefox/toolkit peers will agree. Yeah, I'm quite convinced that they won't agree. Is there anything we could do in /suite? An overlay or something?

A stylesheet overlay should work.

If we even want that change - and it's a different issue than this bug in any case, so let's move it to its own discussion/bug, please.

I'm going to morph this into a polish and update bug.

Bug 464653 Toolbar context menu improvements and fixes. Referenced bits from Firefox: [Bug 595937] Need support for customizing toolbars which are outside of the toolbox. [Bug 598934] Addon Bar should be bottom of Toolbar contextual menu, currently it's top. [Bug 570795] Create basic "Firefox button" widget. [Bug 574688] replace the status bar with the addon bar. [Bug 583386] Implement latest Firefox Menu design. > +++ b/suite/common/utilityOverlay.js > -function onViewToolbarsPopupShowing(aEvent) > +function onViewToolbarsPopupShowing(aEvent, aInsertPoint) > - var firstMenuItem = popup.firstChild; > + var firstMenuItem = aInsertPoint || popup.firstChild; Firefox uses aInsertPoint to support the toolbar menu when embedded it their new App Button menu. > var popup = aEvent.target; > + if (popup != aEvent.currentTarget) > + return; Some submenu e.g. The link toolbar menu is bubbling up. I added a stopPropagation there but extensions could also add submenus to View->Show/Hide as well. > - var toolbar = document.popupNode; > + var toolbar = document.popupNode || popup; We might be called from View->Show/Hide. > while (toolbar.localName != "toolbar") > toolbar = toolbar.parentNode; > - var toolbox = toolbar.parentNode; > + var toolbox = toolbar.toolbox; Gecko 2.0 toolbars now have a r/w toolbox property to support external toolbars so we'll just take advantage of this here. > - var toolbars = toolbox.getElementsByAttribute("toolbarname", "*"); > - for (let i = 0; i < toolbars.length; ++i) { > - let bar = toolbars[i]; > + var toolbars = Array.slice(toolbox.getElementsByAttribute("toolbarname", "*")); > + toolbars = toolbars.concat(toolbox.externalToolbars); External toolbars automatically added to the View->Show/Hide menu like normal toolbars inside the toolbox (externalToolbars also from Gecko 2.0). > + toolbars.forEach(function(bar) { > let menuItem = document.createElement("menuitem"); > + menuItem.setAttribute("id", "toggle_" + bar.id); Firefox App Menu uses "toggle_foobar". > + menuItem.setAttribute("class", "menuitem-iconic"); Only Navigator Show/Hide uses this class. Show/Hide in all our other windows don't use this. Bogus class? > function onViewToolbarCommand(aEvent) > + if (toolbar) Skip if not one of our menu items. > + goToggleToolbar(toolbar); > <toolbar class="chromeclass-toolbar toolbar-primary" > - togglemenuitem="menu_showAbToolbar" Now taken care of by the onViewToolbarsShowing code. > +++ b/suite/mailnews/compose/MsgComposeCommands.js > - case "cmd_showComposeToolbar" : goToggleToolbar('composeToolbar', 'menu_showComposeToolbar'); break; Now taken care of by the onViewToolbarsShowing code. > +++ b/suite/mailnews/mail3PaneWindowCommands.js > case "cmd_expandAllThreads": > case "cmd_collapseAllThreads": > - return (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay); > + return gDBView && (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay); Annoying message spamming the Error Console every time I test a MailNews patch. Error: An error occurred updating the cmd_collapseAllThreads command: TypeError: gDBView is null Source file: chrome://global/content/globalOverlay.js Line: 86 jminta fixed this in Thunderbird in one of his clean-up megapatches. http://hg.mozilla.org/comm-central/rev/5748fe6d63df Bug 462681: mailWindowOverlay.js style/whitespace/indention/comment/linewrap cleanup

Use this to test the toolbar show/hide menu.

(In reply to comment #51) > > var popup = aEvent.target; > > + if (popup != aEvent.currentTarget) > > + return; > Some submenu e.g. The link toolbar menu is bubbling up. I added a > stopPropagation there but extensions could also add submenus to > View->Show/Hide as well. Is it worth having both? > > + menuItem.setAttribute("class", "menuitem-iconic"); > Only Navigator Show/Hide uses this class. Show/Hide in all our other windows > don't use this. Bogus class? Probably. It's remotely possible that one of the themes needed the class because type="checkbox" didn't have the right binding back then.

> > View->Show/Hide as well. > Is it worth having both? No rather. I'll take the stopPropagation out of the link toolbar popup in the next iteration. > > > + menuItem.setAttribute("class", "menuitem-iconic"); > > Only Navigator Show/Hide uses this class. Show/Hide in all our other windows > > don't use this. Bogus class? > Probably. It's remotely possible that one of the themes needed the class > because type="checkbox" didn't have the right binding back then. OK, I'll take it out and wait for someone to complain.

The "menuitem-iconic" dates back to 2000 at least (I gave up at that point :P ). http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/browser/resources/content/navigatorOverlay.xul&rev=1.141#131

Comment on attachment 512113 [details] [diff] [review] Patch Sv0.1 WIP: Make the MailNews searchBar an external toolbar in the thread pane I can still get the previous effect by creating a custom toolbar and dragging all the elements there and hiding the search bar, right? > if (!threadPaneVisible) >- gDisableViewsSearch.setAttribute("disabled", true); >+ GetDisableViewsSearch().setAttribute("disabled", true); > else >- gDisableViewsSearch.removeAttribute("disabled"); >+ GetDisableViewsSearch().removeAttribute("disabled"); Why this change?

> neil@parkwaycc.co.uk 2011-02-15 08:39:50 PST > > Comment on attachment 512113 [details] [diff] [review] > Patch Sv0.1 WIP: Make the MailNews searchBar an external toolbar in the thread > pane > > I can still get the previous effect by creating a custom toolbar and dragging > all the elements there and hiding the search bar, right? Yes. Toolkit support for external toolbars means that they are now included in the Customize Toolbar window and you can drag and drop between external and internal toolbars; and with the customizable window. Note: This is just something I threw together quickly to test that the show/hide menu was picking up external toolbars. I'll eventually move this into a separate bug with tidier code. > > if (!threadPaneVisible) > >- gDisableViewsSearch.setAttribute("disabled", true); > >+ GetDisableViewsSearch().setAttribute("disabled", true); > > else > >- gDisableViewsSearch.removeAttribute("disabled"); > >+ GetDisableViewsSearch().removeAttribute("disabled"); > Why this change? This is just a temporary kludge. I wasn't sure where would be a suitable place to initialize gDisableViewsSearch. A side effect of moving the toolbar into the messagepane is to make the tabmail constructor fire earlier, before Create3PaneGlobals() runs from the onLoad handler. I tried setting |gDisableViewsSearch = document.getElementById("mailDisableViewsSearch");| at the top of the file but that was too early as the overlay containing the search widget hadn't loaded yet. Any advice?

(In reply to comment #57) > A side effect of moving the toolbar into the messagepane is to make the tabmail > constructor fire earlier, before Create3PaneGlobals() runs from the onLoad > handler. I tried setting |gDisableViewsSearch = > document.getElementById("mailDisableViewsSearch");| at the top of the file but > that was too early as the overlay containing the search widget hadn't loaded > yet. Any advice? I can't see how the tabmail constructor relates to mailDisableViewsSearch...

> I can't see how the tabmail constructor relates to mailDisableViewsSearch... Well OK, but I'm pretty sure that the |gDisableViewsSearch is null| aren't just a figment of my imagination. Any hints? By the way Neil, please feel free to steal the review of the main patch from IanN.

Comment on attachment 512111 [details] [diff] [review] Patch v2.0 Polish and Tidy. ># HG changeset patch ># User Philip Chee <philip.chee@gmail.com> ># Date 1297670327 -28800 ># Node ID ac2810e68c048b78c6c4a214abf5c951a004cad8 ># Parent d6059bdc705312f4374638f91745fc27f93866a5 >Bug 464653 Toolbar context menu improvements and fixes. > >diff --git a/suite/browser/linkToolbarOverlay.xul b/suite/browser/linkToolbarOverlay.xul >--- a/suite/browser/linkToolbarOverlay.xul >+++ b/suite/browser/linkToolbarOverlay.xul >@@ -60,20 +60,21 @@ > </script> > > <stringbundleset> > <stringbundle id="languageBundle" > src="chrome://global/locale/languageNames.properties"/> > </stringbundleset> > > <menupopup id="view_toolbars_popup"> >- <menu label="&linkToolbar.label;" position="3" >+ <menu label="&linkToolbar.label;" > accesskey="&linkToolbar.accesskey;" >- oncommand="linkToolbarUI.toggleLinkToolbar(event.target)" >- onpopupshowing="linkToolbarUI.initLinkbarVisibilityMenu()"> >+ insertbefore="menuitem_showhide_tabbar" >+ onpopupshowing="event.stopPropagation();linkToolbarUI.initLinkbarVisibilityMenu(); >+ oncommand="event.stopPropagation();linkToolbarUI.toggleLinkToolbar(event.target);""> > <menupopup> > <menuitem label="&linkToolbarAlways.label;" > accesskey="&linkToolbarAlways.accesskey;" > class="menuitem-iconic" type="radio" value="false" > name="cmd_viewlinktoolbar" id="cmd_viewlinktoolbar_false" /> > <menuitem label="&linkToolbarAsNeeded.label;" > accesskey="&linkToolbarAsNeeded.accesskey;" > class="menuitem-iconic" type="radio" value="maybe" >diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js >--- a/suite/browser/navigator.js >+++ b/suite/browser/navigator.js >@@ -2330,18 +2330,19 @@ function updateComponentBarBroadcaster() > compBarBroadcaster.setAttribute('checked', 'true'); > } > else { > compBarBroadcaster.setAttribute('disabled', 'true'); > compBarBroadcaster.removeAttribute('checked'); > } > } > >-function updateToolbarStates(toolbarMenuElt) >+function updateToolbarStates(aEvent) > { >+ onViewToolbarsPopupShowing(aEvent); > updateComponentBarBroadcaster(); > > const tabbarMenuItem = document.getElementById("menuitem_showhide_tabbar"); > // Make show/hide menu item reflect current state > const visibility = gBrowser.getStripVisibility(); > tabbarMenuItem.setAttribute("checked", visibility); > > // Don't allow the tab bar to be shown/hidden when more than one tab is open >diff --git a/suite/browser/navigator.xul b/suite/browser/navigator.xul >--- a/suite/browser/navigator.xul >+++ b/suite/browser/navigator.xul >@@ -194,17 +194,16 @@ > <menubar id="main-menubar"/> > </toolbaritem> > </toolbar> > > <toolbar class="toolbar-primary chromeclass-toolbar" id="nav-bar" persist="collapsed" > grippytooltiptext="&navigationToolbar.tooltip;" > fullscreentoolbar="true" customizable="true" > toolbarname="&navbarCmd.label;" accesskey="&navbarCmd.accesskey;" >- togglemenuitem="cmd_viewnavbar" > defaultset="back-button,forward-button,reload-button,stop-button,nav-bar-inner,search-button-container,print-button,throbber-box,window-controls" > context="toolbar-context-menu"> > > <hbox id="window-controls" hidden="true" fullscreencontrol="true"> > <toolbarbutton id="minimize-button" > tooltiptext="&minimizeButton.tooltip;" > oncommand="window.minimize();"/> > >@@ -222,17 +221,16 @@ > > <toolbar id="PersonalToolbar" > accesskey="&personalbarCmd.accesskey;" > class="chromeclass-directories" > persist="collapsed" > grippytooltiptext="&personalToolbar.tooltip;" > toolbarname="&personalbarCmd.label;" > nowindowdrag="true" >- togglemenuitem="cmd_viewpersonaltoolbar" > customizable="true" > defaultset="home-button,separator,bookmarks-button,personal-bookmarks" > mode="full" > iconsize="small" > labelalign="end" > defaultmode="full" > defaulticonsize="small" > defaultlabelalign="end" >diff --git a/suite/browser/navigatorOverlay.xul b/suite/browser/navigatorOverlay.xul >--- a/suite/browser/navigatorOverlay.xul >+++ b/suite/browser/navigatorOverlay.xul >@@ -217,18 +217,16 @@ > > </commandset> > > <broadcasterset id="navBroadcasters"> > <broadcaster id="canGoBack" disabled="true"/> > <broadcaster id="canGoForward" disabled="true"/> > <broadcaster id="canGoUp" disabled="true"/> > <broadcaster id="Communicator:WorkMode"/> >- <broadcaster id="cmd_viewnavbar" oncommand="goToggleToolbar( 'nav-bar','cmd_viewnavbar');" checked="true"/> >- <broadcaster id="cmd_viewpersonaltoolbar" oncommand="goToggleToolbar('PersonalToolbar','cmd_viewpersonaltoolbar');" checked="true"/> > <broadcaster id="cmd_viewtaskbar" oncommand="goToggleToolbar('status-bar','cmd_viewtaskbar'); updateWindowResizer();" checked="true"/> > <broadcaster id="cmd_viewcomponentbar" oncommand="goToggleToolbar('component-bar', 'cmd_viewcomponentbar');" checked="true"/> > <broadcaster id="isImage"/> > </broadcasterset> > > <!-- bookmarks context menu --> > <popupset id="bookmarksPopupset"> > <menupopup id="bookmarks-context-menu" >@@ -312,19 +310,19 @@ > <menuseparator id="menu_PrefsSeparator"/> > <menuitem id="menu_preferences" oncommand="goPreferences('navigator_pane')"/> > </menupopup> > </menu> > > <menu id="menu_View" accesskey="&viewMenu.accesskey;" label="&viewMenu.label;"> > <menupopup id="menu_View_Popup"> > <menu label="&toolbarsCmd.label;" accesskey="&toolbarsCmd.accesskey;" id="menu_Toolbars"> >- <menupopup id="view_toolbars_popup" onpopupshowing="updateToolbarStates(this);"> >- <menuitem label="&navbarCmd.label;" accesskey="&navbarCmd.accesskey;" class="menuitem-iconic" type="checkbox" observes="cmd_viewnavbar" /> >- <menuitem label="&personalbarCmd.label;" accesskey="&personalbarCmd.accesskey;" class="menuitem-iconic" type="checkbox" observes="cmd_viewpersonaltoolbar" /> >+ <menupopup id="view_toolbars_popup" >+ onpopupshowing="updateToolbarStates(event);" >+ oncommand="onViewToolbarCommand(event);"> > <menuitem id="menuitem_showhide_tabbar" label="&tabbarCmd.label;" accesskey="&tabbarCmd.accesskey;" class="menuitem-iconic" type="checkbox" oncommand="showHideTabbar();" checked="true"/> > <menuitem label="&taskbarCmd.label;" accesskey="&taskbarCmd.accesskey;" class="menuitem-iconic" type="checkbox" observes="cmd_viewtaskbar" /> > <menuitem label="&componentbarCmd.label;" accesskey="&componentbarCmd.accesskey;" class="menuitem-iconic" type="checkbox" observes="cmd_viewcomponentbar"/> > </menupopup> > </menu> > <menuitem id="menuitem_fullScreen" > label="&fullScreenCmd.label;" > accesskey="&fullScreenCmd.accesskey;" >diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js >--- a/suite/common/utilityOverlay.js >+++ b/suite/common/utilityOverlay.js >@@ -313,43 +313,51 @@ function goCustomizeToolbar(toolbox) > else { > return window.openDialog(customizeURL, > "", > "chrome,all,dependent", > toolbox); > } > } > >-function onViewToolbarsPopupShowing(aEvent) >+function onViewToolbarsPopupShowing(aEvent, aInsertPoint) > { > var popup = aEvent.target; >+ if (popup != aEvent.currentTarget) >+ return; > > // Empty the menu > var deadItems = popup.getElementsByAttribute("toolbarid", "*"); > for (let i = deadItems.length - 1; i >= 0; --i) > popup.removeChild(deadItems[i]); > >- var firstMenuItem = popup.firstChild; >+ var firstMenuItem = aInsertPoint || popup.firstChild; > >- var toolbar = document.popupNode; >+ var toolbar = document.popupNode || popup; > while (toolbar.localName != "toolbar") > toolbar = toolbar.parentNode; >- var toolbox = toolbar.parentNode; >+ var toolbox = toolbar.toolbox; > >- var toolbars = toolbox.getElementsByAttribute("toolbarname", "*"); >- for (let i = 0; i < toolbars.length; ++i) { >- let bar = toolbars[i]; >+ var toolbars = Array.slice(toolbox.getElementsByAttribute("toolbarname", "*")); >+ toolbars = toolbars.concat(toolbox.externalToolbars); >+ >+ toolbars.forEach(function(bar) { > let menuItem = document.createElement("menuitem"); >+ menuItem.setAttribute("id", "toggle_" + bar.id); >+ menuItem.setAttribute("class", "menuitem-iconic"); > menuItem.setAttribute("toolbarid", bar.id); > menuItem.setAttribute("type", "checkbox"); > menuItem.setAttribute("label", bar.getAttribute("toolbarname")); > menuItem.setAttribute("accesskey", bar.getAttribute("accesskey")); > menuItem.setAttribute("checked", !bar.hidden); > popup.insertBefore(menuItem, firstMenuItem); >- } >+ }, this); >+ >+ if (popup.id != "toolbar-context-menu") >+ return; > > var mode = toolbar.getAttribute("mode") || "full"; > var modePopup = document.getElementById("toolbarmodePopup"); > var radio = modePopup.getElementsByAttribute("value", mode); > radio[0].setAttribute("checked", "true"); > > var small = toolbar.getAttribute("iconsize") == "small"; > var smallicons = document.getElementById("toolbarmode-smallicons"); >@@ -383,19 +391,20 @@ function onViewToolbarsPopupShowing(aEve > var command = document.getElementById("cmd_CustomizeToolbars"); > var menuitem = document.getElementById("customize_toolbars"); > menuitem.hidden = !command; > menuitem.previousSibling.hidden = !command; > } > > function onViewToolbarCommand(aEvent) > { >+ aEvent.stopPropagation(); > var toolbar = aEvent.originalTarget.getAttribute("toolbarid"); >- var menuitem = document.getElementById(toolbar).getAttribute("togglemenuitem"); >- goToggleToolbar(toolbar, menuitem); >+ if (toolbar) >+ goToggleToolbar(toolbar); > } > > function goSetToolbarState(aEvent) > { > aEvent.stopPropagation(); > var toolbar = document.popupNode; > while (toolbar.localName != "toolbar") > toolbar = toolbar.parentNode; >diff --git a/suite/mailnews/addrbook/addressbook.xul b/suite/mailnews/addrbook/addressbook.xul >--- a/suite/mailnews/addrbook/addressbook.xul >+++ b/suite/mailnews/addrbook/addressbook.xul >@@ -285,22 +285,19 @@ > command="cmd_properties"/> > <menuitem id="menu_preferences" > oncommand="goPreferences('addressing_pane')"/> > </menupopup> > </menu> > <menu id="menu_View"> > <menupopup id="menu_View_Popup"> > <menu id="menu_Toolbars"> >- <menupopup id="view_toolbars_popup"> >- <menuitem id="menu_showAbToolbar" >- label="&showAbToolbarCmd.label;" >- accesskey="&showAbToolbarCmd.accesskey;" >- oncommand="goToggleToolbar('ab-bar2', 'menu_showAbToolbar')" >- checked="true" type="checkbox"/> >+ <menupopup id="view_toolbars_popup" >+ onpopupshowing="onViewToolbarsPopupShowing(event)" >+ oncommand="onViewToolbarCommand(event);"> > <menuitem id="menu_showTaskbar"/> > <menuseparator/> > <menuitem id="menu_showCardPane" > label="&showCardPane.label;" > accesskey="&showCardPane.accesskey;" > oncommand="goToggleSplitter('results-splitter', 'menu_showCardPane')" > checked="true" type="checkbox"/> > </menupopup> >@@ -416,17 +413,16 @@ > <toolbar class="chromeclass-toolbar toolbar-primary" > id="ab-bar2" > persist="collapsed" > customizable="true" > grippytooltiptext="&addressbookToolbar.tooltip;" > toolbarname="&showAbToolbarCmd.label;" > accesskey="&showAbToolbarCmd.accesskey;" > defaultset="button-newcard,button-newlist,separator,button-editcard,button-newmessage,button-newim,button-abdelete,spring,searchBox,throbber-box" >- togglemenuitem="menu_showAbToolbar" > context="toolbar-context-menu"> > <toolbarbutton id="button-newcard" > class="toolbarbutton-1" > label="&newContactButton.label;" > tooltiptext="&newContactButton.tooltip;" > removable="true" > oncommand="AbNewCard();"/> > <toolbarbutton id="button-newlist" >diff --git a/suite/mailnews/compose/MsgComposeCommands.js b/suite/mailnews/compose/MsgComposeCommands.js >--- a/suite/mailnews/compose/MsgComposeCommands.js >+++ b/suite/mailnews/compose/MsgComposeCommands.js >@@ -530,17 +530,16 @@ var defaultController = > case "cmd_delete" : if (MessageGetNumSelectedAttachments() > 0) RemoveSelectedAttachment(); break; > case "cmd_renameAttachment" : if (MessageGetNumSelectedAttachments() == 1) RenameSelectedAttachment(); break; > case "cmd_selectAll" : if (MessageHasAttachments()) SelectAllAttachments(); break; > case "cmd_openAttachment" : if (MessageGetNumSelectedAttachments() == 1) OpenSelectedAttachment(); break; > case "cmd_account" : MsgAccountManager(null); break; > case "cmd_preferences" : DoCommandPreferences(); break; > > //View Menu >- case "cmd_showComposeToolbar" : goToggleToolbar('composeToolbar', 'menu_showComposeToolbar'); break; > case "cmd_showFormatToolbar" : goToggleToolbar('FormatToolbar', 'menu_showFormatToolbar'); break; > > //Options Menu > case "cmd_selectAddress" : if (defaultController.isCommandEnabled(command)) SelectAddress(); break; > case "cmd_quoteMessage" : if (defaultController.isCommandEnabled(command)) QuoteSelectedMessage(); break; > default: > // dump("##MsgCompose: don't know what to do with command " + command + "!\n"); > return; >diff --git a/suite/mailnews/compose/messengercompose.xul b/suite/mailnews/compose/messengercompose.xul >--- a/suite/mailnews/compose/messengercompose.xul >+++ b/suite/mailnews/compose/messengercompose.xul >@@ -416,23 +416,19 @@ > command="cmd_account"/> > <menuitem id="menu_preferences" > oncommand="goDoCommand('cmd_preferences')"/> > </menupopup> > </menu> > <menu id="menu_View"> > <menupopup id="menu_View_Popup"> > <menu id="menu_Toolbars"> >- <menupopup id="view_toolbars_popup"> >- <menuitem id="menu_showComposeToolbar" >- type="checkbox" >- label="&showComposeToolbarCmd.label;" >- accesskey="&showComposeToolbarCmd.accesskey;" >- command="cmd_showComposeToolbar" >- checked="true"/> >+ <menupopup id="view_toolbars_popup" >+ onpopupshowing="onViewToolbarsPopupShowing(event)" >+ oncommand="onViewToolbarCommand(event);"> > <menuitem id="menu_showFormatToolbar" > type="checkbox" > label="&showFormatToolbarCmd.label;" > accesskey="&showFormatToolbarCmd.accesskey;" > command="cmd_showFormatToolbar" > checked="true"/> > <menuitem id="menu_showTaskbar"/> > </menupopup> >@@ -582,17 +578,16 @@ > <toolbar id="composeToolbar" > class="toolbar-primary chromeclass-toolbar" > persist="collapsed" > grippytooltiptext="&mailToolbar.tooltip;" > toolbarname="&showComposeToolbarCmd.label;" > accesskey="&showComposeToolbarCmd.accesskey;" > customizable="true" > defaultset="button-send,separator,button-address,button-attach,spellingButton,button-security,separator,button-save,spring,throbber-box" >- togglemenuitem="menu_showComposeToolbar" > context="toolbar-context-menu"> > </toolbar> > > <toolbarset id="customToolbars" context="toolbar-context-menu"/> > > <toolbar id="MsgHeadersToolbar" > persist="collapsed" > flex="1" >diff --git a/suite/mailnews/mail3PaneWindowCommands.js b/suite/mailnews/mail3PaneWindowCommands.js >--- a/suite/mailnews/mail3PaneWindowCommands.js >+++ b/suite/mailnews/mail3PaneWindowCommands.js >@@ -434,17 +434,17 @@ var DefaultController = > case "cmd_selectAll": > case "cmd_selectFlagged": > return gDBView != null; > // these are enabled on when we are in threaded mode > case "cmd_selectThread": > if (GetNumSelectedMessages() <= 0) return false; > case "cmd_expandAllThreads": > case "cmd_collapseAllThreads": >- return (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay); >+ return gDBView && (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay); > break; > case "cmd_nextFlaggedMsg": > case "cmd_previousFlaggedMsg": > return IsViewNavigationItemEnabled(); > case "cmd_viewAllMsgs": > case "cmd_viewUnreadMsgs": > case "cmd_viewIgnoredThreads": > return gDBView; >diff --git a/suite/mailnews/mailWindowOverlay.xul b/suite/mailnews/mailWindowOverlay.xul >--- a/suite/mailnews/mailWindowOverlay.xul >+++ b/suite/mailnews/mailWindowOverlay.xul >@@ -1147,30 +1147,19 @@ > oncommand="MsgAccountManager(null);"/> > <menuitem id="menu_preferences" oncommand="goPreferences('mailnews_pane')"/> > </menupopup> > </menu> > > <menu id="menu_View"> > <menupopup id="menu_View_Popup" onpopupshowing="view_init()"> > <menu id="menu_Toolbars"> >- <menupopup id="view_toolbars_popup"> >- <menuitem id="menu_showMessengerToolbar" >- type="checkbox" >- checked="true" >- label="&showMessengerToolbarCmd.label;" >- accesskey="&showMessengerToolbarCmd.accesskey;" >- oncommand="goToggleToolbar('msgToolbar', 'menu_showMessengerToolbar');"/> >- <menuitem id="menu_showSearchToolbar" >- type="checkbox" >- checked="true" >- label="&showSearchToolbarCmd.label;" >- accesskey="&showSearchToolbarCmd.accesskey;" >- observes="mailHideMenus" >- oncommand="goToggleToolbar('searchToolbar', 'menu_showSearchToolbar');"/> >+ <menupopup id="view_toolbars_popup" >+ onpopupshowing="onViewToolbarsPopupShowing(event)" >+ oncommand="onViewToolbarCommand(event);"> > <menuitem id="menu_showTaskbar"/> > </menupopup> > </menu> > <menu id="menu_MessagePaneLayout" label="&messagePaneLayoutStyle.label;" > accesskey="&messagePaneLayoutStyle.accesskey;" observes="mailHideMenus"> > <menupopup id="view_layout_popup" onpopupshowing="InitViewLayoutStyleMenu(event)"> > <menuitem id="messagePaneClassic" type="radio" label="&messagePaneClassic.label;" name="viewlayoutgroup" > accesskey="&messagePaneClassic.accesskey;" oncommand="ChangeMailLayout(kClassicMailLayout);"/> >@@ -1932,17 +1921,16 @@ > <toolbar class="toolbar-primary chromeclass-toolbar" > id="msgToolbar" > persist="collapsed" > grippytooltiptext="&mailToolbar.tooltip;" > toolbarname="&showMessengerToolbarCmd.label;" > accesskey="&showMessengerToolbarCmd.accesskey;" > customizable="true" > defaultset="button-getmsg,button-newmsg,separator,button-reply,button-replyall,button-forward,separator,button-goback,button-goforward,button-next,button-junk,button-delete,button-mark,spring,throbber-box" >- togglemenuitem="menu_showMessengerToolbar" > context="toolbar-context-menu"> > </toolbar> > <toolbarset id="customToolbars" context="toolbar-context-menu"/> > > <toolbarpalette id="MailToolbarPalette"> > <toolbarbutton id="button-getmsg" > class="toolbarbutton-1" > type="menu-button" >diff --git a/suite/mailnews/messenger.xul b/suite/mailnews/messenger.xul >--- a/suite/mailnews/messenger.xul >+++ b/suite/mailnews/messenger.xul >@@ -163,17 +163,16 @@ > nowindowdrag="true" > mode="full" > iconsize="small" > labelalign="end" > defaultmode="full" > defaulticonsize="small" > defaultlabelalign="end" > defaultset="mailviews-container,spring,search-container,button-search-container" >- togglemenuitem="menu_showSearchToolbar" > context="toolbar-context-menu"/> > </toolbox> > > <!-- XXX This extension point (tabmail-container) is only temporary! > (See bug 460252 for details.) > We will readd a mechanism for sidebar panes in bug 178003. > --> > <hbox id="tabmail-container" flex="1">

Comment on attachment 512111 [details] [diff] [review] Patch v2.0 Polish and Tidy. Gah stupid bugzilla bug. > IanN seems to be busy with the Compose Toolbars patch. Over to Neil

(In reply to comment #59) > > I can't see how the tabmail constructor relates to mailDisableViewsSearch... > Well OK, but I'm pretty sure that the |gDisableViewsSearch is null| aren't just > a figment of my imagination. Any hints? Not yet, I haven't had time to apply the patch to get a stack backtrace.

Comment on attachment 512111 [details] [diff] [review] Patch v2.0 Polish and Tidy. >- oncommand="linkToolbarUI.toggleLinkToolbar(event.target)" >- onpopupshowing="linkToolbarUI.initLinkbarVisibilityMenu()"> >+ onpopupshowing="event.stopPropagation();linkToolbarUI.initLinkbarVisibilityMenu(); >+ oncommand="event.stopPropagation();linkToolbarUI.toggleLinkToolbar(event.target);""> Didn't you add code to avoid having to change these? >+function onViewToolbarsPopupShowing(aEvent, aInsertPoint) Who passes aInsertPoint? >- var toolbar = document.popupNode; >+ var toolbar = document.popupNode || popup; I guess this works because the view menu lives inside the toolbox? >+ toolbars.forEach(function(bar) { > let menuItem = document.createElement("menuitem"); >+ menuItem.setAttribute("id", "toggle_" + bar.id); >+ menuItem.setAttribute("class", "menuitem-iconic"); > menuItem.setAttribute("toolbarid", bar.id); > menuItem.setAttribute("type", "checkbox"); > menuItem.setAttribute("label", bar.getAttribute("toolbarname")); > menuItem.setAttribute("accesskey", bar.getAttribute("accesskey")); > menuItem.setAttribute("checked", !bar.hidden); > popup.insertBefore(menuItem, firstMenuItem); >- } >+ }, this); this (!) makes no sense. >+ if (popup.id != "toolbar-context-menu") >+ return; This half of the method should be hived off into a separate function called from the submenu's popupshowing handler. (I don't know why we didn't do it that way in the first place...) > function onViewToolbarCommand(aEvent) > { >+ aEvent.stopPropagation(); Why? > //View Menu >- case "cmd_showComposeToolbar" : goToggleToolbar('composeToolbar', 'menu_showComposeToolbar'); break; > case "cmd_showFormatToolbar" : goToggleToolbar('FormatToolbar', 'menu_showFormatToolbar'); break; You're going to bitrot IanN's toolbar customisation :-( >- return (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay); >+ return gDBView && (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay); This needs r=Mnyromyr ;-)

(In reply to comment #59) > > I can't see how the tabmail constructor relates to mailDisableViewsSearch... > Well OK, but I'm pretty sure that the |gDisableViewsSearch is null| aren't just > a figment of my imagination. Any hints? I applied just the messenger.xul part of attachment 512113 [details] [diff] [review] (but tweaked because it was expecting me to have attachment 512111 [details] [diff] [review] applied) and I didn't get any errors. I did notice that the themeing for the search bar that we removed when we had to move it needs to be restored though.

Comment on attachment 512113 [details] [diff] [review] Patch Sv0.1 WIP: Make the MailNews searchBar an external toolbar in the thread pane ->Future

> neil@parkwaycc.co.uk 2011-03-03 13:46:16 PST > >>- oncommand="linkToolbarUI.toggleLinkToolbar(event.target)" >>- onpopupshowing="linkToolbarUI.initLinkbarVisibilityMenu()"> >>+ onpopupshowing="event.stopPropagation();linkToolbarUI.initLinkbarVisibilityMenu(); >>+ oncommand="event.stopPropagation();linkToolbarUI.toggleLinkToolbar(event.target);""> > Didn't you add code to avoid having to change these? Fixed. >>+function onViewToolbarsPopupShowing(aEvent, aInsertPoint) > Who passes aInsertPoint? Firefox. Well I thought I'd be consistent with Firefox. >>- var toolbar = document.popupNode; >>+ var toolbar = document.popupNode || popup; > I guess this works because the view menu lives inside the toolbox? Yes. Hacky but I couldn't think of something more elegant. >>+ toolbars.forEach(function(bar) { >> let menuItem = document.createElement("menuitem"); >>+ menuItem.setAttribute("id", "toggle_" + bar.id); >>+ menuItem.setAttribute("class", "menuitem-iconic"); >> menuItem.setAttribute("toolbarid", bar.id); >> menuItem.setAttribute("type", "checkbox"); >> menuItem.setAttribute("label", bar.getAttribute("toolbarname")); >> menuItem.setAttribute("accesskey", bar.getAttribute("accesskey")); >> menuItem.setAttribute("checked", !bar.hidden); >> popup.insertBefore(menuItem, firstMenuItem); >>- } >>+ }, this); > this (!) makes no sense. Removed. >>+ if (popup.id != "toolbar-context-menu") >>+ return; > This half of the method should be hived off into a separate function called > from the submenu's popupshowing handler. (I don't know why we didn't do it that > way in the first place...) Fixed. >> function onViewToolbarCommand(aEvent) >> { >>+ aEvent.stopPropagation(); > Why? Err, good question. Removed. >> //View Menu >>- case "cmd_showComposeToolbar" : goToggleToolbar('composeToolbar', 'menu_showComposeToolbar'); break; >> case "cmd_showFormatToolbar" : goToggleToolbar('FormatToolbar', 'menu_showFormatToolbar'); break; > You're going to bitrot IanN's toolbar customisation :-( IanN bitrotted me instead ;-) >>- return (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay); >>+ return gDBView && (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay); > This needs r=Mnyromyr ;-) Jens fixed this in the meantime.

Comment on attachment 518947 [details] [diff] [review] Patch v2.1 fix nits. >+ menuItem.setAttribute("class", "menuitem-iconic"); Sorry I didn't notice this before, but why the class? [It obviously wasn't necessary on the context menu before; if you're comparing with the main menu, it was probably historic.] >+ aEvent.stopPropagation(); As with the link toolbar, probably don't need this any more either. > <menupopup id="toolbarmodePopup" >- onpopupshowing="event.stopPropagation();" >+ onpopupshowing="onToolbarmodePopupShowing(event);" [This name looks odd. I wonder whether we can change it to Mode everywhere.]

> >+ menuItem.setAttribute("class", "menuitem-iconic"); > Sorry I didn't notice this before, but why the class? > [It obviously wasn't necessary on the context menu before; > if you're comparing with the main menu, it was probably historic.] Removed. > >+ aEvent.stopPropagation(); > As with the link toolbar, probably don't need this any more either. Removed. > > <menupopup id="toolbarmodePopup" > >- onpopupshowing="event.stopPropagation();" > >+ onpopupshowing="onToolbarmodePopupShowing(event);" > [This name looks odd. I wonder whether we can change it to Mode everywhere.] s/mode/Mode/g done.

Pushed to comm-central. http://hg.mozilla.org/comm-central/rev/b8d6fb8e050a