132257 - Inserting a link to a [network][image] file into a message inserts the physical file!

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Arnaud de Villèle]

When inserting a link in an html message to a local network file
messenger insert the link right but insert the file too...
I tried to reduce the size of my email and beeeeep I failed !

-- 
arakeis

Info : Build 2002031903 / W2K SP2 / French

Comment 1

[Jean-Francois Ducarroz]

I can understand your frustation Arnaud but network file are not considered as
remote attachment therefore we send the file as well in order to be sure the
recipient will be able to access it. Like we do with any local file.

I don't really know yet how we can fix that dilemna. Maybe we could ask the user
what to do in that specific case which hopfully should not append so often.

Comment 2

[Arnaud de Villèle]

Jean-François wrote :
>network file are not considered as
>remote attachment

That's my point: a link to a file in a message isn't an attachment !
maybe I wasn't clear enough ? Do you think I should start some kind
of a poll on mozillazine ?

-- 
Arakeis

Comment 3

[Jean-Francois Ducarroz]

Why do we send the file data when you put a link to it into your message body?
to let the recipient be able to use the link else it's useless! We do that only
with file:// urls but not for http:// or ftp:// urls.

The problem is that network file use a regular file url (file://) therefore
there is not an easy way to know if a file is local or remote. Sure on Windows
we can look for a double back slash at the beginning but on other platform like
Mac or Unix, it's another story...

BTW, did you try just typing the url into the message body instead of inserting
a link tag? That might does just do what you want.

Feel free to start a poll if you want on mozillazine or a dicussion on
netscape.public.mozilla.mailnews.

Comment 4

[Arnaud de Villèle]

As I understand it, because sending a "pure" link for a local file to somewone
won't be usable for the receiver, the mailnews module translate it into a mix
of attachement and link. This is definitivly not the good solution (IMHO),
the function attachement exit on it's own, if people don't understand what a
link is, let them fail and redo the manipulation the right way !
Anyway thank you for your quick response (even if the response wasn't
the one I woped for)

-- 
Arakeis

Comment 5

[Arthur]

Adding keyword privacy. Just imagine you want to ease the work of someone else
by using a link to *their* file on *their* computer (i.e. /etc/passwd on the
network) then that's the solution I would choose and it never ever would come to
my mind that this could attach *my* /etc/passwd file.

Comment 6

[Jean-Francois Ducarroz]

That's a good point! I think we need to warm/ask the user to attach or not the
file when inserting a link into the message body. In fact the best way would be
do do it during the send process as there is many way to provoke a file to be
automatically attached.

Comment 7

[Arnaud de Villèle]

the ultimate solution ... would be :
1/ ask if user want's his file attached,
2/ if anwser is yes : show the attachment in attachment box,

this allow the user to correct an eventual typo when answering,
by removing the file.

Comment 8

[Brian Smith]

In MS Outlook we often send links to "local" files that are on network shares.
We do not want to attach the file, but send the link. The same think should be
possible for Mozilla/Netscape.

This behavior is _not_ intuitive and the user risks sending private/confidential
information unknowingly. 

Comment 9

[Arnaud de Villèle]

Sad that correction of a incorrect and secret behaviour is considered as a
enhancement...

Comment 10

[Ian Neal]

Actually NS4.7x let you do this without attaching a file, so should really be
marked as 4xp too. In fact if someone using NS4.7x sends u a file:// link,
mozilla doesn't let you open it - I'll log a separate bug on this.

Comment 11

[Ian Neal]

Separate bug as mentioned in comment 10 is bug 135830

Comment 12

[Jean-Francois Ducarroz]

this is similar but different that bug 128693

Comment 13

[Jean-Francois Ducarroz]

The current workaround is to add the following attribute to the link (you can
use the advanced edit button in the link property dialog):
moz-do-not-send = true

I good solution would be to add a checkbox item in the link property dialog to
let the user chose to attach or not the file linked to the message.

Comment 14

[Jean-Francois Ducarroz]

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

Comment 15

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v1

The fix add 2 functionalities:

1) you can setup a user preference in prefs.js (currently named
"mail.compose.dont_attach_source_of_remote_file_url") to tell Mozilla do not
attach the source or a local network file.

2) I added a checkbox "Attach source of [image | link] to message" in the
Insert Image and Insert Link dialog. The value of the check box depend on the
nature of the url but the user can overwrite the default behavior by changing
its state. When the use click on the checkbox, the attribute
moz-do-not-send=true|false is added to the embedded object.

Comment 16

[Cavin Song]

Comment on attachment 108264 [details] [diff] [review]
Proposed fix, v1

r=cavin.

Comment 17

[Charles Manske]

This needs more work for the XUL/JS part:
1. JS warning:
+        mozDoNotSend = globalElement.getAttribute(gMOZDONOTSEND);
should be:
+        var mozDoNotSend = globalElement.getAttribute(gMOZDONOTSEND);

2. You should add some code that reads the current setting when user edits an
image or link (e.g., double click to bring up appropriate dialog). Currently,
the new checkbox shows last state, not the state of "moz-do-not-send" set when 
image or link was inserted.

3. Shouldn't there be some UI in prefs so user can set the new pref?

