Closed Bug 56304 Opened 24 years ago Closed 23 years ago

HTML ja signature displays as garbage in the mail body

Categories

(MailNews Core :: Internationalization, defect, P2)

Product:
MailNews Core
Component:
Internationalization
Platform:
All
Windows 95
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.4

People

(Reporter: ji, Assigned: jbetak)

References

Details

(Keywords: intl, Whiteboard: fixed on the trunk (0.9.4))

Attachments

(9 files)

Attached a mail folder containing the problematic mail.
665 bytes, application/octet-stream
Details
cvs diff -u -b -w .\mailnews\compose
1.18 KB, patch
Details | Diff | Splinter Review
A sjis html file that can be used as a signature file.
3.05 KB, text/html
Details
A utf8 mail with the signature sjis file I attached to this report.
224.94 KB, image/jpeg
Details
SHIFT_JIS signature file with the charset name in all uppercase
3.11 KB, text/html
Details
missed this change in the meta charset parser - it returns the real value of the existing charset, instead of uppercase
1.23 KB, patch
Details | Diff | Splinter Review
looks like nhotta's original recommendation to remove the charset was right on the money, parsing the charset out during user override turns out to be very laborious and inefficient
2.26 KB, patch
Details | Diff | Splinter Review
cleaned-up patch to remove sig charset
1.51 KB, patch
Details | Diff | Splinter Review
Updated patch - addressed the issues raised by sspitzer
1.53 KB, patch
Details | Diff | Splinter Review 
******observed with 2000101208 branch build****

Attach a Japanese HTML signature file to a HTML mail. After received, the 
Japanese signature is displayed garbled in the mail body.

Steps to reproduce:
1. Launch mail.
2. Set mail compose style to HTML format.
3. Set mail preference to attach a Japanese HTML file in ShiftJis when you 
compose a mail.
4. Compose a mail attached with a Ja HTML signature file.
   In the mail compose window, the Japanese signature is displayed alright.
5. Send the mail to your testing account itself after setting the encoding to 
ISO-2022-JP.
6. After you receive the mail, you'll see the Japanese signature is garbled.

Note:
Only HTML mail with Japanese HTML signature combination will have this problem.
This is a cross-platfrom problem.
Hardware: PC → All
This is caused by inconsistent charset labels (the signiture part and META of
the signature HTML). The message can be seen if I edit the mail source and
remove the META tag.
We could try to fix this in two ways.
* When attaching a signature, parse and remove the META tag.
* Do not convert a signature HTML file to mail charset (e.g. ISO-2022-JP) and
not put a charset label for the signature part.


--------------030205000304000908020705
Content-Type: text/html; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit

<html><head></head><body>test<br>
<div class="moz-signature">-- <br><title>moz-sign</title>


.$BF|K\8l$N=pL>$G$9!#.(B
</div>
</body></html>
--------------030205000304000908020705--
Status: NEW → ASSIGNED
Target Milestone: --- → M23
Keywords: intl
Mass change of QA contact to ji for the bugs filed by her.
QA Contact: momoi → ji
Target Milestone: --- → Future
*** Bug 76362 has been marked as a duplicate of this bug. ***
NHotta/Jenm - please re-eval for nsbeta1.
Keywords: nsbeta1
NHotta/Jenm - please re-eval for M0.9.2 or agree to Furture milestone.
The possbile fixes I mentioned before:
* When attaching a signature, parse and remove the META tag.
* Do not convert a signature HTML file to mail charset (e.g. ISO-2022-JP) and
not put a charset label for the signature part.

The first one is not a small change. I can investigate the second option.
I would like to know how often this is used, I mean HTML signature which is
created by HTML editor.
*** Bug 86784 has been marked as a duplicate of this bug. ***
Target Milestone: Future → ---
Keywords: nsBranch
this is on Cindy's list for eMojo, cc'ing myself and ftang
mark it as moz0.9.4 and P2. reassign to jbetak. jbetak- think about how to fix 
this one.
Assignee: nhotta → jbetak
Status: ASSIGNED → NEW
Priority: P3 → P2
Target Milestone: --- → mozilla0.9.4
accepting...
Status: NEW → ASSIGNED
Whiteboard: nsBranch+
I'll be working on this over the weekend... Hope to have a fix by 08/21 - 
before the a= period begins.
it really looks like the trouble begins when reading the signature file. We are 
implicitly assuming it is in the platform encoding when converting it to the 
internal UCS-2 representation. This means that on a US Windows installation we 
will handle a say Shift_JIS signature file as if it was encoded in win1252.

http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompose.cpp#25
83

Second part of the problem is sending the signature from the mail client. Since 
we will need to determine the file encoding when reading it from disk, we could 
use that charset information as a hint, label the sig file properly and send it 
out unmodified. Although, this does not cover mislabeled signatures, it should 
be enough for most real-life scenarios.
upon addressing bug 91670 I've noticed that emails sent from Mozilla per repro 
steps outlined in this bug in fact *do* render correctly in other mail clients 
(say Outlook or Webmail). I'll try to see, how I could tweak the display side 
of things in Mozilla to make the messages appear correctly...
this patch contains the fix for bug 91670 as well. They both seem to be related 
and I'm quite happy with the approach outline in the patch. I believe it's the 
best and most logical modification and has the advantage to be very contained, 
which would make any backout in case of regressions very easy.

I'm beginning to believe that I should limit this fix to work on HTML 
signatures only, although determining the file type will add quite a bit of 
overhead - unless we cache it in an earlier step... I'll look into it tonight. 
Meanwhile the patch is fully functional and I'm very happy with the way things 
are shaping out.

cc'ing some mailnews folks for additional input / possible r/sr.
Whiteboard: nsBranch+ → nsBranch+, have patch - need r/sr
Looks good. R=ducarroz
cc'ing blizzard for potential i18n sr
sr=sspitzer

