Content tabs don't currently have a context menu. We've been discussing this and decided it would be good to have at least a minimal menu for Thunderbird 3 (which unfortunately means some more strings). We can actually obtain a lot of a context menu free by re-using the one we have on the message pane. There's going to be two parts to this bug 1) Make the existing context menu work across different types of tabs & windows. 2) Add new strings specific to content tabs. I'm still not quite sure about extensibility of the context menu, but I think we need to get the strings in and possibly think about that a bit more later.

This patch doesn't affect strings, but the next one will. Main changes in this patch: - Moved the decision making on what to show/not to show wrt message items into nsContextMenu.js - Moved nsContextMenu.js into messenger.jar from comm.jar; there's no reason for it to be in comm.jar now (and comm.jar contains little, so IMHO we should kill it sometime). Also dropped the preprocessing on the file. - Removed the old checks for seeing if there's a background image as we don't really use it. I left comments should we want to put it back later. UX affecting changes: - "Copy" is now only shown when we actually have some text selected rather than all the time and sometimes being disabled (the old code semi-intended to do this, but overwrote one decision with another). - "Move to Again" used to be shown disabled for newsgroups even though "Move To" wasn't, "Move to Again" is now not shown for newsgroups (but is in the other places). - "Archive" is now only shown to the same time as the move option. Before this patch it was being shown virtually everywhere, including when right clicking on audio/video controls. - Content Tabs will have the possibility for the following options to be displayed depending on what is being right-clicked (this list doesn't show the actual order): * Save Link As... * Save Image As... * Copy * Copy Link Location * Copy Image Location * Select All * Add to Address Book... (for mailto links) * Compose Message To (for mailto links) * Copy Email Address (for mailto links) * Play / Pause (for video/audio elements) * Mute / Unmute (for video/audio elements) I've tested this quite a bit and not found any missing items yet ;-)

Comment on attachment 403044 [details] [diff] [review] Part 1 - Rework and share Magnus, if you've got bandwidth, I could use help here, since we need to clear this one out to get to his patch with l10n impact.