4. I tested the patch and it seems to work, but I found some problems that are
probably not because of this patch, but should be addressed:
1) Lots of asserts! Most common were at :
generate_encodedwords(char * 0x04c702d8, const char * 0x05229298, char 81, char
* 0x05438ef8, int 116, int 0, int 72, int 1) line 280 + 32 bytes
apply_rfc2047_encoding(const char * 0x05436898, int 0, const char * 0x05229298,
int 0, int 72) line 795 + 37 bytes
MIME_EncodeMimePartIIStr(const char * 0x05436898, int 0, const char *
0x05229298, const int 0, const int 72) line 1164 + 25 bytes
nsMimeConverter::EncodeMimePartIIStr_UTF8(nsMimeConverter * const 0x036bcc00,
const char * 0x05436898, int 0, const char * 0x05229298, int 0, const int 72,
char * * 0x0012efac) line 159 + 25 bytes
nsMsgI18NEncodeMimePartIIStr(const char * 0x05436898, int 0, const char *
0x05229298, int 0, int 1) line 399 + 54 bytes
mime_generate_attachment_headers(const char * 0x053b9ac0, const char *
0x00000000, const char * 0x05423da8, const char * 0x0542b5f0, const char *
0x05436580, const char * 0x054365c8, const char * 0x05436898, const char *
0x054366b0, int 0, int 0, const char * 0x05436610, const char * 0x05455b00, int
0, const char * 0x04c703d0, int 0) line 814 + 28 bytes
nsMsgComposeAndSend::PreProcessPart(nsMsgAttachmentHandler * 0x0541ef74,
nsMsgSendPart * 0x053b9690) line 1308 + 115 bytes
nsMsgComposeAndSend::GatherMimeAttachments(nsMsgComposeAndSend * const
0x053fe2c8) line 1149
nsMsgAttachmentHandler::UrlExit(unsigned int 0, const unsigned short *
0x00000000) line 1222 + 41 bytes
FetcherURLDoneCallback(unsigned int 0, const char * 0x052297c8, const char *
0x00000000, int 341, const unsigned short * 0x00000000, void * 0x0541f010) line
479 + 16 bytes
nsURLFetcher::OnStopRequest(nsURLFetcher * const 0x0545dc0c, nsIRequest *
0x0545e230, nsISupports * 0x00000000, unsigned int 0) line 322 + 52 bytes
nsDocumentOpenInfo::OnStopRequest(nsDocumentOpenInfo * const 0x0545e308,
nsIRequest * 0x0545e230, nsISupports * 0x00000000, unsigned int 0) line 257
nsFileChannel::OnStopRequest(nsFileChannel * const 0x0545e23c, nsIRequest *
0x0545e444, nsISupports * 0x00000000, unsigned int 0) line 597
nsOnStopRequestEvent::HandleEvent() line 213

and during sendind of message at:
NS_CheckThreadSafe(void * 0x002c1090, const char * 0x06664524) line 533 + 34 bytes
nsURLFetcher::Release(nsURLFetcher * const 0x0545dc08) line 64 + 76 bytes
nsCOMPtr<nsIInterfaceRequestor>::~nsCOMPtr<nsIInterfaceRequestor>() line 491
nsFileTransport::~nsFileTransport() line 306 + 89 bytes
nsFileTransport::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsFileTransport::Release(nsFileTransport * const 0x0545e440) line 312 + 147 bytes

2) I tested various combinations of including images with and without being 
attached, and links with and without being attached. When there is 1 each of 
both images and links, I crashed frequently (I think the order of them in
the page may be important?) It wasn't 100% reproduceable.

Comment 18

[Jean-Francois Ducarroz]

cmanskey, thank you for your testing. Can you just tell me at which point you
were crashing? during the edition (when open the dilog, close it, etc) or did
you send the message?

Comment 19

[Jean-Francois Ducarroz]

todo list:

1) mailbox url should turn checkbox on by default
2) need to initialize the checkbox when opening dialog
3) check cmanskey warning and crash
4) get ok from UI folk

Comment 20

[Jean-Francois Ducarroz]

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

Comment 21

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2

I have addressed most of cmanske comments: no, we will not have a UI for the
new pref, this is only a power user feature. Also, I wasn't able to reproduce
the crash!

This patch also fix couple other issue I found while doing more intensive test:

1) Editor wasn't giving the list of links to remote file. That was preventing
the user from forcing to attach a remote link.

2) When sending a link to a remote attachment and forcing the source to be
attached, we could not browse this link with mozilla because of a bug in mime.

3) I have changed the pref name to
"mail.compose.dont_attach_source_of_local_network_links"

WARNING: if you want to test this patch, make sure you have also the fix for
118038

Comment 22

[Cavin Song]

Comment on attachment 108817 [details] [diff] [review]
Proposed fix, v2

r=cavin.

Comment 23

[Charles Manske]

I don't see a new version of mailComposeEditorOverlay.xul

Comment 24

[Jean-Francois Ducarroz]

Attachment: New file for patch v2

I forget to include those 2 new files in the previous patch

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 108817 [details] [diff] [review]
Proposed fix, v2

sr=sspitzer on this patch, once you address these issues:

1)

is this code only used by mail?

+	   // Include all Links. Let mail decide which one to send or not
	   nsCOMPtr<nsIDOMHTMLAnchorElement> anchor
(do_QueryInterface(content));
	   if (anchor)

also, a typo in the comment:

-// Include all Links. Let mail decide which one to send or not
+// Include all Links. Let mail decide which ones to send or not

2)


