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

VERIFIED FIXED

Status

defect
VERIFIED FIXED
18 years ago
11 years ago

People

(Reporter: arakeis, Assigned: iann_bugzilla)

Tracking

(Depends on 1 bug, Blocks 1 bug, 4 keywords)

Dependency tree / graph
Bug Flags:
blocking1.8b2 -
blocking1.8b5 -
blocking1.8.1.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 8 obsolete attachments)

12.33 KB, patch
cavin
: review+
Details | Diff | Splinter Review
49.07 KB, image/jpeg
Details
29.97 KB, patch
iann_bugzilla
: review+
Bienvenu
: superreview+
mconnor
: approval1.8.1+
Details | Diff | Splinter Review
1.15 KB, patch
neil
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
3.02 KB, patch
neil
: review+
Details | Diff | Splinter Review
9.55 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: superreview+
Details | Diff | Splinter Review
24.23 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: superreview+
Details | Diff | Splinter Review
Reporter

Description

18 years ago
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
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.
Severity: normal → enhancement
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Future
Reporter

Comment 2

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

Comment 4

18 years ago
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

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

Updated

18 years ago
Keywords: privacy
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.
Target Milestone: Future → mozilla1.2alpha
Reporter

Comment 7

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

OS: Windows 2000 → All
Hardware: PC → All

Comment 8

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

Comment 9

18 years ago
Sad that correction of a incorrect and secret behaviour is considered as a
enhancement...
Assignee

Comment 10

18 years ago
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.
Assignee

Comment 11

18 years ago
Separate bug as mentioned in comment 10 is bug 135830
this is similar but different that bug 128693
Target Milestone: mozilla1.2alpha → mozilla1.1beta
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.
Target Milestone: mozilla1.1beta → Future
*** Bug 128693 has been marked as a duplicate of this bug. ***
Depends on: 135830
Whiteboard: have fix
Target Milestone: Future → mozilla1.3beta
Posted patch Proposed fix, v1 (obsolete) — Splinter Review
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

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

r=cavin.
Attachment #108264 - Flags: review+

Comment 17

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

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?
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
*** Bug 177890 has been marked as a duplicate of this bug. ***
Depends on: 118038
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
Attachment #108264 - Attachment is obsolete: true

Comment 22

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

r=cavin.
Attachment #108817 - Flags: review+
No longer depends on: 118038

Comment 23

17 years ago
I don't see a new version of mailComposeEditorOverlay.xul
Posted patch New file for patch v2 (obsolete) — Splinter Review
I forget to include those 2 new files in the previous patch
Attachment #108982 - Flags: review?(cavin)
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.
from an prior comment, this depends on #118038, right?
Depends on: 118038

Comment 27

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

r=cavin.
Attachment #108982 - Flags: review?(cavin) → review+
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".
Attachment #108982 - Flags: superreview-
Attachment #108982 - Flags: review?(cavin)
Attachment #108982 - Flags: review+
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
No longer depends on: 118038
Comment on attachment 108982 [details] [diff] [review]
New file for patch v2

removing the r= request
Attachment #108982 - Flags: review?(cavin)
>addEventListener("load",OnLoadOverlay,true);

Note: Capturing load event listeners are usually bad, as they also capture load
events on child documents (iframe, browser, editor).
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...
Posted patch Proposed fix, v3 (obsolete) — Splinter Review
Use addEventListener instead of hijacking the onload handler.
Attachment #108982 - Attachment is obsolete: true
cc'ing sspitzer
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

17 years ago
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."
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

17 years ago
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

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

Comment 41

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

Comment 42

16 years ago
*** Bug 162802 has been marked as a duplicate of this bug. ***
*** Bug 220772 has been marked as a duplicate of this bug. ***
Product: MailNews → Core

Comment 44

14 years ago
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