Comment on attachment 403044 [details] [diff] [review] Part 1 - Rework and share (In reply to comment #1) > UX affecting changes: > > - "Copy" is now only shown when we actually have some text selected rather than > all the time and sometimes being disabled (the old code semi-intended to do > this, but overwrote one decision with another). > - "Move to Again" used to be shown disabled for newsgroups even though "Move > To" wasn't, "Move to Again" is now not shown for newsgroups (but is in the > other places). > - "Archive" is now only shown to the same time as the move option. Before this > patch it was being shown virtually everywhere, including when right clicking on > audio/video controls. > - Content Tabs will have the possibility for the following options to be > displayed depending on what is being right-clicked (this list doesn't show the > actual order): > > * Save Link As... > * Save Image As... > * Copy > * Copy Link Location > * Copy Image Location > * Select All > * Add to Address Book... (for mailto links) > * Compose Message To (for mailto links) > * Copy Email Address (for mailto links) > * Play / Pause (for video/audio elements) > * Mute / Unmute (for video/audio elements) I'm good with this, did a little testing and everything looked normal but better - i can't archive when right clicking on an email address!

I lied, there will probably be three patches - the third one will do "Open this page in browser" or something like that. This patch adds the following items to the context menu: - Stop, Reload: Shown when right-clicking on a contentTab browser only. - Undo, Cut, Paste (Copy is already in there): Shown only when right-clicking on a textbox or input element. Additional notes: - nsMsgStatusFeedback was wired up to enable/disable the stop command. It now doesn't and we ask it if we're busy if necessary when we update the command handler. This change impacted on the subscribe dialog which now looks after its stop button itself. - Added in the check for onCanvas in nsContextMenu - this allows us to exactly mirror what FF checks when working out if to display the stop and reload commands. - The reload enable/disabling in contentTab probably isn't entirely necessary, but I'd like to keep it matching what FF do for now at least.

Minor update to part 1. I realised that I'd broken the global search results list context menu (show as list or click on message from global search). The logic was incorrectly assuming it was a non-message tab. This is now fixed by comparing tab modes with those in mailTabType.modes. The context menu in the global search results list still isn't as extensive as it could be - bug 519192 will fix that, I just didn't want to make it worse here ;-)

Comment on attachment 403227 [details] [diff] [review] [checked in] Part 1 - Rework and share v2 Looks good, except for the "Move to xxx Again" hiding on news. It should be like it was, since it shifts to "Copy to xxx Again" when copy was the last action. And copy of course works for news.

Comment on attachment 403217 [details] [diff] [review] [checked in] Part 2 - Add options for Stop, Reload, Undo, Cut, Paste Looks good. Tested using Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("mail:3pane").document.getElementById("tabmail").openTab("contentTab",{contentPage: "http://tinyvid.tv/show/2h9led44g152z"});

Comment on attachment 403217 [details] [diff] [review] [checked in] Part 2 - Add options for Stop, Reload, Undo, Cut, Paste nice, works great. thanks to magnus for the testing snippet.

Comment on attachment 403227 [details] [diff] [review] [checked in] Part 1 - Rework and share v2 a=me for both of these patches as I know we really want these. Additionally as most of it was copy/pasted then it should be reasonably low risk.

Comment on attachment 403227 [details] [diff] [review] [checked in] Part 1 - Rework and share v2 Checked in: http://hg.mozilla.org/comm-central/rev/6f68182d5e7f

Comment on attachment 403217 [details] [diff] [review] [checked in] Part 2 - Add options for Stop, Reload, Undo, Cut, Paste Checked in: http://hg.mozilla.org/comm-central/rev/21e93a577fa3

Last patch for this bug (which affects strings). As discussed with davida and dmose on irc, two more items - Open in Browser and Open Link in Browser. "Open in Browser" is shown when right-clicking on the page and not a link/image/video etc. It is also hidden if the link is an about: or chrome: as those two don't make any sense to try and open in a browser. "Open Link in Browser" is shown when right-clicking on a link that isn't a mailto link and isn't an about: or chrome: link. Dan - I've put you as reviewer as you probably know most about the external protocol handler stuff - if you haven't got time, please pass it on.

Comment on attachment 403466 [details] [diff] [review] [checked in] Part 3 - Open in Browser and Open Link in Browser >diff --git a/mail/base/content/nsContextMenu.js b/mail/base/content/nsContextMenu.js >--- a/mail/base/content/nsContextMenu.js >+++ b/mail/base/content/nsContextMenu.js >@@ -152,8 +152,20 @@ nsContextMenu.prototype = { > goUpdateCommand("cmd_reload"); > } > >+ let loadedProtocol = ""; >+ if (content.document && content.document.location) >+ loadedProtocol = content.document.location.protocol; >+ >+ this.showItem("mailContext-openInBrowser", shouldShow && loadedProtocol && >+ loadedProtocol != "about:" && loadedProtocol != "chrome:"); It's not clear from reading this function exactly why shouldShow applies to what it does, both in general, but particularly as it relates to the above context item. Comments would help, renaming "shouldShow" might also make sense, as might splitting up the second argument. >+ const mailContextSeparators = [ >+ "mailContext-sep-open-browser", "mailContext-sep-link", >+ "mailContext-sep-open", "mailContext-sep-open2", >+ "mailContext-sep-reply", "paneContext-afterMove", >+ "mailContext-sep-afterMarkMenu", "mailContext-sep-edit", >+ "mailContext-sep-copy", "mailContext-sep-reportPhishing" >+ ]; >+ >+ for (let i = 0; i < mailContextSeparators.length; ++i) >+ this.hideIfAppropriate(mailContextSeparators[i]); >+ Mmmm, table-driven FTW! It's unlikely to be a problem here, so no need to worry about this now, but for future reference, Array.forEach is a good habit to get into; see <https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for...in> for details about why. r=dmose

Comment on attachment 403466 [details] [diff] [review] [checked in] Part 3 - Open in Browser and Open Link in Browser I clarified the variable and also swapped to forEach. Checked in: http://hg.mozilla.org/comm-central/rev/e722c91a1823

I believe we have all the options we need as a minimum now, so marking this bug as fixed.