+		   nsCOMPtr<nsIPref> prefs(do_GetService(kPrefCID, &rv));
+		   if (NS_SUCCEEDED(rv) && prefs)

nsIPref iss deprecated and should not be used.
See nsIPrefService and nsIPrefBranch for the new implementations.


3)

your comment says mail.compose.dont_attach_source_of_local_network_links
but your code says dont_attach_source_of_local_network_links

4)

no need to set this to null.

+  nsCOMPtr<nsIURI> base = nsnull;

5)

you forgot "mail.compose" in the pref name.

also, the code isn't windows only, it's just that file:///// is windows only
(but what if I manually did that on linux or mac?)

you should indicate in comment in this .js file (and the code) that you rely on
the fact that network files on windows
are \\foo\bar, and file:///// as urls in mozilla.

+// Set this preference to true do tell mozilla to not attach the source of a
link to a local
+// network file (file://///<network name>/<path>/<file name>). Windows only
+pref ("dont_attach_source_of_local_network_links", false);
+

6)

make sure to get someone on editor to review, as well.

Comment 26

[(not reading, please use [e-mail] instead)]

from an prior comment, this depends on #118038, right?

Comment 27

[Cavin Song]

Comment on attachment 108982 [details] [diff] [review]
New file for patch v2

r=cavin.

Comment 28

[(not reading, please use [e-mail] instead)]

Comment on attachment 108982 [details] [diff] [review]
New file for patch v2

I didn't review the whole patch, because I had some  issues.

1)

>     function SetupMailOverlayHook()
>     {
>       if (!document.firstChild.getAttribute("original_onload"))
>       {
>         document.firstChild.setAttribute("original_onload", document.firstChild.getAttribute("onload"));
>         document.firstChild.setAttribute("onload", "OnLoadOverlay()");
>       }
>     }

why aren't you doing:

addEventListener("load",OnLoadOverlay,true);

can you also explain what the timer is for?

can you screen shot what this UI looks like, so that we can get jglick to
comment?

2)

> <!ENTITY attachImageSource.label         "Attach the source of the image to the message">
> <!ENTITY attachImageSource.accesskey     "s">
> 
> <!ENTITY attachLinkSource.label          "Attach the source of the link to the message">
> <!ENTITY attachLinkSource.accesskey      "s">

I'm not sure if this is the best wording.  Maybe "Attach a copy" instead of
"Attach the source".

Comment 29

[Jean-Francois Ducarroz]

This does not depend on bug 118038 anymore as charley backup his change until
bug 118038 is fixed.

I have asked cmanske to review the editor part

Comment 30

[(not reading, please use [e-mail] instead)]

Comment on attachment 108982 [details] [diff] [review]
New file for patch v2

removing the r= request

Comment 31

[[e-mail]]

>addEventListener("load",OnLoadOverlay,true);

Note: Capturing load event listeners are usually bad, as they also capture load
events on child documents (iframe, browser, editor).

Comment 32

[Jean-Francois Ducarroz]

I will use addEventListener, that works perfectly with it too.
I use a timer to detect when the image/link url has changed. I could listen for
"change" or "keypress" event but that will miss case when the url get changed by
the "Choose File" button. Unfortunately, I could not easely detect when the
chosse file dialog get closed! Therefore I use a time like editor does to update
the image preview.

I'll post soon a new patch as well some dump screen for the UI change...

Comment 33

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v3

Use addEventListener instead of hijacking the onload handler.

Comment 34

[Jean-Francois Ducarroz]

cc'ing sspitzer

Comment 35

[Jean-Francois Ducarroz]

Attachment: UI changes screen dump

Comment 36

[Jean-Francois Ducarroz]

Jennifer, can you review the UI changes: I added a checkbox in both the image
and link properties dialog. The new checkbox is visible only when the dialog is
open from message compose.

Seth suggested to use "Attach a copy" instead of
"Attach the source".

Comment 37

[jglick]

This is only for local file links correct? We don't want to be attaching source 
for webpages/headings/anchors, correct? So would "For links to local files, 
attach a copy of the file to the message" be more accurate? ("For local images, 
attach a copy of the image to the message").

Or, why not just do what the menu item states "Insert a Link", but provide a 
disclaimer note that it might not work for local files. "Note: When creating 
links to local files, be sure the recipient has access to that file from a 
shared network. Otherwise, attach the local file instead."

Comment 38

[Jean-Francois Ducarroz]

This new feature is for remote and local files. By default only the source of
local file is sent/attached but the user has now the possibility to change that.
In some case, is important to be able to send/attach the source of a remote
image/file when this one is not available for every recipient (maybe behind a
firewall). In other case, you don't need to attach the source of a fine on a
local server.

About Jennifer second solution, it could works for links but not for inline images!

Comment 39

[jglick]

If the user wants to attach a file, webpage, image, etc., to a message they 
should use the "Attach" feature. If they just want to insert a link, they 
should use the Link feature (and we should only include the link). Why are we 
trying to duplicate the Attach feature in the Insert Link dialog?  

I'd recommend that Insert Link just insert a link and Attach be used to attach 
files, webpages, images, etc. If desired, a Note as mentioned in c#37 could be 
used to explain why a link might not work.

Comment 40

[Charles Manske]

I agree with jglick's suggestion so we can avoid making the link dialog more
complicated.

Comment 41

[Ktulu]

The target milestone should be changed isn't it ? 
we're on moz 1.3beta now

