353102 - "Copy email address" - incomplete parsing of the mailto URI results in missing email addresses

Status: NEW

(Firefox :: Menus, defect)

Description

[Michael A. Puls II]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7

In Firefox, when you right-click on a mailto link and left-click on "Copy email address", not all TO addresses are copied to the clipboard.

This is inconsistent with how mail clients like Thunderbird and Opera's M2 parse mailto links into their compose windows.

For example, if you have the following properly-encoded mailto URI:

"mailto:email1%40site.com%2C%20email2%40site.com?to=email3%40site.com%2C%20email4%40site.com&to=email5%40site.com%2C%20email6%40site.com&subject=subject&body=body"

, you will get "[e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail]" in M2's TO: field in its compose window. In Thunderbird, you'll get the same.

That's because the final TO: value should be a comma-separated list of all email addresses in the mailto: keyword value and all email addresses in all TO= keyword values. M2 and Thunderbird get this right.

However, Firefox's "copy email address" function will only return "[e-mail], [e-mail]". It doesn't parse any of the TO keywords, which are valid according to RFC 2368 - Section 2.

If you goto :

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.705#5017 

, you can see why Firefox doesn't get all of the TO email addresses. The function gets a substring of between 'mailto:' and the first '?' or the end of the URI if no '?' is present. That's all it does as far as parsing goes.

I see the "There are other ways of embedding email addresses in a mailto: link, but such complex parsing is beyond us", but I really think the function should be fixed to get all the addresses.

Using a shorter example:

"Copy email address" on "mailto:email1%40site.com?to=email2%40site.com" should copy "[e-mail], [e-mail]" to the clipboard and NOT just "[e-mail]" like it currently does.

Reproducible: Always

Comment 1

[Michael A. Puls II]

Attachment: *Rough* draft *example* of how it can be done ( view source to see )

Comment 2

[Michael A. Puls II]

Attachment: browser.js from Gecko/20060609 Minefield/3.0a1 with fix. ( search for "Email : funct" )

Comment 3

[Michael A. Puls II]

****. I'm splitting by '&' and forgot that other keywords count as elements in the array. So, the code in the attachments are close, but broken. I'll fix it.

Comment 4

[Michael A. Puls II]

Attachment: This new browser.js fix actually works right. ( start at line 4194 )

Comment 5

[Peter Kasting]

