Closed Bug 525955 Opened 10 years ago Closed 10 years ago

Images added to an HTML message body now get an incorrect "attachment" disposition by default

Categories

(MailNews Core :: Composition, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.1a1

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

Details

Attachments

(1 file, 1 obsolete file)

With bug 65794, the default content disposition was changed to "attachment" and is applied to all outgoing messages. This has the side effect that also images included into the message body using Insert > Image receive an "attachment" disposition by default, which is wrong. While Thunderbird and SeaMonkey ignore the disposition for content within an HTML part, some other clients (including web mail systems) may not and omit the image from from the display as they are advised to treat it as an attachment. So, this is a conflict situation between content-disposition versus content-id referencing.

Steps to reproduce:
1) ensure that mail.content_disposition_type = 1 (default);
2) compose a new HTML message;
3) attach an image file using the Attach button;
4) insert the same image using Insert > Image;
5) send or save as draft, inspect the encoded message.

Actual result:
- both attached and inserted image have "Content-Disposition: attachment".

Expected result:
- the image explicitly attached has "Content-Disposition: attachment",
- the image within the message has "Content-Disposition: inline".

To resolve the issue while waiting for a more comprehensive fix in bug 452092, the content disposition should be "inline" for content which is identified as part of the message body. Since we are in full control of the message encoding during composition, it is safe to assume that only content which is inserted by either Insert > Image, drag-and-drop, or copy-and-paste will have both a name and a content-id reference. This is the case to be covered here. Images quoted in replies don't have a name and receive "inline" disposition without regard to the preference setting with the current code.
Attached patch Possible patch (obsolete) — Splinter Review
This patch adds another case to mime_generate_attachment_headers() where #1
we have a name, and #2 we also have a content id specified, in which case the setting of mail.content_disposition_type is ignored and "inline" selected.
This combination should only occur for content which is referred to in an HTML part of our composition and would serve as a good workaround for the 1.9.1 branch to avoid possible conflicts with other mail clients.
Attachment #409745 - Flags: superreview?(neil)
Attachment #409745 - Flags: review?(bienvenu)
Unfortunately name, and filename gets dropped when you "edit as new" on a saved template. (unless you re-insert the image)
The only way you could be sure it was inline would be to compare the CID: <part x.y.z...  to the content_ID: <partx.y.z...
Yes, I've noticed that on a reply as well, but that has been before already. Removing the name of an attachment with a content-ID also removes the content disposition, which should then be implied as "inline". This code needs a major cleanup, the motivation here is to cover a specific well-defined case where a freshly inserted image gets an explicit wrong content disposition and may cause issues on the receiving side. So, it's a very limited but safe fix for the sake of being feasible for the upcoming release.
Agreed.
If I had regular discourse in html with a Gmail user I would definitely toggle the default pref. That's pretty much undiscoverable to the new user.
David, Neil: I was hoping to get this into a TB 3.0 RC2 (if it's coming) or at least into the TB/SM .1 updates, do you have any opinion on the patch so far?
I don't think we're going to accept this for 3.0 at this point.  3.1 is possible.
Comment on attachment 409745 [details] [diff] [review]
Possible patch

Seems to work - could use a comment in the code to explain why we're doing this.
Attachment #409745 - Flags: review?(bienvenu) → review+
Same patch as before, now with a comment added as requested.
Carrying forward r=bienvenu from attachment 409745 [details] [diff] [review].
Assignee: nobody → rsx11m.pub
Attachment #409745 - Attachment is obsolete: true
Attachment #416938 - Flags: superreview?(neil)
Attachment #416938 - Flags: review+
Attachment #409745 - Flags: superreview?(neil)
Comment on attachment 416938 [details] [diff] [review]
Proposed patch (v2)

I tried and failed to think of an easy way to avoid the pref read.
Attachment #416938 - Flags: superreview?(neil) → superreview+
Thanks for the reviews, push for comm-central please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
(In reply to comment #9)
> I tried and failed to think of an easy way to avoid the pref read.

There shouldn't be much of a performance penalty, and I'd assume that any solution for bug 452092 will have to rework this part substantially anyway,
thus it's hopefully ok.
Checked in: http://hg.mozilla.org/comm-central/rev/af7faefb7cb7
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → Thunderbird 3.1a1
Duplicate of this bug: 539846
You need to log in before you can comment on or make changes to this bug.