Comment 42

[Jason Bassford]

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

Comment 43

[Mike Cowperthwaite]

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

Comment 44

[Scott Davis]

What needs to happen on this bug for the fix to be considered for inclusion?
Patches were submitted and reviewed two years ago. Then discussion seems to have
stopped over the issue of whether Insert Link should attach a local file at all.

In the mean time, Windows users have no discoverable way (one workaround was
mentioned in the comments) for inserting a link to a network file. This is
something I want to do on a daily basis within our business. My boss doesn't
want me emailing him 50MB attachments, nor does he want a text description of
how to find it on the network - he wants a link he can click to quickly open the
file. I imagine this would be the case in many other corporate environments.

To me, the argument that an Insert Link dialog should insert a link, rather than
attaching a file, is a strong one. Inserting a link and attaching a file are
orthogonal tasks. The UI has matching orthogonal elements (attach button, drag
to attachments area vs. insert link, drag to compose area). The worst part about
this bug is that it does something counterintuitive and non-orthogonal by
attaching a file when you attempt to insert a link.

If a decision were made which way to move forward, I would be happy to be a part
of working on a patch.

Should we:

1) add hidden pref for power users (existing patch includes this pref) to link
to files instead of attaching them when Insert Link is used 

2) add to UI (per existing patch), adding check box to Insert File dialog, for
when someone is sure they want to link to a network/local file rather than attach.

3) Make Insert Link / Attach truly orthogonal by making them do exactly what
they say. Deal with concerns over privacy & linking to local files by adding
"Are you really sure you know what you're doing" message box when someone adds a
link to a local/network file. Allow D&D to work as per user expectations (at
least on Windows platform): by default, D&D attaches file when dropped anywhere
on compose window per "copy" paradigm. When D&D with modifier key or button,
insert link per "shortcut" paradigm.

Scott

Comment 45

[Arthur]

I think this bug has simply been forgotten. Setting severity to normal and
tentatively asking for blocking new versions.

Comment 46

[Mike Cowperthwaite]

(In reply to comment #44)
> My boss doesn't want me emailing him 50MB attachments, nor does he want a
> text description of how to find it on the network - he wants a link he can
> click to quickly open the file. I imagine this would be the case in many
> other corporate environments.

You can't put the file on an in-house webserver and send him an http: link?  
That *is* the way to make linking work as desired.  A file: link with a 
hardcoded path to a file on a LAN is not only tangled up with privacy issues, 
it's also unreliable (if, for instance, the drive letter mapping is different 
between sender and receiver).

Comment 47

[Scott Davis]

Putting the file on a local webserver, or any webserver, would of course make
linking work, since http:// linking isn't broken in composer. But that's besides
the point. Why should I copy a file to a webserver when it is already on our
file server? That's the main reason to have a central file server - shared file
access.

In our corporate network, common file server directories are mapped to
consistent drive letters, enforced by domain policy, so that company files have
a consistent file path. This has been the case in every company I remember
working for, in fact. For corporate users, it makes complete sense to want to
send a file link to a co-worker, enough so that the absence of this ability in
Thunderbird is a real pain.

Comment 48

[emma]

it would be great to have a fix for this.  I've recently rolled out
Thunderbird/Firefox to 150 users and this is the one problem that we are having
as an organisation with the software.  We previously used Netscape 4.7 and staff
are used to sending links to policy documents etc on the network in internal
emails to save disk space rather than sending files as attachments (file links
worked OK in Netscape and we spent some time educating users to send links
rather than attachments so they are a bit bemused now that this function doesn't
work).  

As with other comments on this bug, we use consistent drive mapping letters on
domain logon so there are consistent file paths to organisationl documents.  I
also agree with other comments that having a UI option to insert a link that
then inserts an attachment is confusing and un-intuitive.  

At the very least, implementing a fix that provides a pref that can be set for
users in corporate environments who expect the software to handle file links
would be great.    

Please, please can someone fix this - it looks like a fix is nearly there, but
it's beyond me to delve in the code.  We have some money as an organisation to
spend on fixing this, so would even be prepared to pay some to implement this as
an extension that we could install.  Can anyone help? cheers, emma

Comment 49

[David :Bienvenu]

taking - I'll try to drive this in.

Comment 50

[David :Bienvenu]

Comment on attachment 109439 [details] [diff] [review]
Proposed fix, v3