(In reply to comment #4)
> Created an attachment (id=239344) [edit]
> This new browser.js fix actually works right.  ( start at line 4194 )

It will be easier for people to look at your work if you provide patches, rather than whole new .js files.  If you have a CVS checkout of the source, use cvs diff -u8pN.  If you don't, use diff -u8pN against an unmodified copy of the source files you're changing.  Thanks!

Comment 6

[Michael A. Puls II]

"If you don't, use diff -u8pN against an unmodified copy of the
source files you're changing".

O.K. I'll try to do that now that I know the desired way. Thanks

Comment 7

[Michael A. Puls II]

Attachment: Patch for "chrome/browser.jar/content/browser/browser.js" that comes with "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060929 Minefield/3.0a1".

With the patch in place, Firefox passes the TCs at <http://shadow2531.com/opera/testcases/mailto/001.html>

Comment 8

[Michael A. Puls II]

Attachment: Uploaded TC page for reference, so we have it

Comment 9

[Michael A. Puls II]

Attachment: Patch for "chrome/browser.jar/content/browser/nsContextMenu.js" that comes with "Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a3pre) Gecko/20070325 Minefield/3.0a3pre".

Since the copyEmail function was moved to nsContextMenu.js, here's a new patch. Also, the previous patch did not skip empty TO hvalues. This one does.

The same TC is still valid, but you can also right-click on the mailto link at <http://shadow2531.com/opera/testcases/mailto/rfc2368-3.html> and Copy Email Address to test.

Comment 10

[Michael A. Puls II]

Attachment: New nsContextMenu patch

This will make Firefox *really* good at Copy Email Address.

Made nsContextMenu.diff from nsContentxMenu.js from Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090314 Minefield/3.2a1pre

This makes the current comment "There are other ways of embedding email addresses in a mailto: link, but such complex parsing is beyond us." untrue.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shadow, would you mind to create the patch against the current head of mozilla-central?

Comment 12

[Michael A. Puls II]

(In reply to comment #11)
> Shadow, would you mind to create the patch against the current head of
> mozilla-central?

Sure. What are the exact steps to do it?

Are these it?

1. hg clone http://hg.mozilla.org/mozilla-central src
2. cd src
3. Modify \browser\base\content\nsContextMenu.js with my changes
4. hg diff -p -U 8 browser/base/content/nsContextMenu.js > nsContextMenu.js.diff

And, if so, should the diff file contain all +s?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yeah, looks good. Here just two links which will give you all the needed information in detail. Just x-check:

https://developer.mozilla.org/en/Mozilla_Source_Code_%28Mercurial%29
https://developer.mozilla.org/En/Creating_a_patch

Thanks!

Comment 14

[Michael A. Puls II]

Attachment: Patch against mozilla-central

O.K., that's the diff that I got.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 367603 [details] [diff] [review]
Patch against mozilla-central

Thanks for this patch Michael.

Gavin, would you mind having a look at it?

Comment 16

[Michael A. Puls II]

Attachment: Here's a detailed description of what the new code does step by step.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

mailnews already has mailto: parsing code exposed via nsIMailtoURL:
http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsSmtpUrl.cpp#67

I wonder if it would make sense to move that into netwerk rather than duplicate the code. I'm not too excited about adding a bunch of custom URL parsing code for what is likely a pretty uncommon use case.

Comment 18

[Michael A. Puls II]

That would be cool to use the same code as what Thunderbird uses to get all the To addresses from a mailto URI. The custom parsing I created is really just duplicating what Thunderbird already does.

Although, Thunderbird suffers from <https://bugzilla.mozilla.org/show_bug.cgi?id=423768> and the JS code I created does not.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #17)
> mailnews already has mailto: parsing code exposed via nsIMailtoURL:
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsSmtpUrl.cpp#67
> 
> I wonder if it would make sense to move that into netwerk rather than duplicate
> the code. I'm not too excited about adding a bunch of custom URL parsing code
> for what is likely a pretty uncommon use case.

Mark and David, what do you think about this move? Would this be fine for you?

Comment 20

[David :Bienvenu]

I'm ambivalent - Thunderbird has to do quite a  bit more parsing of mailto than Firefox would, since we care about more than the e-mail address. And putting things in necko makes it that much tougher if we have to fix a bug. On the other hand, parsing mailto: links seems like a feature browsers and browser extensions would be interested in, so I'd be curious what the necko folks think. Michael, do you think you might be able to propose a patch for bug 423768?

Comment 21

[Michael A. Puls II]

(In reply to comment #20)
>  Michael, do you think you might be able to propose a patch for bug
> 423768?

<http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsSmtpUrl.cpp#120>

There needs to be a version of PL_strcasecmp() that percent-decodes the token* value before converting it to lowercase and comparing. Then, you could use the new function instead.

I don't know anything more than that though. I see the code is trying to be very efficient though by just scanning through the char*, which might make things harder to adapt, but I don't know the code at all.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #20)
> extensions would be interested in, so I'd be curious what the necko folks
> think.

Lets cc biesi and boris...

Comment 23

[Boris Zbarsky [:bzbarsky]]

I would be ok with having something for doing this in necko, I think.

Comment 24

[Jason Duell]

> I would be ok with having something for doing this in necko, I think.

I second that.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 367603 [details] [diff] [review]
Patch against mozilla-central

This review request has been pending for far too long - I'm very sorry about that, Michael.

I think we should get the following before we change the existing context menu mailto: parsing code:
- some tests that represent the set of cases we want to handle (this should be relatively easy to test, using the browser-chrome harness)
- comparison of those test cases against handling from other browsers/clients, and the spec (if there is a useful spec here - I don't know that there is)
- investigation of whether we can move this parsing into the core and share it amongst different consumers (Thunderbird/Firefox)

That's probably sounds like a lot to demand in response to what started out as a rewrite of a small, existing JS function, I realize. It shouldn't be too hard to do (at least not compared to initially preparing this patch), though, and I'd be happy to help if someone is still willing to take this task on.

Comment 26

[Michael A. Puls II]

Attachment: Example implementation with tests

Attaching example implementation with tests. This one handles fragment identifiers properly. The patch doesn't.

Comment 27

[Michael A. Puls II]

(In reply to comment #25)
> - comparison of those test cases against handling from other browsers/clients,
> and the spec (if there is a useful spec here - I don't know that there is)

You can look at <http://www.ietf.org/rfc/rfc6068.txt> for the most updated "to" handling.

Or, you can look at <http://shadow2531.com/opera/testcases/mailto/mailto_uri_scheme_idea.html#consuming> which is also updated often and was used as feedback for RFC6068 and is way more specific than RFC6068.

You can see <http://shadow2531.com/js/mailto_uri_parser.js> that implements the rules, which is used by <https://addons.opera.com/addons/extensions/details/gmail-compose/>.

The stuff above is largely based on Thunderbird and M2's parsing of mailto URIs (including "to" values and cases where there are multiple "to" values).

Where things are not defined anywhere, I've done and specified what makes sense, what works and what's robust.

But, specifically for right-click -> copy address in Opera, it used to only copy one address and Opera was fixed to copy all of them by using M2's parsing code that it uses for its To field, which is the same thing that's really being suggested here for Firefox. It just makes sense.

------
Also, of interest:

You can look at the older <http://shadow2531.com/opera/testcases/mailto/modern_mailto_uri_scheme.html#duplicates>, which more or less specified what Thunderbird did at the time.

<http://shadow2531.com/opera/userjs/BeforeMailtoURL.js> is a simplification of the rules used for webmail composing.

I've also written a mailto URI parser in 13 different languages at <http://shadow2531.com/opera/testcases/mailto/MailtoURIParserPack.zip> (although they need to be updated to handle fragment identifiers and "&" before "?"). Parsing "to" values as suggested doesn't seem to be a problem in any of the languages.

Comment 28

[Michael A. Puls II]

(In reply to comment #26)
> Created attachment 529623 [details]
> Example implementation with tests
> 
> Attaching example implementation with tests. This one handles fragment
> identifiers properly. The patch doesn't.

Feel free to create a patch that implements that idea or fix or fix <https://bugzilla.mozilla.org/show_bug.cgi?id=423768> and use Thunderbird's code.

Hope that helps.

Comment 30

[Michael A. Puls II]

(In reply to Jo Hermans from comment #29)
> *** Bug 678927 has been marked as a duplicate of this bug. ***

Is that really the same? That's about a "copy link address" option missing.

Comment 31

[Nicolas Barbulesco]

(In reply to Jo Hermans from comment #29)

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

(In reply to Michael A. Puls II from comment #30)

> (In reply to Jo Hermans from comment #29)
> > *** Bug 678927 has been marked as a duplicate of this bug. ***
> 
> Is that really the same? That's about a "copy link address" option missing.

That's not a duplicate.
Bug 353102 is about improving "Copy email address(es)".
My request bug 678927 is about "Copy link address", which is missing.
Even if "Copy email address(es)" becomes perfect and copies 10 email addresses, that will not give me the link address. I want to copy the link address, like for any normal link. With "mailto:" and the email addresses and the subject and whatever. Web browsers have been providing that simple feature for 10 years. Please bring it back. Thanks.

Comment 32

[Nicolas Barbulesco]

I particularly miss this feature because sometimes the link address is not fully shown although there is plenty of space : bug 658918.

Comment 33

[Ulf Zibis]

Same problem still here with:
<a href="mailto:[Bitte Empfänger angeben]?subject=Link-Empfehlung: Indonesien: Terror und Vertreibung für Palmöl&body=Hallo,%0A%0Aich möchte gerne folgende Seite weiterempfehlen:%0A%0Ahttps://www.regenwald.org/aktion/936/indonesien-terror-und-vertreibung-fuer-palmoel">
 Empfehlung direkt aus Ihrem Mailprogramm verschicken.</a>