Currently, even though we overlay messengercompose.xul with contentAreaContextOverlay.xul we don't actually make use of the context menu from it. If we did, we could remove some code duplication. This patch: * Adds a getBrowser function to the compose window. * Removes unneeded openEditorContextMenu function. * Adds event listener for popupshowing so that updateEditItems is still called. * Removes msgComposeContext context menu. * Adds overlay that gives two missing context items for context menu.

I notice that the mailContext has a hybrid approach - it defines its own menuitems but then calls on nsContextMenu to set things up.

This alternative patch: * Replaces the onpopupshowing on contentAreaContextMenu, this means a getBrowser function is not needed nor is adding an event listener.

This patch: * Does the context menu the same way as mailContext does in mailWindowOverlay, but I do think this one adds as well as taking away some duplication of code.

Comment on attachment 530935 [details] [diff] [review] Use contentAreaContext for messenger compose window alternative patch v1.0 >+ gContextMenu = >+ new nsContextMenu(popup, document.getElementById("content-frame")); > updateEditItems(); [As it happens, nsContextMenu already updates the standard edit items.] >+ <popupset id="contentAreaContextSet"> Nit: don't need this, because you know the id of the popup. >+ <menupopup id="contentAreaContextMenu" >+ onpopupshowing="return event.target != this || >+ openEditorContextMenu(this);"> >+ <menuitem id="context-pasteNoFormatting" >+ insertafter="context-paste" >+ command="cmd_pasteNoFormatting"/> >+ <menuitem insertafter="context-pasteNoFormatting" >+ label="&pasteQuote.label;" >+ accesskey="&pasteQuote.accesskey;" >+ command="cmd_pasteQuote"/> This almost works, but the sidebar uses the same popup, so the menuitems appear (permanently) on the sidebar content context menu.

Comment on attachment 530875 [details] [diff] [review] Use contentAreaContext for messenger compose window patch v1.0 Actually I can't decide between either of these patches. I guess it depends on which way turns out to be easier to hide the extra menuitems in the sidebar.

Comment on attachment 531470 [details] [diff] [review] Do it the same way as mailContext alternative patch v1.0 I see what you mean about the duplication, because we already have a context menu in this window, which means that we don't get the right overlay love.

Comment on attachment 530935 [details] [diff] [review] Use contentAreaContext for messenger compose window alternative patch v1.0 >+ gContextMenu = >+ new nsContextMenu(popup, document.getElementById("content-frame")); > updateEditItems(); >+ return gContextMenu.shouldDisplay; This version has the advantage that you could write something like this: gContextMenu = new nsContextMenu(popup, document.getElementById("content-frame")); if (gContextMenu.shouldDisplay) { // handle extra paste items here } return gContextMenu.shouldDisplay;

Changes since v1.0: * Hide extra menuitems as default and unhide with JS in onpopupshowing. * Remove unneeded <popupset> tags in overlay. * Only update extra menuitems as rest are done by nsContextMenu code.

Comment on attachment 532052 [details] [diff] [review] Use contentAreaContext for messenger compose window alternative patch v1.1 [I don't think this is going to work, because the sidebar also uses context="contentAreaContextMenu" which will call openEditorContextMenu.]

(In reply to comment #9) > Comment on attachment 532052 [details] [diff] [review] [review] > Use contentAreaContext for messenger compose window alternative patch v1.1 > > [I don't think this is going to work, because the sidebar also uses > context="contentAreaContextMenu" which will call openEditorContextMenu.] Okay a custom sidebar tab (added using http://edmullen.net/mozilla/sidebarpreview.php) gives a: Error: window.getBrowser is not a function Source file: chrome://navigator/content/navigator.xul Line: 1 which suggests to me that, probably due to the load order and how overlays work (at the moment), it is not getting overlaid by the new file.

(In reply to comment #10) > Error: window.getBrowser is not a function Oh dear. Regression from bug 562339 :-(

Changes since v1.1: * Use WhichElementHasFocus() to determine if on message body. * Take into account fix for non-browser sidebar issue (bug 656886).

Comment on attachment 532199 [details] [diff] [review] Use contentAreaContext for messenger compose window alternative patch v1.2 >+ var focusedElement = WhichElementHasFocus(); Nit: could optimise this, since you're only interested in == content >+ var showPasteExtra = focusedElement && focusedElement == content; Nit: superfluous extra test for focusedElement &&

For checkin

Comment on attachment 532331 [details] [diff] [review] Use contentAreaContext for messenger compose window alternative patch v1.2a [Checked in: Comment 15] http://hg.mozilla.org/comm-central/rev/ac811b06e1c2