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

NEW
Unassigned

Status

()

Firefox
Menus
11 years ago
4 years ago

People

(Reporter: Michael A. Puls II, Unassigned)

Tracking

({polish})

Trunk
polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

11 years ago
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 "email1@site.com, email2@site.com, email3@site.com, email4@site.com, email5@site.com, email6@site.com" 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 "email1@site.com, email2@site.com". 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 "email1@site.com, email2@site.com" to the clipboard and NOT just "email1@site.com" like it currently does.

Reproducible: Always
(Reporter)

Comment 1

11 years ago
Created attachment 239153 [details]
*Rough* draft *example* of how it can be done ( view source to see )
(Reporter)

Comment 2

11 years ago
Created attachment 239340 [details]
browser.js from Gecko/20060609 Minefield/3.0a1 with fix. ( search for  "Email : funct" )
(Reporter)

Comment 3

11 years ago
****. 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.
(Reporter)

Comment 4

11 years ago
Created attachment 239344 [details]
This new browser.js fix actually works right.  ( start at line 4194 )
Attachment #239153 - Attachment is obsolete: true
Attachment #239340 - Attachment is obsolete: true

Comment 5

11 years ago
(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!
(Reporter)

Comment 6

11 years ago
"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
(Reporter)

Comment 7

11 years ago
Created attachment 240682 [details] [diff] [review]
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>
(Reporter)

Comment 8

11 years ago
Created attachment 240684 [details]
Uploaded TC page for reference, so we have it
Keywords: polish
(Reporter)

Comment 9

11 years ago
Created attachment 261037 [details] [diff] [review]
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.
(Reporter)

Comment 10

9 years ago
Created attachment 367462 [details] [diff] [review]
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.
Attachment #239344 - Attachment is obsolete: true
Attachment #240682 - Attachment is obsolete: true
Attachment #240684 - Attachment is obsolete: true
Attachment #261037 - Attachment is obsolete: true
Shadow, would you mind to create the patch against the current head of mozilla-central?
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 12

9 years ago
(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?
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!
(Reporter)

Comment 14

9 years ago
Created attachment 367603 [details] [diff] [review]
Patch against mozilla-central

O.K., that's the diff that I got.
Attachment #367462 - Attachment is obsolete: true
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?
Attachment #367603 - Flags: review?(gavin.sharp)
(Reporter)

Comment 16

9 years ago
Created attachment 367758 [details]
Here's a detailed description of what the new code does step by step.
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.
(Reporter)

Comment 18

9 years ago
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.
(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

9 years ago
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?
(Reporter)

Comment 21

9 years ago
(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.
(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...
I would be ok with having something for doing this in necko, I think.
> I would be ok with having something for doing this in necko, I think.

I second that.
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.
Attachment #367603 - Flags: review?(gavin.sharp)
(Reporter)

Comment 26

7 years ago
Created attachment 529623 [details]
Example implementation with tests

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

Comment 27

7 years ago
(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.
(Reporter)

Comment 28

7 years ago
(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.

Updated

6 years ago
Duplicate of this bug: 678927
(Reporter)

Comment 30

6 years ago
(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

6 years ago
(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

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

Comment 33

4 years ago
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>
You need to log in before you can comment on or make changes to this bug.