Jesse Ruderman Reporter
	Description • 25 years ago

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.

	Richard Zach
	Comment 1 • 25 years ago

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

	Bill Law
	Comment 2 • 25 years ago

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...

	Bill Law
	Comment 3 • 25 years ago

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

	Bill Law
	Comment 4 • 25 years ago

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).

	sairuh (rarely reading bugmail)
	Comment 5 • 25 years ago

lisa, german, your thoughts...?

	lchiang
	Comment 6 • 25 years ago

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.

	jglick
	Comment 7 • 25 years ago

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

	don
	Comment 8 • 25 years ago

Move to M16 for now ...

	don
	Comment 9 • 25 years ago

Move to M20 target milestone.

	sairuh (rarely reading bugmail)
	Comment 10 • 24 years ago

*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.

	Sean Richardson
	Comment 11 • 24 years ago

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.

	Matthew T (active 1999-2002)
	Comment 12 • 24 years ago

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)?

	Jesse Ruderman Reporter
	Comment 13 • 24 years ago

cc blake in case he's interested.

	Erich 'Ricky' Iseli
	Comment 14 • 24 years ago

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

	Erich 'Ricky' Iseli
	Comment 15 • 24 years ago

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.

	Gervase Markham [:gerv] Assignee
	Comment 16 • 24 years ago

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

Gerv

	Gervase Markham [:gerv] Assignee
	Comment 17 • 24 years ago

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

	Gervase Markham [:gerv] Assignee
	Comment 18 • 24 years ago

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

	Bill Law
	Comment 19 • 24 years ago

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. 

	Gervase Markham [:gerv] Assignee
	Comment 20 • 24 years ago

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

	Gervase Markham [:gerv] Assignee
	Comment 22 • 24 years ago

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

	Gervase Markham [:gerv] Assignee
	Comment 23 • 24 years ago

[ 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

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 26 • 24 years ago

+ 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)

	Gervase Markham [:gerv] Assignee
	Comment 27 • 24 years ago

> 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

	Gervase Markham [:gerv] Assignee
	Comment 29 • 24 years ago

/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

	Alec Flett
	Comment 33 • 24 years ago

coolness! sr=alecf

	Gervase Markham [:gerv] Assignee
	Comment 34 • 24 years ago

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

	timeless
	Comment 36 • 24 years ago

+            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]

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 24 years ago

+        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.

	Gervase Markham [:gerv] Assignee
	Comment 38 • 24 years ago

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

	Gervase Markham [:gerv] Assignee
	Comment 39 • 24 years ago

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

	Dan Rosen
	Comment 40 • 24 years ago

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...

	Gervase Markham [:gerv] Assignee
	Comment 42 • 24 years ago

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

	Dan Rosen
	Comment 43 • 24 years ago

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.

	Gervase Markham [:gerv] Assignee
	Comment 44 • 24 years ago

> 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

	Dan Rosen
	Comment 45 • 24 years ago

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 :)

	Gervase Markham [:gerv] Assignee
	Comment 46 • 24 years ago

> 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

	Dan Rosen
	Comment 48 • 24 years ago

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...

	Gervase Markham [:gerv] Assignee
	Comment 49 • 24 years ago

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

	Dan Rosen
	Comment 50 • 24 years ago

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.

	Gervase Markham [:gerv] Assignee
	Comment 51 • 24 years ago

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

Gerv

	Boris Zbarsky [:bzbarsky]
	Comment 53 • 24 years ago

The code inside "isLinkType : function ( linktype, link ) {" could use some
better indentation...

fix that and r=bzbarsky@mit.edu

	Alec Flett
	Comment 54 • 24 years ago

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

	Dan Rosen
	Comment 55 • 24 years ago

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.

	Gervase Markham [:gerv] Assignee
	Comment 57 • 24 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 24 years ago

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

	Bill Law
	Comment 60 • 24 years ago

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.

	Gervase Markham [:gerv] Assignee
	Comment 61 • 24 years ago

Checked in by bz - thanks :-)

Gerv

	Dan Rosen
	Comment 62 • 24 years ago

verif.