14 years ago
I think this bug has simply been forgotten. Setting severity to normal and
tentatively asking for blocking new versions.
Severity: enhancement → normal
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
Target Milestone: mozilla1.3beta → ---
(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

14 years ago
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

14 years ago
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



Updated

14 years ago
Flags: blocking1.8b2? → blocking1.8b2-

Updated

14 years ago
Flags: blocking-aviary1.1? → blocking1.8b4?
Whiteboard: have fix

Comment 49

14 years ago
taking - I'll try to drive this in.
Assignee: ducarroz → bienvenu
Status: ASSIGNED → NEW

Comment 50

14 years ago
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)
Attachment #109439 - Flags: review?(daniel)
we'll certainly consider a reviewd patch but we're not going to block on this
unless someone makes a compelling case. 
Flags: blocking1.8b4? → blocking1.8b4-
Assignee

Comment 52

14 years ago
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

14 years ago
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

14 years ago
mscott's going to look at getting the overlay stuff working in tbird.
Assignee: bienvenu → mscott

Comment 55

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

14 years ago
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

14 years ago
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.)
Flags: blocking-seamonkey1.0b?
Flags: blocking-seamonkey1.0a?

Comment 58

14 years ago
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.
Flags: blocking-seamonkey1.0b?
Flags: blocking-seamonkey1.0b-
Flags: blocking-seamonkey1.0a?
Flags: blocking-seamonkey1.0a-
*** Bug 315868 has been marked as a duplicate of this bug. ***

Comment 60

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

Updated

14 years ago
Flags: blocking-thunderbird2?
*** Bug 327857 has been marked as a duplicate of this bug. ***

Comment 62

13 years ago
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

13 years ago
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 ?
Reporter

Updated

13 years ago
Summary: Inserting a link to a local network file in a message insert the physical file ! → Inserting a link to a network file in a message insert the physical file !
*** Bug 176416 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Summary: Inserting a link to a network file in a message insert the physical file ! → Inserting a link to a [network][image] file into a message inserts the physical file!

Comment 65

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

Updated

13 years ago
Attachment #109439 - Attachment is obsolete: true
Attachment #109439 - Flags: review?(daniel)

Comment 66

13 years ago
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?
Attachment #190085 - Flags: review?(daniel)

Comment 67

13 years ago
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

13 years ago
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

13 years ago
Pinging Daniel about the editor part of that patch...
Comment on attachment 190085 [details] [diff] [review]
Unbitrotted version of patch v3.0a

r=daniel@glazman.org for editor part
Attachment #190085 - Flags: review?(daniel) → review+
Assignee

Comment 71

13 years ago
Posted patch new unbitrotted version v3.1 (obsolete) — Splinter Review
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
Assignee: mscott → iann_bugzilla
Attachment #190085 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #235600 - Flags: review?(neil)
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.
Attachment #235600 - Flags: review?(neil) → review-
Assignee

Comment 73

13 years ago
Posted patch Revised patch v3.2 (obsolete) — Splinter Review
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.
Attachment #235600 - Attachment is obsolete: true
Attachment #235965 - Flags: review?(neil)
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.
Attachment #235965 - Flags: review?(neil) → review-
Assignee

Comment 75

13 years ago
Posted patch The no exclamation patch v3.3 (obsolete) — Splinter Review
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.
Attachment #235965 - Attachment is obsolete: true
Attachment #236630 - Flags: review?(neil)
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.
Attachment #236630 - Flags: review?(neil) → review+
Assignee

Comment 77

13 years ago
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
Attachment #236630 - Attachment is obsolete: true
Attachment #236657 - Flags: review?(neil)
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?
Attachment #236657 - Flags: review?(neil) → review+
Assignee

Comment 79

13 years ago
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.
Attachment #236657 - Attachment is obsolete: true
Attachment #236890 - Flags: superreview?(bienvenu)
Attachment #236890 - Flags: review+

Comment 80

13 years ago
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

13 years ago
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.
Attachment #236890 - Flags: superreview?(bienvenu) → superreview+
Assignee

Comment 82

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

Comment 83

13 years ago
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
Attachment #236890 - Attachment description: Checkbox by MakeRelative patch v3.5 → Checkbox by MakeRelative patch v3.5 (Checked into trunk)
Assignee

Comment 84

