1.29 KB, text/plain
3.27 KB, patch
|Details | Diff | Splinter Review|
2.92 KB, patch
|Details | Diff | Splinter Review|
if you have display body as original html the message is blank. The alternate html file or body doesn't show. I can attach email later. Content-Transfer-Encoding: binary Content-Type: multipart/alternative; boundary="_----------=_1268324732523914" This is a multi-part message in MIME format. --_----------=_1268324732523914 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-Type: text/plain --_----------=_1268324732523914 Content-Disposition: inline; filename="message.html" Content-Transfer-Encoding: quoted-printable Content-Type: text/html; name="message.html" --_----------=_1268324732523914--
Created attachment 431896 [details] example of bug typical craigslist.org email set display body as orig html clear view attachments inline drag message to local folder and view
Phil, do you think this is a regression? Which version are you running?
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100221 Shredder/3.2a1pre I don't know if it's a regression. Unfortunately it can be fixed by the universal mime cure-all of turning on 'display attachments inline', so that may have kept users from reporting it sooner.
(In reply to comment #1) > example of bug > --_----------=_1268324732523914 > Content-Disposition: inline; filename="message.html" > Content-Transfer-Encoding: quoted-printable > Content-Type: text/html; name="message.html"> Checked with Tb 3.0.3(Win-XP). (1) If changed to next, HTML part is displayed as expected. > --_----------=_1268324732523914 > Content-Disposition: inline > Content-Transfer-Encoding: quoted-printable > Content-Type: text/html (2) If original(with filename, name), HTML part is displayed by View/Display Attachments Inline=On. Probably same as old/known problem with multipart/mixed. If name and/or filename is specified for text part used as mail body, Tb considers the part as attachment instead of mail body, then the text part is not displayed. However, the text part is part which is considered mail body first, (first part of mixed. same on text/html part of alternative,Original HTML) the non-displayed text part is not displayed at attachment pane.
Old/known bug is Bug 182627. Problem occurs even on simplest text/plain mail. > Bug 182627 Message body is not displayed for some messages if view attachments inline is false > (name in Content-Type/filename in Content-Disposition for mail-body/mail-body-part > forces "attachment" for Tb even though real mail body)
I say bump this since setting the view attachments inline is a performance hit on imap. Turning off 'view attachments...' is a nice feature that shouldn't be broken. David I may try working on this can you tell me if this is mime code issue or a display issue or some other problem.
I'm thinking if disposition is inline and text/plain or text/html the part should be displayed and if the disposition is 'attachment' then it should be shown in attachment pane and displayed if display attachments inline is set.
Phil, I figured out the issue, and am looking at a fix. But I'm happy to give you what I have so far and you can improve on it. Basically, the code is doing this on purpose, and doesn't have enough information to even special case the first body part.
Be my guest. The fix appeared it might be a bit involved for me since it affected local folders and not just imap. If you don't think you can squeeze it in your schedule then post your what you have so far.
Created attachment 432224 [details] [diff] [review] starting point for a fix This shows you why the last couple messages in the test folder fail to display anything for a body, and fixes them. It doesn't fix the display of multiple parts in some of the other messages, if I'm understanding the issue correctly. Phil, thx for looking at this!
FYI. As I wrote in bug 182627 comment #38, for next two types of mail, - Simple text/plain mail, with name parameter - text/plain in multipart/mixed mail, with name/content-disposition/filename behaviour of Tb 3.0.3 was consistent with explanation in source code. - When Display Attachments Inline=Off, mail body(or mail body part) is not displayed at message display box - When Display Attachments Inline=Off and On, mail body(or mail body part) is displayed at attachment pane in any case (problem of "not displayed in some cases" was already resolved) (In reply to comment #7) > I'm thinking if disposition is inline and text/plain or text/html the part > should be displayed and if the disposition is 'attachment' then it should be > shown in attachment pane and displayed if display attachments inline is set. "no displayed mail body, no attachment at attachment pane" is also observed with next part in multipart/alternative. > --_----------=_1268324732523914 > Content-Transfer-Encoding: quoted-printable > Content-Type: text/html; name="message.html" What do you think about this case? "name=..." exists == generated by attach operation && no "inline" ==> not mail body, then attachmet? (same as mail-body of simple text/xxx, text/xxx part in multipart/mixed) If so, I think it's better to be displayed at attachment pane, even though a part in multpart/alternative, because text/html matches with "View/Message Body As=HTML". How about "Content-Disposition:inline" for text & image/jpeg part which is a part of multipart/related, which is not reffered via cid: by HTML(<img src="cid:..">, <iframe src="cid:...">). I think Content-Diposition:(inline or attachment) should be ignored.
Right now, for a bug to block 3.1, it needs to either: a) make the upgrade experience from TB2 very painful for a large number of users or b) be a new, reproducible, severe quality issue (eg dataloss, frequent crashes) Just from skimming this bug, it's not clear to me how this would fit into those buckets. Setting as blocking-, based on that. Phil, if I'm missing something, please renominate with details. Thanks!
(In reply to comment #12) > Phil, if I'm missing something, please renominate with details. Thanks! No, no problem, explanation wasn't necessary. I'm on 3.2pre and wanted to bump it for next release but couldn't find a 3.2 flag.
We'd take a fix for this in 3.1, as long as we have one. We just wouldn't block on it. Marking wanted.
David, we assume if it has a filename then it was meant to be attachment not a typed message, and we make it a mimeExternalObjectClass, but how do we get these types to show up in the attachment bucket? That would probably take care of this bug. In the craigs list example, it was probably intended recipients should open html in outside browsers for security.
actually, that doesn't sound right, if it is marked inline, text/html, with file name, and part of mp/alt then it was probably meant to display in html mode and show up in the attachment bucket.
OExp shows it like comment 16, in text or html mode and always as an attached file. I suspect any part with a file name should be loading into our attachment bucket.
(In reply to comment #16) > if it is marked inline, text/html, with > file name, and part of mp/alt then it was probably meant to display in html > mode and show up in the attachment bucket. I think if it was marked inline, it was not intended as an attachment, period. The alternative setting to "inline" is "attachment." The superfluous filename is probably an artifact of the MIME generator. We should only handle "inline" items as attachments if we don't support inline rendering for the type.
How do you save an attachment if it's not in the attachment bucket, because it's inline? I believe that's the point of showing it in the attachment pane.
Wait -- we're talking about the message body. The problem is, the message body has been tagged with a filename (for no readily apparent reason) -- and due to a BUG in Mozilla's Not-Wonderful MIME Implementation, the name is overriding the 'inline' flag. Why would we care if it's saveable as an attachment? And in the context of the message, you should be able to tell it's the body, because it's one of the top-level parts of a multipart/alternative structure. Now, I can readily believe that the MIME implementation is not sophisticated enough to make that determination, but the MIME structure has enough information.
with a 'name =' or 'filename =' I'd say sender intended showing an attached file and with it being 'inline' and the html alternative part then sender intended also having it displayed. Haven't found out where we can do that yet. It would save me more looking if anyone knows where we determine what goes in our attachments display, I guess it's not really the 'bucket' that is the compose window name.
(In reply to comment #20) > Wait -- we're talking about the message body. > I think if it was marked inline, it was not intended as an attachment, period. I'm sorry - I thought you were talking about any inline part.
Created attachment 444804 [details] [diff] [review] header with filenames goes to attachment view David, this is only a partial fix but comment on it if you get a chance. This patch will put *any* part with a name= or filename= parameter in its header into the attachment view.
Comment on attachment 432224 [details] [diff] [review] starting point for a fix >diff --git a/mailnews/mime/src/mimemsg.cpp b/mailnews/mime/src/mimemsg.cpp > >- body = mime_create(ct, msg->hdrs, obj->options); >+ body = mime_create(ct, msg->hdrs, obj->options, obj); I don't believe this will work when mime_create gets called from mimealt.cpp
Comment on attachment 432224 [details] [diff] [review] starting point for a fix > /* It's a text type. Write it only if it's the *first* part > that we're writing, and then only if it has no "filename" > specified (the assumption here being, if it has a can we just re-assume this and say a filename is not a deal breaker in displaying. If it's a text type, write it if it's the first_part.
(In reply to comment #23) > Created an attachment (id=444804) [details] > header with filenames goes to attachment view > > David, this is only a partial fix but comment on it if you get a chance. > This patch will put *any* part with a name= or filename= parameter in its > header into the attachment view. This needs fixing in so far as it is assuming all attachments have a filename parameter.
Bug 229075 Seems to be a lot of similiar bugs and maybe overlap. Wada, can you look at all these and close them as appropriate or assign me. I seem to be getting into another bug with my attachment 444804 [details] [diff] [review]. That attachment causes inline parts with filenames to be added to attachment view and assigned attachment status. This bug should only address the display problem with inline parts that have filenames.
Created attachment 445055 [details] [diff] [review] inline parts with filenames displayed and show as attachment We had assumed a filename parameter meant it wasn't typed-in in spite of it being inline. I removed that since it was more of a guess then an assumption. This may fix a gamut of bugs. Maybe we can do a try serve build for someone to help in testing. I'll think about a possible test although it will be limited to the new 'assumption'. This patch fixes the craigslist.org standard letters to display and have an attachment, like OExpress works.
(In reply to comment #27) I set dependency to this bug for three bugs and assigned to you.
Comment on attachment 445055 [details] [diff] [review] inline parts with filenames displayed and show as attachment sorry for the delay; I agree that this is the right thing to do.
Comment on attachment 445055 [details] [diff] [review] inline parts with filenames displayed and show as attachment can you sr it too, David.
Comment on attachment 445055 [details] [diff] [review] inline parts with filenames displayed and show as attachment sure, thx for the patch.
(In reply to comment #31) > (From update of attachment 445055 [details] [diff] [review]) > can you sr it too, David. Any way to add some unit test for this so we make sure not to regress in the future ?
Can the test be linked to a new bug and resolve this to keep the trunk builds up to date?
(In reply to comment #34) > Can the test be linked to a new bug and resolve this to keep the trunk builds > up to date? we can keep this one resolved with the in-testsuite ? set
Phil/David, should we be thinking about this for 3.1.x?
Being a basic design flaw fix I'd say it should be ok for 3.1.x. I haven't tested it on 3.1 though
(In reply to comment #38) > Being a basic design flaw fix I'd say it should be ok for 3.1.x. I haven't > tested it on 3.1 though Looking at bug 583419 it appears we'd need to fix that before taking this on a branch.