609992 - Attachment header <fieldset> in the wrong spot for inline text attachments

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jim Porter (:squib)]

Attachment: The problem

When libmime emits inline attachments, it first adds a <fieldset> for the header. However, when emitting inline text attachments, the <fieldset> gets added slightly too late (after writing out the wrapper <div>/<pre> tags). As a result, the header for inline text attachments is a different size from inline image attachments. Aside from looking strange, the inline text version is also generally smaller, making it harder to read.

Comment 1

[Jim Porter (:squib)]

Attachment: Partial fix

This fixes the issue with plain text attachments, but I bet the problem exists with other text attachments too.

Comment 2

[Jim Porter (:squib)]

Attachment: Do the same for HTML attachments

This doesn't have any visual effect, but HTML attachments now put the fieldset/legend outside of the moz-text-html div, just like plain text attachments.

:bienvenu, does this seem reasonable, and are there other places this needs to be done (it doesn't seem like it)? Also, do I need tests for this?

Comment 3

[David :Bienvenu]

(In reply to comment #2)

> 
> :bienvenu, does this seem reasonable, and are there other places this needs to
> be done (it doesn't seem like it)? Also, do I need tests for this?

This seems reasonable. I don't know of any other places this needs to be done. I think this is a minor enough change - do we have any tests for this code that could be tweaked to test this change?

Comment 4

[Jim Porter (:squib)]

Nothing's jumping out in the way of tests. I assume they'd be in mailnews/mime/test/unit, but there doesn't seem to be anything that actually checks the formatted message. It probably wouldn't be a bad idea to have something like that, but that's a potentially big project.

Comment 5

[Jim Porter (:squib)]

Comment on attachment 504898 [details] [diff] [review]
Do the same for HTML attachments

Since I can't find any related tests that I could build upon to test this, I'm asking for review. If we *do* need tests, let me know.

Comment 6

[David :Bienvenu]

Comment on attachment 504898 [details] [diff] [review]
Do the same for HTML attachments

ok, thx, I ran with this patch earlier, and it looked good.

Comment 7

[Jim Porter (:squib)]

Hooray, checkin-needed time!

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Committed as http://hg.mozilla.org/comm-central/rev/66953e3f5a21