If you select "Open link in new window" for a mailto link such as this@fake.email.address.org , a blank mozilla window opens, followed by a composer (e-mail) window. This is kinda pointless, so I think the menu item should be removed. Other fun items on context menus for mailto links: - "Edit link in composer" opens both editor (blank) and composer. - "Save link as" opens a download window with some information "missing" in a weird way. "#1K of #2K bytes..." See also bug 32356, "copy link location" should be changed for mailto links.

Alternatively, you could just not open a new browser/editor window in these cases. Reassign to law.

If somebody were to summarize what they'd like the context menu to have for mailto: links, it would minimize the likelihood that they'd be unhappy with what I'd choose...

*** Bug 32356 has been marked as a duplicate of this bug. ***

Just to make sure it doesn't get lost, 32356 said to have two menu choices: copy mailto link, copy mailto address. I suggest just always copying only the address (and tweak the wording of the selection for bonus points).

lisa, german, your thoughts...?

I'll cc: jglick for mail. The mail menu spec doesn't show what the context menu for mailto links should be. Only shows context menu for other types of links.

For mailto links, I think "copy email address" is enough.

Move to M16 for now ...

Move to M20 target milestone.

*spam*: transferring current XP Menu bugs over to jrgm, the new component owner. feel free to add me to the cc list (unless am the Reporter) of any of these, if you have any questions/etc.

IMO: (a) there should be a link-traversing item for mailto: links (b) the existing "Open Link in New Window" and "Edit link in Composer" should be replaced by a "Compose Message" item (c) the existing "Copy Link Location" item should be replaced by a "Copy E-mail address" item (d) on Mac, "Compose Message" should be the default item (a) having only a "Copy E-mail address" item would remove existing functionality (sending e-mail to the address in a mailto:) from Mozilla (Following the steps in the first paragraph above with 4.x on Win, there are two separators in a row -- it looks like the two "open link" items were removed to work around the same problem!) (b) and (c) would be enhancements in the spirit of this bug Between them, (a), (b) and (d) would allow Mac users to send a message without dismissing the context menu first if it appears because the link has been hovered over for half a second.

