Toolbar context menu improvements and fixes

RESOLVED FIXED

Status

SeaMonkey
UI Design
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
+++ 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.

Comment 1

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

Comment 2

9 years ago
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).

Comment 3

9 years ago
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.

Comment 4

9 years ago
Is there any way we can get rid of those checkbox menuitems? I find them really counter-intuitive.
(Assignee)

Comment 5

9 years ago
> 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.

Comment 6

9 years ago
> 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.

Comment 7

9 years ago
Created attachment 348379 [details]
context sub-menu

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?

Comment 8

9 years ago
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.
(Assignee)

Comment 9

9 years ago
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.

Comment 10

9 years ago
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?".

Comment 11

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

Comment 12

9 years ago
> 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)
(Assignee)

Comment 13

9 years ago
(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".

Comment 14

9 years ago
> 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?

Updated

9 years ago
Severity: enhancement → normal

Comment 16

9 years ago
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"

Comment 17

9 years ago
"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.

Comment 18

9 years ago
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.
(Assignee)

Comment 19

9 years ago
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.

Comment 20

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

Comment 21

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

Comment 22

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

Yeah, probably.

Comment 23

9 years ago
Can we address the issues that this bug was filed for?
(Assignee)

Comment 24

9 years ago
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.

Comment 25

9 years ago
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.
(Assignee)

Comment 26

9 years ago
Created attachment 367782 [details] [diff] [review]
[obsolete] Patch v1.0 (Draft for comments)

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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #367782 - Flags: review?(neil)
(Assignee)

Updated

9 years ago
Attachment #367782 - Flags: ui-review?(stefanh)

Updated

9 years ago
Attachment #367782 - Flags: review+

Comment 27

9 years ago
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.

Comment 28

9 years ago
Do we need the sub-menu?
(Assignee)

Comment 29

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

Comment 30

9 years ago
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..." ?

Comment 32

9 years ago
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?
(Assignee)

Comment 33

9 years ago
> 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.

Comment 34

9 years ago
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)?
(Assignee)

Comment 35

9 years ago
> 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.

Comment 37

9 years ago
(In reply to comment #34)
> because all other apps

"All" meaning about "three", where two just copied the first without thinking? ;-)

Comment 38

9 years ago
(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).
(Assignee)

Comment 39

9 years ago
> 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.

Comment 40

9 years ago
(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.

Comment 41

9 years ago
"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?

Comment 42

9 years ago
(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.

Comment 43

9 years ago
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.

Updated

9 years ago
Attachment #367782 - Flags: ui-review?(stefanh) → ui-review+

Comment 44

9 years ago
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.

Comment 45

9 years ago
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?).
(Assignee)

Comment 46

9 years ago
> 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.

Comment 47

9 years ago
(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?

Comment 48

9 years ago
A stylesheet overlay should work.

Comment 49

9 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #367782 - Flags: review?(neil)
(Assignee)

Comment 50

7 years ago
I'm going to morph this into a polish and update bug.
Summary: Fine Tune the toolbar context menu after Bug 394288 lands. → Toolbar context menu improvements and fixes
(Assignee)

Comment 51

7 years ago
Created attachment 512111 [details] [diff] [review]
Patch v2.0 Polish and Tidy.

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
Attachment #512111 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

7 years ago
Attachment #367782 - Attachment description: Patch v1.0 (Draft for comments) → [obsolete] Patch v1.0 (Draft for comments)
(Assignee)

Comment 52

7 years ago
Created attachment 512113 [details] [diff] [review]
Patch Sv0.1 WIP: Make the MailNews searchBar an external toolbar in the thread pane

Use this to test the toolbar show/hide menu.
Attachment #512113 - Flags: feedback?(neil)
(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.
(Assignee)

Comment 54

7 years ago
> > 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.
(Assignee)

Comment 55

7 years ago
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?
(Assignee)

Comment 57

7 years ago
> 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...
(Assignee)

Comment 59

7 years ago
> 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.
(Assignee)

Comment 60

7 years ago
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">
Attachment #512111 - Flags: review?(neil)
Attachment #512111 - Flags: review?(iann_bugzilla)
Attachment #512111 - Flags: review?
(Assignee)

Comment 61

7 years ago
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
Attachment #512111 - Flags: review?
(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.
(Assignee)

Comment 65

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

->Future
Attachment #512113 - Flags: feedback?(neil)
(Assignee)

Comment 66

7 years ago
Created attachment 518947 [details] [diff] [review]
Patch v2.1 fix nits.

> 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.
Attachment #512111 - Attachment is obsolete: true
Attachment #518947 - Flags: review?(neil)
Attachment #512111 - Flags: review?(neil)
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.]
(Assignee)

Comment 68

7 years ago
Created attachment 519907 [details] [diff] [review]
Patch v2.2 fix more 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.]
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.
Attachment #518947 - Attachment is obsolete: true
Attachment #519907 - Flags: review?(neil)
Attachment #518947 - Flags: review?(neil)

Updated

7 years ago
Attachment #519907 - Attachment is patch: true
Attachment #519907 - Attachment mime type: application/octet-stream → text/plain

Updated

7 years ago
Attachment #519907 - Flags: review?(neil) → review+
(Assignee)

Comment 69

7 years ago
Pushed to comm-central.
http://hg.mozilla.org/comm-central/rev/b8d6fb8e050a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.