13 years ago
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.
Attachment #236890 - Flags: approval1.8.1?
Attachment #236890 - Flags: approval-thunderbird2?
Assignee

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

Comment 85

13 years ago
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.
Assignee

Comment 86

13 years ago
(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 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.
Attachment #236890 - Flags: approval1.8.1? → approval1.8.1+
Blocks: 352317
Assignee

Comment 88

13 years ago
This patch:
* Fixes the fact we only want forceToBeAttached to be true when moz_do_not_send exists and is set to false.
Attachment #238007 - Flags: superreview?(bienvenu)
Attachment #238007 - Flags: review?(neil)

Updated

13 years ago
Attachment #238007 - Flags: superreview?(bienvenu) → superreview+
Blocks: 352341

Updated

13 years ago
Attachment #238007 - Flags: review?(neil) → review+
Assignee

Comment 89

13 years ago
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
Attachment #238007 - Attachment description: forceToBeAttached fix → forceToBeAttached fix (Checked into trunk)
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.
Assignee

Updated

13 years ago
Blocks: 353086
Assignee

Comment 91

13 years ago
This patch:
* Updates suite help to reflect changes implemented in this bug.
Attachment #238940 - Flags: review?(neil)

Updated

13 years ago
Attachment #238940 - Flags: review?(neil) → review+
Blocks: 353186
Assignee

Updated

13 years ago
No longer blocks: 353186
Assignee

Comment 92

13 years ago
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
Attachment #238940 - Attachment description: Help patch v3.5 → Help patch v3.5 (Checked into trunk)

Comment 93

13 years ago
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

13 years ago
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

13 years ago
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...
Assignee

Comment 96

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

Comment 97

13 years ago
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.
Attachment #236890 - Flags: approval1.8.1.1?
Assignee

Comment 98

13 years ago
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

13 years ago
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.
Assignee

Comment 100

13 years ago
Locale only changes for branch approval/checkin.
Attachment #242118 - Flags: superreview+
Attachment #242118 - Flags: review+
Attachment #242118 - Flags: approval-thunderbird2?

Updated

13 years ago
Attachment #242118 - Flags: approval-thunderbird2? → approval-thunderbird2+
Assignee

Comment 101

13 years ago
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
Attachment #242118 - Attachment description: Locale changes for branch only v0.1 → Locale changes for branch only v0.1 (Checked into 1.8.1 branch)
Assignee

Updated

13 years ago
Attachment #236890 - Flags: approval1.8.1.1?
Attachment #236890 - Flags: approval-thunderbird2?
Assignee

Comment 102

13 years ago
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.
Attachment #242121 - Flags: superreview+
Attachment #242121 - Flags: review+
Attachment #242121 - Flags: approval1.8.1.1?
Attachment #242121 - Flags: approval-thunderbird2?
Assignee

Updated

13 years ago
Flags: blocking1.8.1.1?

Comment 103

13 years ago
glazman or anyone else: Are we sure that the changes to nsHTMLEditor.cpp will not affect midas?
Flags: blocking1.8.1.1? → blocking1.8.1.1-

Comment 104

13 years ago
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.
Attachment #242121 - Flags: approval1.8.1.1?
Attachment #242121 - Flags: approval1.8.1.1+
Attachment #242121 - Flags: approval-thunderbird2?
Attachment #242121 - Flags: approval-thunderbird2+
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)
Assignee

Comment 106

13 years ago
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
Attachment #242121 - Attachment description: Non-locale changes (inc core) for branch patch v0.2 → Non-locale changes (inc core) for branch patch v0.2 (Checked into 1.8.1 branch)
Assignee

Updated

13 years ago
Status: RESOLVED → VERIFIED
verified non-existence of UI changes in 20061109 on Mac. Loaded latest update, verified UI additions in link properties and image link properties.
Keywords: verified1.8.1.1
(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.

Updated

13 years ago
Flags: blocking-thunderbird2?

Updated

13 years ago
Duplicate of this bug: 366492

Updated

13 years ago
Duplicate of this bug: 372435
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.