Straw-man proposal: _Back _Forward -------------------------- _New Message [default item] _Copy E-mail Address Copy _Link _Add to Address Book ... (where `default item' has the meaning given in bug 37596) Two other questions: * Where is the spec for the context menu for links of type http:// links? * Where is the spec for the context menu for links of a type we don't know about (jabber:foo@bar, or worple://smeg@foo.org/wotsit)?

cc blake in case he's interested.

*** Bug 67881 has been marked as a duplicate of this bug. ***

MPT's proposal for the mailto context menu is very good. I like it. However don't forget that e-mail addresses inside a non formatted e-mail message also appear as links and this should also apply there, even if we don't have the mailto: scheme in the code.

The current behaviour (described above) is pants. :-) We need to fix this, in the way that mpt suggests. Nominating. Gerv

law: There are a few questions for you at the bottom :-) OK, I'm having a go at this. :-) I've got it completely working, subject to the below. I'm following MPT's spec roughly, but I'm using "Send _Mail To" to be consistent with a right-click in Mail/News. I've left "Copy Link Location" as it is, for consistency, and added "Copy _Email Address" which comes immediately above Copy Link Location. Do we really need Add To Address Book? Looks like bloat to me... law: Can you tell me how I can check, in JS, if Mail/News is installed or not? Secondly, when over a mailto: link I turn off some commands which lead to two separators next to one another. Do I need to insert logic to remove one of them, or is that OK? Gerv

For that matter, why do we need "Send Mail To"? That's what happens when you click the link! We don't have an "Open Link in this Window" normally, so we shouldn't have Send Mail To. law: following the above BGO, I don't need to know how to detect Mail/News any more :-) I could still do with an answer to the separators question, though. Related questions: should we be "Edit Link In Composer"ing for HTTP links only? And how come we have "Edit Link In Composer" and not "Edit This Page in Composer"? (I hate both, but we should be consistent.) I know 4.x does it that way, but that's no excuse... Gerv

There's examples of removing redundant separators within nsContextsMenu.js. See, for example, http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/nsContextMenu.js#126 Whether you can use Composer on a linked-to document depends on that document's content-type, not the protocol by which it's fetched. I don't know why there's no "Edit this page in composer" choice.

Cheers. Patches coming. Please review :-) (Side note: diff on Win32 sucks rocks. If these patches are broken, that is why. Please ask for individual file patches if this is the case.) Changed files: contentAreaContextOverlay.xul contentAreaCommands.dtd nsContextMenu.js contentAreaCommands.dtd is from the en-US jar. I take it I don't have to localise it for DE etc. myself? ;-) Gerv

*** Bug 39572 has been marked as a duplicate of this bug. ***

[ law: just noticed you are no longer getting mail from this bug, of course. Adding CC. Could I have review of these, please? :-) ] OK, so that last patched sucked big rocks. Let's have another go... Attaching single non-zipped diff for review for the two major files. This replaces diff.txt in the zip. No whitespace version this time - there's not much difference. It also needs diff2.txt from the zip for the entire fix. Note that this patch also fixes a few missing key bindings, and various troubles with the "Save Link As..." and other menu items appearing on e.g. News links. Gerv

+ isLinkSaveable : function ( link ) { + return !(this.isLinkType( "mailto:" , link ) || + this.isLinkType( "javascript:" , link ) || + this.isLinkType( "news:", link ) || + this.isLinkType( "secnews:", link ) ); + }, Can't we save news/secnews (isn't it ``snews''?) articles? + // Generate email address and put it on clipboard. + copyEmail : function () { + // Copy the comma-separated list of email addresses only. + var qmark = this.linkURL().indexOf("?"); + if (qmark > 7) { + this.copyToClipboard( this.linkURL().substring( 7, qmark ) ); + } + else { + this.copyToClipboard( this.linkURL().substr( 7 ) ); + } How about: copyEmail: function() { var url = this.linkURL(); var qmark = url.indexOf("?"); if (qmark > 7) // XXX document magic constant! var addrs = url.substring(7, qmark); else addrs = url.substr(7); this.copyToClipboard(url, addrs); } But then, will that work for mailto:alecf@netscape.com?to=attinasi@netscape.com&to=ben@netscape.com&to=bienvenu@netscape.com&to=blizzard@mozilla.org&to=brendan@mozilla.org&to=darin@netscape.com&to=erik@netscape.com&to=hewitt@netscape.com&to=hyatt@netscape.com&to=jband@netscape.com&to=jst@netscape.com&to=kin@netscape.com&to=mscott@netscape.com&to=roc+moz@cs.cmu.edu&to=rpotts@netscape.com&to=sfraser@netscape.com&to=tor@cs.brown.edu&to=scc@mozilla.org&to=shaver@mozilla.org&to=sspitzer@netscape.com&to=vidur@netscape.com&to=waterson@netscape.com anyway? (http://www.mozilla.org/hacking/reviewers.html)

> Can't we save news/secnews (isn't it ``snews''?) articles? Yeah, it's snews. My mistake. Theoretically, you should be able to save these, but we don't Do The Right Thing - attempting to save a news://URL give a 0-byte file. So I thought we could turn it off for now to save bug reports. <shrug> > copyEmail: function() { > var url = this.linkURL(); > var qmark = url.indexOf("?"); > if (qmark > 7) // XXX document magic constant! > var addrs = url.substring(7, qmark); > else > addrs = url.substr(7); > this.copyToClipboard(url, addrs); > } My mummy always told me: "Never leave brackets off if/else, otherwise someone will come along and add something else to the clause, and things will break." There's nothing wrong with your version; but you haven't said what's wrong with mine either. Unless you mean the substr/substring thing. > But then, will that work for > mailto:alecf@netscape.com?<snip> No. mailto: URLs cunningly have several ways of specifying recipients; a comma-separated list before the question mark, or the contents of any of the to=, cc= or bcc= name/value pairs after the ?. Unless there's a service that'll decode this for me, I thought I'd go for the 95% case rather than attempt to roll my own parser. Gerv

/me swears never to do any hacking on Win32 again... Here's a proper patch. <sigh>. alecf: you sr=ed my last change in this area. Any chance of you doing it again? Gerv

coolness! sr=alecf

blake, timeless (or anyone for that matter): Any chance of an r= from either of you? It's only 30 lines or so. Then we can check this in at last :-) (/me hopes blake doesn't object to reviewing Gerv's patches any more) Gerv

+ return return link.protocol.toLowerCase() === linktype; return return foo === bar; // what? and i'm assuming this is different from bug 47743 which i think i'll check in now [it looks different]

+ this.copyToClipboard(url, addresses); what is this copyToClipboard? It's not defined anywhere; LXR hath it not, and it's not in this patch. Needless to say this "copy email address" functionality does not work.

Timeless' catch is a typo, but I I'm really confused now about bzbarsky's. Sorry, people. This patch has been waiting for review for so long that I'd forgotten how it worked. Back to square 1. <sigh> Gerv

OK, I've worked out what happened. dr ripped out that code three days ago when he changed CopyLinkLocation to use cmd_copyLink. Because that was the only function that was using copyToClipboard, he nuked it (reasonably enough.) Now we could put it back. However, perhaps a more sane thing to do would be to add something to xpfe/global/resources/content/nsClipboard.js - which currently seems to have functions for getting stuff from the clipboard, but not for writing stuff to it. However, that file seems to be slightly deeper JS-fu than I understand. I could just add my function, but it wouldn't "fit", if you see what I mean. dr or anyone - any comments? Gerv

Hmm, yeah, the code I ripped out of JS got reincarnated in C++ more-or-less wholesale as the helper function PresShell::CopyStringToClipboard. Note that PresShell also has a copy-to-clipboard helper function for HTML content, DoCopy, which is significantly more complicated... Regardless, It'd be nice to better situate these helpers in an nsIClipboardHelpers or some such. Maybe this even exists -- I haven't looked at nsClipboard.js...

This patch transplants the copyToClipboard JS functionality to nsClipboard.js. There is some fu in there I don't totally understand, not to mention a funky brace style, so this needs careful review to see if there are better ways to do what I've done. It would be good to get this in, because I can use it to help fix bug 35835 as well. Gerv

Gerv: best not to check in code you don't grok :) Seriously, I'd suggest slapping together a quick and dirty C++-implemented, scriptable nsIClipboardHelper and transplant the code that I have in PresShell right now (and possibly the DoCopy in PresShell as well, if it's at all doable). If you want to file a bug to that effect on me, feel free.

> Gerv: best not to check in code you don't grok :) Well, it's been in the tree before, and it works and does the right thing, so I think the chances of it being problematic are slim. Re: the nsIClipboardHelper, I'm happy to file a bug on you if you can tell me when you might get it done ;-) It doesn't seem to matter whether this is done in C++ or JS and we have an implementation in JS already... A bird in the hand, and all that. We could use this now, and convert later if you like. Gerv

Mmm, seems fair, I guess. So, a review: - In contentAreaCommands.dtd, why are a whole bunch of those entities disappearing? - Please clean up the indentation and braces in nsClipboard.js. If you want to understand that code better, look at the CopyStringToClipboard function in nsPresShell.cpp. You have to jump through some real hoops to use the clipboard... Hence my desire for a clipboard helper. You also mentioned that you can use the clipboard code to fix 35835. In fixing both of these, please comment in the code to the effect of "an nsIClipboardHelper should be coming online sooner or later, that we'll want to use instead." I'll file a bug on myself to implement that and clean up users of the old js helpers. r=dr, with these changes. (btw, please accept and target this bug, if you're working on it :)

> In contentAreaCommands.dtd, why are a whole bunch of those entities > disappearing? Nothing's disappearing - it's just whitespace changes. I've added the comment referencing the bug you've filed. I'm not sure how many people actually use nsClipboard.js at the moment - I think it's very few. New patch coming with changes required for r=dr. Gerv

Gerv: I'm working on the nsIClipboardHelper right now. If you can wait a few days, I'll likely have it landed. If you're itching to land immediately, go ahead, but be prepared to rip out your js clipboard code. By the way, I noticed a method in nsIClipboard which you'll want to use in your copyText() function: supportsSelectionClipboard(). You'll want to wrap your second copyTextTo() call in an if(whatever.supportsSelectionClipboard()) so as not to no-op on windows and mac. I'm curious why you've gone and totally reformatted nsClipboard.js, but hey...

Perhaps because a certain Dan Rosen said: "Please clean up the indentation and braces in nsClipboard.js." ? :-) Or is that not what you meant? Anyway, it used to use some half-assed indentation scheme. If you are working on the helper right now, I might as well hold off :-) Is there a proposed interface definition? Gerv

Oh, I only meant in your own function :) Umm, there's no proposed IDL just yet, but it'll be obvious enough... Check 78010 for patches, coming soon.

Ok, now we're cooking. New, simpler patch, using Dan's nice new shiny helper, coming right up. Gerv

The code inside "isLinkType : function ( linktype, link ) {" could use some better indentation... fix that and r=bzbarsky@mit.edu

your tabs are screwed up... look over the patch. sr=alecf once you use spaces appropriately.

The patch looks good to me. Here's the thing: I did all this work to make cmd_copyLink, cmd_copyImageLocation and cmd_copyImageContents happen. All this functionality was implemented before, but only in javascript called from our front-end. In fact, you'll notice that the js code in nsContextMenu to determine whether we're over a copyable link or image is *still there* even though there's duplication in PresShell. In other words, If you want to be a *really* good citizen, and make copy-email-address work not just in navigator but also in any client, using the command structure that we love so much (eg. cmd_copyEmailAddress) is what you want to do. It's not that much extra work, as a matter of fact -- you can look at what I did for cmd_copyLink and duplicate that work. You'll then notice that all of this complex code which law wrote in nsContextMenu would also be better suited to a helper interface living in /content... I suppose it's a lot to ask, though. Here's what you should do. Your patch looks fine to me, so I'll give you r=dr. But you should file two new bugs: (1) implement a helper api in content to determine things like "am I on a link", "am I on an image", or "what is my href", and (2) clean up commands and context menus to use the new api.

dr: sure. I see your point. I'm not sure I'm quite up to sorting that out yet, though. Do I assign these bugs to you? ;-) Anyone feel like checking this in? Gerv

er.. the new patch still has broken spaces...

You might also see if there's drag/drop code that could use the new "content analyzing" apis. I noticed once that there was some overlap with that, too.

Checked in by bz - thanks :-) Gerv

verif.