358277 - Email page location doesn't insert the URL (url in angle brackets; HTML compose)

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[jay]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20061018 Camino/1.1a1
Build Identifier: Camino/1.1a1

Launching File>Email Page Location does not work, at least in Thunderbird.  The URL is not inserted into the body of the email.  All that I get is a summary of the URL in the subject box.  The lack of the URL itself, however, means that the location is not mailed at all.  This seems new.  In earlier versions of Camino, there was no such problem. 

Reproducible: Always

Steps to Reproduce:
1. Try with any URL
2.
3.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

WFM in Eudora.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Camino sends urls in angle brackets to prevent url breakage, truncation, and other assorted suckage in mail clients (bug 200040).  What we're doing works fine in Eudora and Mail.app, so this sounds like a Thunderbird bug.

Comment 3

[jay]

(In reply to comment #2)
> Camino sends urls in angle brackets to prevent url breakage, truncation, and
> other assorted suckage in mail clients (bug 200040).  What we're doing works
> fine in Eudora and Mail.app, so this sounds like a Thunderbird bug.
> 

I've been using sequential versions of Camino and Thunderbird for more than a year.  There was no failure to insert the URL using Email Page Location.  I went to a daily build of Camino a couple days ago, and noticed the problem today.  My Thunderbird version hasn't changed. 

Comment 4

[stefano giovannini]

Have the same problem -
I posted to the Camino forum thinking it was a Camino bug [have no issue with Fx]
here is what I wrote

http://forums.mozillazine.org/viewtopic.php?p=2560586#2560586

"Email Page Location" does not work anymore in Camino 1.1 and 1.2
either the toolbar button or the menu.

If I click it Thunderbird creates a new message with the right subject but there is no URL in the body of the message.
I need to go back and copy and paste it.
[Profile] [Pri. Msg.]

replies stated that there was no problems with Eudora and Mail.app.

Thunderbirg bug then. Camino 1.2 TB 2.0b

Comment 5

[Chris Lawson (gone)]

I'd be more than happy to look into this if someone would point me in the right direction. I don't know editor well at all, so I have no idea where the relevant code is.

cl

Comment 6

[Chris Lawson (gone)]

Confirming in Tb 2.0b2 (20070116).

While Camino (the application that manifests the bug) is Mac OS X-only, I rather doubt the bug is confined to OS X, as others have claimed this bug exists in all Gecko-based mail clients on OS X (probably excepting Correo). That would indicate it's either a bug in editor or an XUL bug shared across the various apps.

cl

Comment 7

[Mike Cowperthwaite]

Is the bug with TB or with Camino?  The way this information is passed -- at least on Windows -- is that the message body is constructed by the program that wants to send the message; TB doesn't do anything with the text other than pump it into a composition window for editing.

Comment 8

[Chris Lawson (gone)]

(In reply to comment #7)
> Is the bug with TB or with Camino?

Thunderbird. Every other mail client in the world handles a link passed into it like this:

<http://foo>

Thunderbird has no idea how to handle anything in <> ... but ONLY if HTML composition is the default. This works fine if you use plain text, I was told last night.

>  The way this information is passed -- at
> least on Windows -- is that the message body is constructed by the program that
> wants to send the message; TB doesn't do anything with the text other than pump
> it into a composition window for editing.

It's the "pump it into a composition window" part that's broken. Camino is passing a perfectly valid URL, as you can see here:

http://lxr.mozilla.org/mozilla/source/camino/src/browser/BrowserWindowController.mm#2631

Comment 9

[Phil Ringnalda (:philor)]

(In reply to comment #8)
> Thunderbird has no idea how to handle anything in <> ... but ONLY if HTML
> composition is the default.

That's not strictly true, and you have to be careful about not regressing existing things (like, possibly something in SeaMonkey) that expect what currently happens, that passing in (a URI-escaped version of) |<a href="http://foo">foo</a>| creates a link to foo. I'd be inclined to only entity-escape the <> when the innards look like a URI, and nothing but a URI.

Comment 10

[Chris Lawson (gone)]

Attachment: Testcase with clickable URLs

Here's a fun testcase that replicates (via HTML) what Camino is passing in to Tb.

With HTML composition on, the first and third links work fine, but the second (identical to the behaviour Camino exibits) fails.

With HTML composition turned off, all three work exactly as expected.

Someone should be able to use this testcase to determine whether the bug exists on Windows as well, and to what extent (if any).

Comment 11

[[e-mail]]

Comment on attachment 257639 [details]
Testcase with clickable URLs

Example 2 doesn't work with Mozilla 1.6 in HTML mode.

>mailto:[e-mail]?body=%26lt;http://foo.example.com%26gt;
This works in HTML mode, but not in plain text mode:

>mailto:[e-mail]?html-body=<a%20href="http://foo.example.com">http://foo.example.com</a>
This forces HTML mode, but might not work with other mailers:

>mailto:[e-mail]?body=<http://foo.example.com>&html-part=<a href="http://foo.example.com">http://foo.example.com</a>
This forces HTML mode, but might work with other mailers:

Comment 12

[Mike Cowperthwaite]

Thanks for the testcase, Chris; I wasn't getting what you were saying, but now I see.  This does occur in Windows; and Opera is another browser that does Send Page like that: it formats the link as:  <URL: http://mozilla.org >   
with, of course, the same result.

Comment 15

[Phil Ringnalda (:philor)]

(In reply to comment #5)
> I'd be more than happy to look into this if someone would point me in the right
> direction.

Heh. That's what you think!

The problem looks to be in http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/compose/src/nsMsgComposeService.cpp&rev=1.135&mark=552-558#538 - the current flow is "is there an html-body? use it. none? use body. find out if we compose HTML, if so sanitize the body" when it should instead be "do we compose HTML? is there an html-body? if no html-body and we compose HTML, escape the body, if html-body (which forces composing HTML) sanitize the body."

The current scheme, treating body=<a href="http://foo">bar</a> as an HTML link, rather than as the less-than character followed by an "a", etc., doesn't just break Camino's link-passing, it violates http://tools.ietf.org/html/rfc2368#section-2 - the hvalue for body contains the first text/plain body part, not something which when plaintext serialized from HTML becomes the first text/plain body part. It's unfortunate for anyone lazily using the body= in mailto links for HTML message templates, but they need to switch to putting their HTML in html-body or html-part instead.

(In reply to comment #11)
> mailto:[e-mail]?html-body=<a%20href="http://foo.example.com">http://foo.example.com</a>
> This forces HTML mode, but might not work with other mailers:

Just to add to the amusing pile of brokenness, while that forces HTML mode and inserts a link in SeaMonkey, it forces HTML mode and inserts the text "%3Ca%20href" etc. in Thunderbird. For bonus fun, as near as I can tell, in both Thunderbird and SeaMonkey, what's actually sent from that forced HTML composition is plaintext only, no matter whether or not you default to composing in HTML (unless SM + default to HTML works, I think that's the one combination I didn't try).

Comment 16

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

While I could make a career out of filing bugs on the things that are broken within a few dozen lines of here (if the HTML parser chokes, we'll stick the HTML source in a plaintext composer, even if there's also a plaintext part?!), this appears to be enough to fix *this* bug.

Keep in mind that I usually screw up whenever I touch C++, so don't spare the reviewing.

Comment 17

[Phil Ringnalda (:philor)]

Comment on attachment 285707 [details] [diff] [review]
Fix v.1

Er, oops.

Comment 18

[Phil Ringnalda (:philor)]

Attachment: Fix v.3

This might be a bit less wrong.

Comment 19

[David :Bienvenu]

thx, Phil, this looks like the right thing to do, but I think I'm going to have to test it to make sure I understand when this is called and what it does...

Comment 20

[David :Bienvenu]

Comment on attachment 285713 [details] [diff] [review]
Fix v.3

thx, Phil. This looks ok, and at least doesn't break things on Windows (which doesn't seem to add the < > )

Comment 21

[Phil Ringnalda (:philor)]

(In reply to comment #20)
> doesn't break things on Windows (which doesn't seem to add the < > )

Though as comment 12 notes, Opera on Windows does, with a freaky
  <URL: http://foo >
that was already strange by the time RFC 2396 appendix E said "um, yeah, whatever dude." At any rate, this patch also makes Tbird work with Opera/Win, where it didn't before. 

Comment 22

[Phil Ringnalda (:philor)]

Ah, the trivia you can find if you look longer: RFC 1738 was rather fond of <URL:http://foo>, to distinguish it from <URI:urn:whateverdude> and <whatever:dude>.

Comment 23

[[e-mail]]

Comment on attachment 285713 [details] [diff] [review]
Fix v.3

>+          CopyUTF8toUTF16(escaped, rawBody);
Is it possible to copy this directly into the sanitised body?
(I doubt we need to sanitise something we just escaped).

Comment 24

[Phil Ringnalda (:philor)]

Attachment: Fix v.5

It's not just possible to copy directly to sanitizedBody, it's eminently sensible. However, that called for another round of testing, and I couldn't persuade myself to ignore the brokenness around the edges another time, so, some scope creep.

Bug 12851 added decoding of MIME encoded headers in mailto: URIs, but despite bug 12851 comment 0 very clearly and carefully calling out the parts of RFC 2368 which say that only non-body parts should be MIME decoded, it also added decoding of body, so we're misleading our users into thinking that it's okay to use MIME encoded words in body, even though it won't work with other clients.

Bug 90728 added support for html-body and html-part, but failed to URI-decode them. That wasn't casually obvious, since typing something trivial like mailto:[e-mail]?html-body=<b>bold foo</b> in the addressbar worked, until bug 389106 added escaping of spaces in URIs passed to external handlers, which turned the resulting compose window to "*bold%20foo*". In fact, it never did work right, since a properly-constructed link to test would have been |<a href="mailto:nobody&mozilla.org?html-body=%3Cb%3Ebold%20foo%3C%2Fb%3E">mailto</a>|, which would have failed all along.

Comment 25

[Phil Ringnalda (:philor)]

Attachment: Test links

A not-quite-random selection of varieties of escaping in various parts.

Comment 26

[Phil Ringnalda (:philor)]

Comment on attachment 286444 [details] [diff] [review]
Fix v.5

Wups, as Neil points out, this would look better if I didn't go back to my earlier leaking use of nsEscapeHTML.

Comment 27

[Phil Ringnalda (:philor)]

Attachment: Fix v.6

Must be getting close - I actually managed to attach an even-numbered version, without realizing I needed to do something else before I got it attached.

Comment 28

[Phil Ringnalda (:philor)]

mailnews/compose/src/nsSmtpUrl.cpp 1.83
mailnews/compose/src/nsMsgComposeService.cpp 1.136

Comment 30

[Mike Cowperthwaite]

V fixed with TB 3.0a1pre-1201.  Thanks, Phil!