make sure to test that signatures are still ok on US-ASCII machines.
fix checked in - thanks everyone!

ji - I'll write an email to marina regarding bug 91670, as this checkin
addresses that bug too. We should retest mail signatures (both ASCII and
non-ASCII) very carefully to make sure there will be no regressions for eMojo.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
With 08-22-08-trunk build, it seems bug 91670 is fixed. But the problem
described in this report is not completely gone. It only works when mail is sent
in plain text only format. If the mail is sent in HTML only or both in plain
text and HTML format with a html signature file, the mail is garbled after
received.  Reopened the bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
>If the mail is sent in HTML only or both in plain
>text and HTML format with a html signature file, the mail is garbled after
>received

Xianglang, I'd like to talk to you about this - I certainly tested all mail 
formats in my debug build and from the nature of the fix it would be highly 
surprising to see it work for HTML only. If something is still broken, it will 
be most likely a little more specific and not affect all HTML-formatted mails.

Please provide more info - like what was your mail composition charset, what 
signature did you use, what was its charset, did it have a meta tag etc.
Keywords: nsBranch → nsbranch
The mail composition charset is ISO-2022-JP, the signature files I tested 
include euc-jp html file and Shift-JIS html file, and the signature files have 
charset meta tag. 
Again, it only works when the mail is sent in plain-text only format.
It doesn't work when the mail is sent in HTML only format or in plain text and 
HTML format. (there are three options to choose before sending out the mail, it 
only works when the 2nd one is chosen)
Xianglan, is the display broken while you compose or you see it when the mail is
received?
I think I get it - it's still broken for messages composed in ISO-2022-JP with 
a Japanese signature file. I have a feeling I know why that is, but Xianglang 
just try to change your composition encoding to anything else except ISO-2022-
JP while keeping the the signature the same. I think it'll be not garbled.
When the mail is sent in utf8, it's not garbled, but the signature ( a sjis 
html file) is not displayed correctly. Screen shot to follow.
OK, I think I found the glitch in this fix:

http://lxr.mozilla.org/seamonkey/source/mailnews/base/util/nsMsgI18N.cpp#571

We parse the charset from the signature file correctly, but it gets converted 
to all uppercase in the process! This means that we won't find it again, when 
trying to replace the charset encoding string in the meta tag, which can cause 
problems with the display since we are sending the sig file properly converted, 
but mislabeled.

Xianglang, could you please change the charset string in your test signature 
file to all uppercase and try again? I'll attach your test file modified in 
such a way also. If that succeeds, then we can be sure that it's just this 
silly omission...

ducarroz: 

I missed this change in the patch. I didn't realize that the parser changes the 
charset into an all-uppercase value. I'd like to modify it, so that it returns 
the true value of the charset it found. It can be then more easily substituted -
 the string classes don't provide case-insensitive substring replacement yet.

Could you please have another look?
The signature file attached at 08/23/01 00:20 doesn't work. It seems that
changing the charset meta tag to upper case doesn't make differences.
looks good. R=ducarroz
thanks ducarroz!

sspitzer: could you please have another look as well? I missed this in my 
original patch. The parsed meta charset is currently converted to all uppercase 
by the i18n utility parser, which makes it hard to replace.
Seth's on vacation sr=bienvenu
thanks bienvenu!

asa, could you a=?
Whiteboard: nsBranch+, have patch - need r/sr → nsBranch+, have patch - need a=
ducarroz, bienvenu: could I pester you some more? 

Looks like keeping the signature charset in sync with the message charset takes 
up too much bandwidth. Local i18n QA pointed out that manual charset overrides 
are quite frequent during composition and as it turns out, it's takes quite a 
bit of effort to parse the signature charset from the message body. I think it 
would be inefficient to burden the product with this.

I'd like to stick with nhotta's original recommendation  to remove the meta 
charset from the signature, although I was originally hesitant, since it's a 
more intrusive approach.
R=ducarroz
Status: REOPENED → ASSIGNED
Whiteboard: nsBranch+, have patch - need a= → nsBranch+, have patch - need sr/a=
bienvenu: are you reading this? Could you help me to get this into the tree 
before tomorrow's freeze?

cc'ing mscott as last resort mailnews sr, since sspitzer is apparently on 
vacation
blizzard: 

would you be able to sr this? This bug is one of i18n's top priorities for 
0.9.4.
Per Selmer suggestion, I am doing this myself.

Moving nsbranch+ designation from the Status Whiteboard to Keywords.

Jbetak - Pls send an email to drivers, if blizzard can help with sr/a by the end
of the day.
Keywords: nsbranch → nsbranch+
Whiteboard: nsBranch+, have patch - need sr/a= → have patch - need sr/a=
comments:

+    metaCharset.AssignWithConversion("charset=");

can't you do metaCharset = NS_LITERAL_STRING("charset=").get(); or something
that doesn't require a conversion?

+    metaCharset.Append(sigEncoding);
+    int metaCharsetOffset = sigData.Find(metaCharset,true,0,-1);

use a PRInt32 instead of int.
 
+    if (0 < metaCharsetOffset)

I'd prefer:

if (metaCharsetOffset != kNotFound)

since Find returns kNotFound (-1) on failure, right

can you address those nits, and attach a new patch?

sspitzer:

I've attempted to address the issues you raised, could you have another look?
looks good.  sr=sspitzer
Whiteboard: have patch - need sr/a= → have patch - need a=
a=asa on behalf of drivers.
OK, so the second installment of this fix just went in. Hope the QA will be 
more content with the results this time.

Thanks everyone for all your help!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Whiteboard: have patch - need a= → fixed on the trunk (0.9.4)
Verified with 09/04 builds. It's fixed.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: