messagePaneContext and threadPaneContext are about 85% the same, and the xul duplication not only adds unnecessary nodes to the DOM, it creates extra code duplication. This patch combines to two menus into 1.

With this patch a lot of menus that are shown when they shouldn't. E.g. Report scam for the thread pane, Compose Mail To, Add to Address Book... and probably some more. Also, I think it will be quite confusing if the ids still say threadpaneContext when it's not only that...

This fixes a couple of bugs that caused things to remain showing when they shouldn't have. Note that I haven't changed the IDs here, because there are a variety of other places that depend on these IDs, so I think it'd be better to do that in a second patch.

Comment on attachment 345940 [details] [diff] [review] patch v2 > + // don't show mail items for links/images, just show related items. Capital D. >+ // Most menu items are visible if there's 1 or 0 messages selected, and >+ // enabled if there's exactly one selected. Handle those here. >+ function setSingleSelection(aID, aOptional) { >+ var optional = aOptional != undefined ? aOptional : true; >+ ShowMenuItem(aID, single && !hideMailItems && optional); >+ EnableMenuItem(aID, single); >+ } Doxygen/javadoc style for the function doc would be nice. Especially a @param comment about what the aOptional should do. I'm not too crazy about aParam names for javascript either. >+ // handle our separators >+ function hideIfAppropriate(aID) { >+ var separator = document.getElementById(aID); >+ var sibling = separator.previousSibling; >+ while (sibling) { >+ if (sibling.getAttribute("hidden") != "true") { >+ if (sibling.localName != "menuseparator" && >+ hasAVisibleNextSibling(separator)) { >+ ShowMenuItem(aID, true); >+ } else { >+ ShowMenuItem(aID, false); >+ } Wrong braces style (here, and a few other places). But then again you don't need any braces at all here. >- if(item && item.hidden != "true") >+ if (item.hidden != "true") > item.hidden = !showItem; Trailing space, and i'd think you don't have to explicitly compare to "true" here --- Then a few functional comments: E.g. for a HTML message, right click image, or link Select ALl Copy [disabled] ---- ---- Copy Image Location I also get a disabled "Copy" item shown a lot. In the standalone msg window I seem to get no context menu at all. As much as it sucks to change ids, I'd prefer making the id names sane now (a few months after they landed) rather than later. Maybe threadPane/messagePaneContext-XXX -> mailContext-XXX or something.

Addresses previous review comments. (For the record, I'm much in favor of "} else {" style, but you're right, it wasn't necessary here.)

Comment on attachment 347086 [details] [diff] [review] patch v3 For multiple selection, fwd as attachments is missing. (Also Get Selected Messages, but i guess that's intentional, as it was grayed out earlier - didn't investigate furhter.) Html msg img/link context menu, still get the (double) extra separator. > + var enabled = (item.getAttribute('disabled') !='true'); Missing space. > + function setSingleSelection(aID, aOptional) { > + var optional = aOptional != undefined ? aOptional : true; Still don't quite understand what this param implies. I suggest you rename it to something that says what it really does. > + var enabled = (item.getAttribute('disabled') !='true'); > + if (enableItem != enabled) { > + item.setAttribute('disabled', enableItem ? '' : 'true'); item.disabled = !enableItem; ? I also got an exception "GetThreadTree is not defined" while I was clicking around, in news? (I assume it's due to this patch.)

Addresses comments from previous review.

Comment on attachment 348204 [details] [diff] [review] patch v4 Looks good! r=mkmelin One minor nit, add a period: > + // Copy is available as long as something is selected

Comment on attachment 348204 [details] [diff] [review] patch v4 >diff -r 9413bd795016 mail/base/content/ABSearchDialog.xul >--- a/mail/base/content/ABSearchDialog.xul Fri Nov 14 15:05:50 2008 +0100 >+++ b/mail/base/content/ABSearchDialog.xul Fri Nov 14 11:51:47 2008 -0500 >@@ -105,17 +105,17 @@ > </hbox> > </vbox> > > <splitter id="gray_horizontal_splitter" collapse="after" persist="state"/> > > <vbox id="searchResults" flex="4" persist="height"> > <vbox id="searchResultListBox" flex="1" > > <tree id="abResultsTree" flex="1" enableColumnDrag="true" class="plain" >- context="threadPaneContext" >+ context="mailContext" As this has a tree id="abResultsTree" instead of "threadTree" is this going to cause a problem with your new code? > onclick="AbResultsPaneOnClick(event);" > onkeypress="AbResultsPaneKeyPress(event);" > onselect="this.view.selectionChanged();" > sortCol="GeneratedName" > persist="sortCol"> > >diff -r 9413bd795016 mail/base/content/mailContextMenus.js >+ // Select-all and copy are only available in the message-pane >+ if (inThreadPane) { >+ document.getElementById("mailContext-selectall").hidden = true; >+ document.getElementById("mailContext-copy").hidden = true; >+ } >+ ShowMenuItem("threadPaneContet-openNewTab", inThreadPane); Typo? Contet? Correct here and in the xul whilst you're touching the files? >diff -r 9413bd795016 mail/base/content/mailWindowOverlay.xul > <menuitem id="threadPaneContet-openNewTab" > label="&contextOpenNewTab.label;" > accesskey="&contextOpenNewTab.accesskey;" > oncommand="MsgOpenNewTabForMessage();"/>

And as mentioned on IRC, need to make sure any Lightning bustage (as they rely on threadPaneContext to overlay) is dealt with.

(In reply to comment #9) > And as mentioned on IRC, need to make sure any Lightning bustage (as they rely > on threadPaneContext to overlay) is dealt with. and messagePaneContext

Fallen asked me to cc the lightning bugmail, since this will cause issues with Lightning. (I'm not sure why Lightning redefines these function rather than using addEventListener("popupshowing") to handle its overlaid elements...)

Comment on attachment 348204 [details] [diff] [review] patch v4 >+ /** >+ * Most menu items are visible if there's 1 or 0 messages selected, and >+ * enabled if there's exactly one selected. Handle those here. >+ * @param aID the id of the element to display/enable >+ * @param aHide (optional) an additional criteria to evaluate when we >+ * decide whether to display the element. If false, we'll hide >+ * the item no matter what messages are selected >+ */ >+ function setSingleSelection(aID, aHide) { >+ var hide = aHide != undefined ? aHide : true; >+ ShowMenuItem(aID, single && !hideMailItems && hide); >+ EnableMenuItem(aID, single); >+ } To me the second (optional) argument of this function is wrong. aHide should either be renamed or the logic changed - "If false, we'll hide the item no matter what messages are selected", the name suggests it should hide the item not show it! So I would change it to: >+ * Most menu items are visible if there's 1 or 0 messages selected, and >+ * enabled if there's exactly one selected. Handle those here. >+ * @param aID the id of the element to display/enable >+ * @param aHide (optional) an additional criteria to evaluate when we >+ * decide whether to display the element. If true, we'll hide >+ * the item no matter what messages are selected >+ */ >+ function setSingleSelection(aID, aHide) { >+ var hide = aHide != undefined ? aHide : false; >+ ShowMenuItem(aID, single && !hideMailItems && !hide); >+ EnableMenuItem(aID, single); >+ } then change the various callers where need be.

or change aHide/hide to aShow/show

SM equivalent bug 469498

threadPaneContet-openNewTab type fixed in 470509. (Fix caller before checking in.)

Landed as 1544:71688230bf66

seems this change might have caused bug 473168