553526 - CJK long line is wrapped by each 72 characters on Mail editor if the message charset is UTF-8

Status: RESOLVED DUPLICATE of bug 653342

(MailNews Core :: Composition, defect)

Description

[Makoto Kato [:m_kato]]

- Step
1. Launch Thunderbird 3.0 or 3.1
2. [File] - [New] - [Message]
3. type "あ”100 times
4. Save this message, then close compose window
5. Reopen it from draft

- Result
The saved message is different before saved.  The space is inserted to message each 72 character.

- Expected Result
Don't insert space.

- Note
Since CJK has soft-line break, this only occurs on CJK text.

Comment 1

[David :Bienvenu]

nsMsgCompUtils.cpp - mime_generate_attachment_headers seems to be trying to use base64 for multibyte character sets already. Kato-san, any idea why that's not working?

Comment 2

[xunxun]

(In reply to David :Bienvenu from comment #1)
> nsMsgCompUtils.cpp - mime_generate_attachment_headers seems to be trying to
> use base64 for multibyte character sets already. Kato-san, any idea why
> that's not working?

It's only related with the attachment?
We should use base64 for all mail.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to David :Bienvenu from comment #1)
> nsMsgCompUtils.cpp - mime_generate_attachment_headers seems to be trying to
> use base64 for multibyte character sets already. Kato-san, any idea why
> that's not working?

The function does not work as expected.

837     // If charset is multibyte then no charset to be specified (apply base64 instead).
838     // The list of file types match with PickEncoding() where we put base64 label.
839     if ( ((attachmentCharset && !nsMsgI18Nmultibyte_charset(attachmentCharset)) ||
840          ((PL_strcasecmp(type, TEXT_HTML) == 0) ||
841          (PL_strcasecmp(type, TEXT_MDL) == 0) ||
842          (PL_strcasecmp(type, TEXT_PLAIN) == 0) ||
843          (PL_strcasecmp(type, TEXT_RICHTEXT) == 0) ||
844          (PL_strcasecmp(type, TEXT_ENRICHED) == 0) ||
845          (PL_strcasecmp(type, TEXT_VCARD) == 0) ||
846          (PL_strcasecmp(type, APPLICATION_DIRECTORY) == 0) || /* text/x-vcard synonym */
847          (PL_strcasecmp(type, TEXT_CSS) == 0) ||
848          (PL_strcasecmp(type, TEXT_JSSS) == 0)) ||
849          (PL_strcasecmp(encoding, ENCODING_BASE64) != 0)) &&
850          (*charset_label))

This if statement is passed whenever encoding is not "base64", in this issue case the encoding is "UTF-8" (it's in my case, it will be "ISO-2022-JP" or something). And the if statement is also passed in case of "text/plain"..

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I found where the real issue is laying:

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1843

1843 function ComposeFieldsReady()
1844 {
1845   //If we are in plain text, we need to set the wrap column
1846   if (! gMsgCompose.composeHTML) {
1847     try {
1848       gMsgCompose.editor.QueryInterface(nsIPlaintextEditorMail).wrapWidth
1849           = gMsgCompose.wrapLength;
1850     }

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I was wrong.

As mkato pointed out in bug 653342 comment 51, to use nsIDocumentEncoder::OutputRaw will solve this issue.

Comment 6

[Ludovic Hirlimann [:Usul]]

so this is a duplicate of 653342 ?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I have no idea whether this bug is a duplicate or not.

I know:

1. The wrapping issue in draft message on html mode editor will be solved by the patch for bug 653342.
2. The wrapping issue in draft message on plain text editor will be solved by the fix for bug 26734.

but I am not sure:

What the difference between this bug and bug 355209.

I'd say bug 355209 is a duplicate of bug 653342 and bug 26734.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)

> I'd say bug 355209 is a duplicate of bug 653342 and bug 26734.

Or bug 353209 depends on the two bugs.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: set preferences for signature in each test instead of runtest.py

Most of mozmill tests does not need "mail.identity.xx.htmlSigXXX" so those preferences should be set in each test. Otherwise once clearUserPref is called against those variables, then the test which needs the preferences will be broken.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Editing delsp=yes message as new should remove a trailing space on each line.

Even if thunderbird does not support "delsp=yes", thunderbird can receive "delsp=yes" messages from other MUAs. So if editing those message as new, we should handle "delsp=yes" case in message composition.

https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=5b5efb9ecb6d

This try server results are still polluted by a couple of bugs, but I think the results are fine at least for this patch.

Comment 11

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8530844 [details] [diff] [review]
Part 2: Editing delsp=yes message as new should remove a trailing space on each line.

Review of attachment 8530844 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/public/nsIMsgCompFields.idl
@@ +91,5 @@
> +
> +  /**
> +   * Represents the message format is delsp=yes if true.
> +   */
> +  attribute boolean delSp;

This is the main reason I'm declining this patch, when taken in context with bug 26734 comment 106. This attribute is being used solely to have compose munge its draft body differently--it doesn't affect the main compose pipeline, the primary use of this interface, and feels like an abuse of the API.

It would be vastly preferable to have the munging of the plaintext f=f content happen within the libmime code than to need to add this attribute to the interface.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Joshua Cranmer [:jcranmer] from comment #11)
> Comment on attachment 8530844 [details] [diff] [review]
> Part 2: Editing delsp=yes message as new should remove a trailing space on
> each line.
> 
> Review of attachment 8530844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/compose/public/nsIMsgCompFields.idl
> @@ +91,5 @@
> > +
> > +  /**
> > +   * Represents the message format is delsp=yes if true.
> > +   */
> > +  attribute boolean delSp;
> 
> This is the main reason I'm declining this patch, when taken in context with
> bug 26734 comment 106. This attribute is being used solely to have compose
> munge its draft body differently--it doesn't affect the main compose
> pipeline, the primary use of this interface, and feels like an abuse of the
> API.
> 
> It would be vastly preferable to have the munging of the plaintext f=f
> content happen within the libmime code than to need to add this attribute to
> the interface.

Thanks Joshua for reviewing.
Then the codes for format-flowed has to be moved into mimedrft first. Actually the current code is wrong because the code is run even if the message is not format-flowed.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)

> Then the codes for format-flowed has to be moved into mimedrft first.

Or into nsStreamConverter?

Comment 14

[Joshua Cranmer [:jcranmer]]

Seeing as how mimedrft.cpp isn't doing any f=f munging in the first place, I believe the code that would need to be fixed would be in mimetpfl.cpp, which governs the conversion of text/plain format=flowed to HTML for display (or other body conversion, which should be how compose gets its body).

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Joshua Cranmer [:jcranmer] from comment #14)
> Seeing as how mimedrft.cpp isn't doing any f=f munging in the first place, I
> believe the code that would need to be fixed would be in mimetpfl.cpp, which
> governs the conversion of text/plain format=flowed to HTML for display (or
> other body conversion, which should be how compose gets its body).

I know but there is no method to get the content-type of target message before parsing it in mime_bridge_create_draft_stream. 
Anyway I will investigate it.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

I was wrong. MimeInlineTextPlainFlowed is already used for draft message.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

OK. I was misunderstanding this issue.
The problem is that format=flowed is used for CJK message. Do not use formar=flowed on those messages even if the charset is UTF-8.

So the real problem is here in UseFormatFlowed():

  1999   // the line. Not all charsets like that.
  2000   return !(PL_strcasecmp(charset, "UTF-8") && nsMsgI18Nmultibyte_charset(charset));
  2001 }

Comment 18

[Hiroyuki Ikezoe (:hiro)]

To solve this issue we need to detect language of message. 
We also need this library.
http://mxr.mozilla.org/mozilla-central/source/browser/components/translation/cld2/

Comment 19

[Joshua Cranmer [:jcranmer]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> To solve this issue we need to detect language of message. 

Strictly speaking, it's script that we'd need, not language, right? Column 7 of <http://unicode.org/cldr/trac/browser/trunk/common/properties/scriptMetadata.txt> appears to be the metric we're trying to get at.

(We'd still need to import a character->script mapping table, or maybe harangue ICU to get us that data).

Comment 20

[Hiroyuki Ikezoe (:hiro)]

(In reply to Joshua Cranmer [:jcranmer] from comment #19)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> > To solve this issue we need to detect language of message. 
> 
> Strictly speaking, it's script that we'd need, not language, right? Column 7
> of
> <http://unicode.org/cldr/trac/browser/trunk/common/properties/scriptMetadata.
> txt> appears to be the metric we're trying to get at.

Right! That's exactly needed for this issue.

Comment 21

[Jorg K (CEST = GMT+2)]

Here we have another bug in the terrible mangle. Once again we try to remove spaces that got erroneously added by the M-C selialiser, bug 1225864.

I decided to close all the bugs in the mangle and leave three open to fix:
Bug 1225864 - the root source of the spaces.
Bug 1225904 - another source of spaces/newlines/white-space being added where it shouldn't even shredding through multi-byte characters.
Bug 653342 - This is the only one I will keep open until the two above close.

I can see that more "delsp=yes" processing is proposed in the patch (attachment 8530844 [details] [diff] [review]), so I will put a note into the "delsp" bug 26734.

I'm making this a duplicate of bug 653342, since it will get fixed when the other one is fixed.