Daniel, while I try to get this patch working, I wanted to check that you'd be
OK with the editor parts of it (if not, I'd need to find an other approach)

Comment 51

[Asa Dotzler [:asa]]

we'll certainly consider a reviewd patch but we're not going to block on this
unless someone makes a compelling case. 

Comment 52

[Ian Neal]

Attachment: Unbitrotted version of patch v3.0a

Changes since v3.0
* Made corrections suggested (I think I captured them all anyway)
* Changed from using nsCaseInsensitiveStringComparator to using
LowerCaseEqualsLiteral
* Other bitrotting so it applies

Comment 53

[David :Bienvenu]

I've been trying to get this to work in Thunderbird. I've also un-bitrotted the
patch, and cleaned it up a little, especially redoing the sense of the pref,
which makes the code a little clearer. But I'm having trouble getting the
overlay to work in tbird.

Comment 54

[David :Bienvenu]

mscott's going to look at getting the overlay stuff working in tbird.

Comment 55

[emma]

(In reply to comment #51)
> we'll certainly consider a reviewd patch but we're not going to block on this
> unless someone makes a compelling case. 
Trying to make a compelling case:
There is no point providing an option in the UI to "create a link" to a file if
all it is doing is inserting some text and then attaching the file.
When evaluating the software for use I should have tested this feature more
thoroughly and found that it didn't quite do what it said on the box, BUT the
option shouldn't really be in the UI. Sending links to files is pretty standard
practise on corporate LANs with common drive mountings and this was a feature
that Netscape Mail provided.

Comment 56

[Richard Moore]

What's happening with this? Please can someone try to finish this patch? 

We are having huge problems in our organisation due to our regional users 
(sharing ISDN connections) having to download enormous email messages every 
time they log on to Thunderbird. Everybody in our organisation has been taught 
to send links to files rather than attachments in order to prevent this problem 
from happening, but since we moved to Thunderbird we may as well be sending 
attachments. 

We cannot copy our documents to a web server as this is a hugely inefficient 
use of disk space. The only thing we can afford to change is the email client 
we use, and I know for a fact that other email clients provide the 
functionality we are looking for. I don't want to stop using Thunderbird but I 
don't see how Mozilla can appeal to organisations if this fix isn't 
implemented. 

For a further plea: We are an environmental charity and this is really 
preventing us doing our jobs as efficiently as we could be. A fix for this 
would be so greatly appreciated!

Thanks

Rich

Comment 57

[Arthur]

Asking for Seamonkey checkin as overlay problems seem to affect Thunderbird
only. Adding privacy keyword as by my comment #5. (Yes, it is quite usual to
help someone finding her/his system files by adding a file:/// link to a mail.)

Comment 58

[Robert Kaiser]

SeaMonkey 1.0 Alpha has facutally already happened, even if it's not announced
yet, so that chance is gone anyways. Also minusing for beta as it's just
nice-to-havem but I don't think we would hold the release for it. This doesn't
mean it can't go in, it just means we won't push out the release for this fix.

Comment 59

[Mike Cowperthwaite]

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

Comment 60

[Elmar Ludwig]

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

Comment 61

[Mike Cowperthwaite]

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

Comment 62

[Victor Cisneros]

Having faith that the proposed v3 fix will get checked in soon for Corporate users, even though this bug is four years old, i'm adding myself to the cc list.

Comment 63

[Thomas Bertels]

Would it be possible to remove "local network" from the bug summary, since it's the only thing which prevents the bug 176416 from beeing marked as dupe of this one ?

Comment 64

[Karsten Düsterloh]

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

Comment 65

[Thomas Bertels]

David, perhaps should you consider requesting review from ducarroz or mscott.

Comment 66

[David :Bienvenu]

Comment on attachment 190085 [details] [diff] [review]
Unbitrotted version of patch v3.0a

Daniel, thoughts on the editor part of this patch, i.e., nsHTMLEditor.cpp? Does code other than mailnews use this code?

Comment 67

[Alex Janssen]

I also posted this to 176416, which is close to a clone of this issue.

This should be converted to a request for a interface feature.
I'd like to see it in the T-bird  Tools -> Options -> Composition -> General dialog.
It could be a checkbox that defaults to true as:
  [x] Embed images referenced by URL in outgoing message
If we un-check it, the image is not sent out embedded in the message, only the
URL.

I wonder what the odds are of this attracting the attention of someone who can actually change the code?

Comment 68

[Mike Stockman]

This isn't really an issue of whether to show images from their URLs. This is about file URLs, and whether the mail app should honor the user's intent (insert a file link) or override the user's intent (load the file when the user clearly inserted a URL instead of an attachment).

And it appears to me that people have already submitted patches to fix this behavior. Are these changes going ignored because of a philosophical concern (that we shouldn't honor the user's request for some reason) or for administrative reasons?

This bug causes me no end of trouble on my corporate network. When I forget about this bug, I find Thunderbird attaching a 1.5MB file against my wishes, when I was clearly trying to insert a link to a file on the network that all of my recipients could easily have reached. This slows down the mail, making it painful for users connecting over our VPN, and causes unnecessary wasted resources on the mail server.

Please, can someone review the patches that have been submitted or, if they're bitrotted at this point (no surprise if they are), ask the submitter to update them?

Comment 69

[David :Bienvenu]

Pinging Daniel about the editor part of that patch...

Comment 70

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 190085 [details] [diff] [review]
Unbitrotted version of patch v3.0a

r=[e-mail] for editor part

Comment 71

[Ian Neal]

Attachment: new unbitrotted version v3.1

Changes since v3.0a:
* Unbitrotted patch
* forceToBeAttached set to PR_TRUE instead of testing attributeValue against false as that would always be true anyway.
* tweaked some comments
* necessary TB files altered to make use of the backend changes

Comment 72

[[e-mail]]

Comment on attachment 235600 [details] [diff] [review]
new unbitrotted version v3.1

>         // See if it's an image or an embed
>         if (tagName.EqualsLiteral("img") || tagName.EqualsLiteral("embed"))
>           (*aNodeList)->AppendElement(node);
>         else if (tagName.EqualsLiteral("a"))
>         {
>-          // Only include links if they're links to file: URLs
>+          // Include all Links. Let mail decide which one to send or not
>           nsCOMPtr<nsIDOMHTMLAnchorElement> anchor (do_QueryInterface(content));
>           if (anchor)
>-          {
>-            nsAutoString href;
>-            if (NS_SUCCEEDED(anchor->GetHref(href)))
>-              if (StringBeginsWith(href, NS_LITERAL_STRING("file:"),
>-                                   nsCaseInsensitiveStringComparator()))
>-                (*aNodeList)->AppendElement(node);
>-          }
>+            (*aNodeList)->AppendElement(node);
>         }
The AnchorElement QI only existed to get its href, you should be find merging the link test into the previous block.

>+      clearInterval(gMsgCompTimerID); //stop the timer to prevent conflicting we us
That shouldn't happen, the timer won't fire simultaneously.

>+    function AdjustAttachSourceCheckbox(inputElement)
It might be possible to replace SetRelativeCheckbox with this function,
that way we could be sure that it was called on any url change.

>+      // Does the URL has changed
"Has the URL changed"

>+            var url = /\s*(\S*):\/\/(\S*)/;
>+            var results = inputElement.value.match(url);
>+            if (results && results[1].toLowerCase() == "file")
>+            {
>+              //is it a Windows remote file?
>+              if (results[2].substr(0, 3) == "///")
Can't you do that all in one regexp test i.e.
if (^\s*file:\/\/\/\/\//i.test(url))

>+        gMsgCompAttachSourceElement.checked = (gMsgCompPrevMozDoNotSendAttribute == null || gMsgCompPrevMozDoNotSendAttribute != "true");
Hmm, that's not really accurate, is it? If it's null then it depends on the pref. Maybe you should test for the attribute change first, and if it was null then pretend the value changed and you need to reset the checkbox.

>+      var windowtype = window.opener.document.firstChild.getAttribute("windowtype");
Note: Other editor dialogs use GetCurrentEditor().flags & nsIPlaintextEditor.eEditorMailMask;

>+          switch (document.firstChild.getAttribute("id"))
document.documentElement.id

>+          gMsgCompAttachSourceElement.hidden = false;
Surely this should be false by default anyway?

>+      addEventListener("load", OnLoadOverlay, true);
This should be in the main script block, and not using capturing.

>+<!ENTITY attachImageSource.label         "Attach the source of the image to the message">
Shouldn't this say "Attach this image to the message"?

>+                if (!nsCRT::strncasecmp(urlSpec.get(), "file://///", 10))
You should use StringBeginsWith here.

>+  PRBool        mozDoNotSendIsFalse;
I don't understand the changes related to this member variable.

Comment 73

[Ian Neal]

Attachment: Revised patch v3.2

Changes since v3.1:
* Removed unneeded AnchorElement QI.
* Stopped using timer to test for input, replaces SetRelativeCheckbox function instead for mailnews editors as that function is not used for mailnews.
* Streamlined tests for networked files as suggested.
* Test for attribute change first and use the same code for both that and URL change to reset checkbox.
* Move addEventListener to main script block and stopped using capture in it.
* Tweaked entity label for images.
* Removed unneeded changes from nsMsgComposeAndSend::ProcessMultipartRelated.

Comment 74

[[e-mail]]

Comment on attachment 235965 [details] [diff] [review]
Revised patch v3.2

>+<!ENTITY attachLinkSource.label          "Attach the source of this link to the message">
Nice :-)

>+          // the rule should be in sync with to the one in nsMsgComposeAndSend::GetEmbeddedObjectInfo
Except I don't think it's quite right. The rule in GetEmbeddedObjectInfo appears to be correct, as follows:
If the image/link isn't a file, then attach an image but not a link.
If the file does not exist, then don't try to attach it.
Otherwise a link to a network file follows the pref.

I'm not too bothered if you don't check that the file exists, in which case your code is correct for images, but for links, you should try to attach a local file, unless it appears to be on the local network. Marking r- because I want to test out your next patch to see if it matches my expectations ;-)

>+                if (!StringBeginsWith(urlSpec, NS_LITERAL_CSTRING("file://///")))
I think your ! is spurious.

Comment 75

[Ian Neal]

Attachment: The no exclamation patch v3.3

Changes since v3.2:
* Got rid of the spurious !
* Renamed value to attach.
* If the "file" is not a network one, default attach to true.

Comment 76

[[e-mail]]

Comment on attachment 236630 [details] [diff] [review]
The no exclamation patch v3.3

No, this isn't working right either. You're now defaulting to attaching all links, even to web pages, instead of just local files.

Comment 77

[Ian Neal]

Attachment: Check for local too patch v3.4

Changes since v3.3:
* Checks link, defaults are: if network file - look at pref, if non-network file - attach, if not file at all - don't attach

Comment 78

[[e-mail]]

Comment on attachment 236657 [details] [diff] [review]
Check for local too patch v3.4

>       <radio id = "noAltTextRadio" 
>         label   = "&noAltText.label;"
>         accesskey = "&noAltText.accessKey;"
>         oncommand = "SetAltTextDisabled(true);"/>
>     </radiogroup>
>+    <!-- mail compose will insert custom item here defined in mailComposeEditorOverlay.xul -->
>   </vbox>
I've just realised that this isn't where the "URL is relative to page location" checkbox is. Is there any way you can put it there instead?

Comment 79

[Ian Neal]

Attachment: Checkbox by MakeRelative patch v3.5 (Checked into trunk)

Changes since v3.4
* Moved the attach to mail checkbox on the insert image dialog to the same place as the make relative checkbox.

Carrying forward r+ from daniel and neil and requesting sr.

Comment 80

[David :Bienvenu]

Comment on attachment 236890 [details] [diff] [review]
Checkbox by MakeRelative patch v3.5 (Checked into trunk)

+<!ENTITY attachImageSource.label         "Attach this image to the message">
+<!ENTITY attachImageSource.accesskey     "s">
+
+<!ENTITY attachLinkSource.label          "Attach the source of this link to the message">
+<!ENTITY attachLinkSource.accesskey      "s">


should these have the same accessKey?

Comment 81

[David :Bienvenu]

Comment on attachment 236890 [details] [diff] [review]
Checkbox by MakeRelative patch v3.5 (Checked into trunk)

sr=bienvenu, modulo the question about the access keys being the same.

Comment 82

[Ian Neal]

(In reply to comment #80)
> (From update of attachment 236890 [details] [diff] [review] [edit])
> +<!ENTITY attachImageSource.label         "Attach this image to the message">
> +<!ENTITY attachImageSource.accesskey     "s">
> +
> +<!ENTITY attachLinkSource.label          "Attach the source of this link to
> the message">
> +<!ENTITY attachLinkSource.accesskey      "s">
> 
> 
> should these have the same accessKey?
> 
They can be the same because they are for overlays to two different dialog boxes.

Comment 83

[Ian Neal]

Comment on attachment 236890 [details] [diff] [review]
Checkbox by MakeRelative patch v3.5 (Checked into trunk)

Checking in (trunk)
editor/libeditor/html/nsHTMLEditor.cpp;
new revision: 1.544; previous revision: 1.543
editor/ui/dialogs/content/EdImageOverlay.xul;
new revision: 1.11; previous revision: 1.10
editor/ui/dialogs/content/EdLinkProps.xul;
new revision: 1.82; previous revision: 1.81
mail/components/compose/jar.mn;
new revision: 1.21; previous revision: 1.20
mail/locales/jar.mn;
new revision: 1.42; previous revision: 1.41
mail/locales/en-US/chrome/messenger/messengercompose/mailComposeEditorOverlay.dtd;
initial revision: 1.1
mailnews/jar.mn;
new revision: 1.113; previous revision: 1.112
mailnews/mailnews.js;
new revision: 3.275; previous revision: 3.274
mailnews/base/resources/content/contents.rdf;
new revision: 1.41; previous revision: 1.40
mailnews/compose/src/nsMsgSend.cpp;
new revision: 1.382; previous revision: 1.381
mailnews/compose/resources/content/mailComposeEditorOverlay.xul;
initial revision: 1.1
mailnews/compose/resources/locale/en-US/mailComposeEditorOverlay.dtd;
initial revision: 1.1
done

Comment 84

[Ian Neal]

Comment on attachment 236890 [details] [diff] [review]
Checkbox by MakeRelative patch v3.5 (Checked into trunk)

Asking for approval to go into 1.8.1 branch, might need to make for a few days first though.

Comment 85

[Mike Schroepfer]

Does this affect both FF and TBird - it is a large patch so we are concerned about risk this close (within a week) of RC1.

Comment 86

[Ian Neal]

(In reply to comment #85)
> Does this affect both FF and TBird - it is a large patch so we are concerned
> about risk this close (within a week) of RC1.
> 
Just TBird as FF does not make use of any of this code AFAIK

Comment 87

[Mike Connor [:mconnor]]

Comment on attachment 236890 [details] [diff] [review]
Checkbox by MakeRelative patch v3.5 (Checked into trunk)

a=mconnor on behalf of drivers for the small non-mailnews code, looks like only mailnews code calls this, so it should be ok from our perspective.

Comment 88

[Ian Neal]

Attachment: forceToBeAttached fix (Checked into trunk)

This patch:
* Fixes the fact we only want forceToBeAttached to be true when moz_do_not_send exists and is set to false.

Comment 89

[Ian Neal]

Comment on attachment 238007 [details] [diff] [review]
forceToBeAttached fix (Checked into trunk)

Checking in (trunk)
nsMsgSend.cpp;
new revision: 1.383; previous revision: 1.382
done

Comment 90

[Mike Cowperthwaite]

In testing this, it seems to be working pretty well (TB 3a1-0915).  Thanks, Ian; it's great to see a big old bug like this finally squashed.

One slight thing, which may not be a problem: if inserting an image, and I select the 'Attach' checkbox and then deselect it again, the message goes out with -moz-do-not-send=false in the <img> tag; but if I don't touch the checkbox at all, there is no -moz-do-not-send attribute at all.  Net result is the same, and it's not something that causes compatibility issues.

Good to know:  -moz-do-not-send=true  stays in place when forwarding-inline, so the image continues to remain remote.  However, if the message sender isn't in one's address book (or if whitelisting is turned off) the image will be blocked (good); but forward-inline of that image will automatically load the image in the compose window, ignoring the Block setting -- this is bug 263345.

Comment 91

[Ian Neal]

Attachment: Help patch v3.5 (Checked into trunk)

This patch:
* Updates suite help to reflect changes implemented in this bug.

Comment 92

[Ian Neal]

Comment on attachment 238940 [details] [diff] [review]
Help patch v3.5 (Checked into trunk)

Checking in (trunk)
composer_help.xhtml;
new revision: 1.34; previous revision: 1.33
done

Comment 93

[David :Bienvenu]

I think we should take this for 2.0, so we might as well get it before the beta to shake out any particular problems.

Comment 94

[Scott MacGregor]

David, I don't think we can take this for 2.0, it looks like it makes changes to editor which is code compiled by Firefox. We can no longer take changes that effect Firefox on the branch anymore. 

Comment 95

[David :Bienvenu]

the editor changes have approval 1.8.1, though I don't know if they were checked in, and if not, if it's too late to check them in...

Comment 96

[Ian Neal]

(In reply to comment #94)
> David, I don't think we can take this for 2.0, it looks like it makes changes
> to editor which is code compiled by Firefox. We can no longer take changes that
> effect Firefox on the branch anymore. 
> 
The parts of editor being patched are not used by Firefox. GetEmbeddedObjects in nsHTMLEditor.cpp is only used by nsMsgCompose.cpp and nsMsgSend.cpp. So all that code is for TB/mailnews only, approval was asked for 1.8.1 as it does sit in core.

Comment 97

[Ian Neal]

Comment on attachment 236890 [details] [diff] [review]
Checkbox by MakeRelative patch v3.5 (Checked into trunk)

Requesting a= for 1.8.1.1 - would like to get this into branch before TB2 but after FF2. This code is compiled for Firefox but not used by it, only by TB and SM.

Comment 98

[Ian Neal]

Scott, I'd like to get the non-core string changes in now, can you a= that?
Then once FF2 is out get the core changes and other non-core changes in.

Comment 99

[Scott MacGregor]

That's a great idea Ian. Do you want to attach a patch that has just the locale changes and i'll approve that? Good thinking.

Comment 100

[Ian Neal]

Attachment: Locale changes for branch only v0.1 (Checked into 1.8.1 branch)

Locale only changes for branch approval/checkin.

Comment 101

[Ian Neal]

Comment on attachment 242118 [details] [diff] [review]
Locale changes for branch only v0.1 (Checked into 1.8.1 branch)

Checking in (1.8.1 branch)
extensions/help/resources/locale/en-US/composer_help.xhtml;
new revision: 1.31.6.2; previous revision: 1.31.6.1
mail/locales/jar.mn;
new revision: 1.35.2.6; previous revision: 1.35.2.5
mail/locales/en-US/chrome/messenger/messengercompose/mailComposeEditorOverlay.dtd;
new revision: 1.1.4.1; previous revision: 1.1
mailnews/jar.mn;
new revision: 1.99.2.11; previous revision: 1.99.2.10
mailnews/compose/resources/locale/en-US/mailComposeEditorOverlay.dtd;
new revision: 1.1.4.1; previous revision: 1.1
done

Comment 102

[Ian Neal]

Attachment: Non-locale changes (inc core) for branch patch v0.2 (Checked into 1.8.1 branch)

Requesting a= for TB2, SM1.1b and 1.8.1.1 core, carrying forward r= and sr=
As mentioned before, some of the code these changes effect is compiled by Firefox but not actually used, it is only used by TB and SM.

Comment 103

[Jay Patel [:jay]]

glazman or anyone else: Are we sure that the changes to nsHTMLEditor.cpp will not affect midas?

Comment 104

[Scott MacGregor]

Comment on attachment 242121 [details] [diff] [review]
Non-locale changes (inc core) for branch patch v0.2 (Checked into 1.8.1 branch)

I talked this over with Dan who read the helpful e-mail Neil sent to drivers.

Comment 105

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 242121 [details] [diff] [review]
Non-locale changes (inc core) for branch patch v0.2 (Checked into 1.8.1 branch)

a=me for sm1.1 (approval requested by iann)

Comment 106

[Ian Neal]

Comment on attachment 242121 [details] [diff] [review]
Non-locale changes (inc core) for branch patch v0.2 (Checked into 1.8.1 branch)

Checking in (1.8.1 branch)
editor/libeditor/html/nsHTMLEditor.cpp;
new revision: 1.529.2.9; previous revision: 1.529.2.8
editor/ui/dialogs/content/EdImageOverlay.xul;
new revision: 1.10.28.1; previous revision: 1.10
editor/ui/dialogs/content/EdLinkProps.xul;
new revision: 1.81.28.1; previous revision: 1.81
mail/components/compose/jar.mn;
new revision: 1.18.4.1; previous revision: 1.18
mailnews/jar.mn;
new revision: 1.99.2.15; previous revision: 1.99.2.14
mailnews/mailnews.js;
new revision: 3.249.2.29; previous revision: 3.249.2.28
mailnews/base/resources/content/contents.rdf;
new revision: 1.40.4.1; previous revision: 1.40
mailnews/compose/resources/content/mailComposeEditorOverlay.xul;
new revision: 1.1.4.1; previous revision: 1.1
mailnews/compose/src/nsMsgSend.cpp;
new revision: 1.374.2.5; previous revision: 1.374.2.4
done

Comment 107

[Rob Campbell [:rc] (:robcee)]

verified non-existence of UI changes in 20061109 on Mac. Loaded latest update, verified UI additions in link properties and image link properties.

Comment 108

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #107)
> verified non-existence of UI changes in 20061109 on Mac. Loaded latest update,
> verified UI additions in link properties and image link properties.
> 

should have included build id of 20061129 for verified version of Thunderbird.