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

RESOLVED FIXED in Thunderbird 3.3a3

Status

MailNews Core
MIME
--
trivial
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

Trunk
Thunderbird 3.3a3
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 488550 [details]
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.
(Assignee)

Updated

7 years ago
Component: Message Reader UI → MIME
Product: Thunderbird → MailNews Core
QA Contact: message-reader → mime
(Assignee)

Comment 1

7 years ago
Created attachment 488554 [details] [diff] [review]
Partial fix

This fixes the issue with plain text attachments, but I bet the problem exists with other text attachments too.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 years ago
Created attachment 504898 [details] [diff] [review]
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?
Attachment #488554 - Attachment is obsolete: true
Attachment #504898 - Flags: feedback?(bienvenu)

Comment 3

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

Updated

7 years ago
Attachment #504898 - Flags: feedback?(bienvenu) → feedback+
(Assignee)

Comment 4

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

Comment 5

7 years ago
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.
Attachment #504898 - Flags: review?(bienvenu)

Comment 6

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

Comment 7

7 years ago
Hooray, checkin-needed time!
Keywords: checkin-needed
Committed as http://hg.mozilla.org/comm-central/rev/66953e3f5a21
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
You need to log in before you can comment on or make changes to this bug.