Closed Bug 131889 Opened 23 years ago Closed 22 years ago

Edit as New menu inserts extra http-equiv charset lines into mail body & head elements

Categories

(MailNews Core :: Internationalization, defect)

Product:
MailNews Core
Component:
Internationalization
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.1alpha

People

(Reporter: momoi, Assigned: bugzilla)

References

Details

(Keywords: intl, topembed-, Whiteboard: have fix [ish1+])

Attachments

(3 files, 4 obsolete files)

Test file containing 4 msgs as described above.
5.80 KB, text/plain
Details
a message with a line added above the existing text (Edit as new)
1.73 KB, text/plain
Details
Proposed fix, v5
2.56 KB, patch
bugzilla
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
** Observed with 2002-03-12 Win32 trunk build ** I will attch a mailbox file with 4 test cases. One of the test cases is not consistent but others seem to be consistent. 1. Case 1: Meta equiva charset line is inserted within the <head> element. This is a normal case. Sent with no special setting (options | format auto-detect) 2. Case 2: Meta equiva charset line is NOT inserted within the <head> element. This is a normal case. Sent with options | format | HTML only option ON. 3. Case 3: a longer message but normal. Sent with no special setting (options | format auto-detect) 4. Case 4: a longer message with a duplicated charset lines in the <head> element and within the <body> area. This was created by taking the received message in step 3 and using Message | Edit as new .. menu. It seems that the charset line gets inserted again and again. We should fix this problem. I have not been able to consistently reproduce case 2. We need help from others on this.
Place this file in your local mail folder and view the sources.
nabeta1 and topembed. This problem will affect embedded client's mail program badly.
Keywords: nsbeta1, topembed
Keywords: intl
So do we want to put http-equiv charset or not?
Status: NEW → ASSIGNED
>nabeta1 and topembed. This problem will affect embedded client's mail >program badly. I don't understand here, you use Mozilla to generate these messages and you think some other software cannot handle these message, is that right? 1. in what case the message is illegal in MIME or HTML ? IS this bug file against mail sending or receiving? If all the message we send are legal, then you should file bug against that receiving software, because other mailer may choose to send them these kind of message to break them if they don't fix it regardless mozilla change the sending format or not. I don't think HTML or MIME require consistency of meta tag What is the real issue here ? I think we should nsbeta1- this one
> 1. in what case the message is illegal in MIME or HTML ? We are inserting an extra <Meta HTTP-equiv ...> charset line in the body as well as the <head> ... </head>. This is illegal HTML. We need to fix this. This is a Mozilla-side problem --> Case 4 above. Study Case 4 please before you make your conclusion. > IS this bug file against mail sending or receiving? This is filed against Mail sending by Mozilla. It IS OUR bug. > I don't think HTML or MIME require consistency of meta tag This comment applies to Case 2 only. You may be right but Mozilla should be consistent in generating its own HTML code. I concede that this is not as critical as Case 4. > What is the real issue here ? The big issue is Case 4. We are sending illegal HTML. We should stop doing this. We will probably cause a problem to some mail programs which have to interpret our bad HTML. > I think we should nsbeta1- this one I disagree. We must fix at least Case 4.It is a bad bug and is likely to cause a problem for other mail programs. Case 2, I will try to track it down as time allows me. But it is not a good thing, either, for us to be incoistent this way. It could be a symptom of something else that might cause us a problem. Note: The test msgs are all generated by Mozilla. This is our problem.
>3. Case 3: a longer message but normal. Sent with no special setting >(options | format auto-detect) >4. Case 4: a longer message with a duplicated charset lines in the <head> >element and within the <body> area. > > This was created by taking the received message in step 3 and using > Message | Edit as new .. menu. how do you create case 4. what does "taking" mean here ? copy and paste ? Please specify step-to-step reproduce procedure. Thanks. >The big issue is Case 4. We are sending illegal HTML. Is that ILLEGAL html ? or invalid tag in html? I think it is invalid to have META in the body but not illegal.
> > This was created by taking the received message in step 3 and using > Message | Edit as new .. menu. > how do you create case 4. what does "taking" mean here ? copy and paste ? > Please specify step-to-step reproduce procedure. Thanks. Please let mail QAs reproduce this problem. It should be easy to use the menu item. If you like, here are the steps: 1. Send yourseld a HTML message with 30 lines of text or more in the body. 2. Receive it and view it. Now choose Message | Edit Message as New menu. Add some comments to this and send it out. 3. Receive it and view the source. You will see <Meta HTTP-Equiv...> line in the body as well as in the head element.
i tested this a little bit and here is what i found: - in case you Edit message as new (Kat's message or compose your own) and add some text on top of the existing text then NO <Meta HTTP-Equiv...> is inserted in the header= meaning the <Meta HTTP-Equiv...> is inserted once in the body ( normal) - in case you Edit as new inserting some text inside the exsiting lines then you'll get a second <Meta HTTP-Equiv...> in the head ( not normal...) i'll attach the Kat's test case with a line added on top of his text and you'll see that no second <Meta HTTP-Equiv...> exist on the page source
Since the main problem here is that of inserting extra invalid Meta Equiv charset lines into body and head areas, I changed the summary line. The incosistent insertion will be filed as a new bug when I can isolate the condition that leads to the problem.
Summary: Inconsistent insertion of http-equiv charset line into mail body → Edit as New menu inserts extra http-equiv charset lines into mail body & head elements
<title> is also duplicated.
I am getting different results the first composed mail and others (Edit as new to the same message). The very first mail I compose after I launch. <html> <head> <title></title> </head> <body> <meta http-equiv="Content-Type" content="text/html;charset=ISO-2022-JP"> <title></title> The second mail. <html> <head> <meta http-equiv="Content-Type" content="text/html;charset=ISO-2022-JP"> <title></title> </head> <body> <meta http-equiv="Content-Type" content="text/html;charset=ISO-2022-JP"> <title></title>
So far, I cannot reproduce this when I set "mail.compose.max_recycled_windows" to 0. cc to ducarroz, sspitzer. For the recycle windows feature, is HEAD section always cleared per each message send?
Blocks: 132191
nsbeta1- per i18n triage
Keywords: nsbeta1 → nsbeta1-
embed triage: topembed-, not critical for immediate embedding needs
Keywords: topembed → topembed-
Reassign to ducarroz.
Assignee: nhotta → ducarroz
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1alpha
nominate nsbeta1
Keywords: nsbeta1- → nsbeta1
the difference with recycle compose window is gone since a while. The apparent problem is that when you edit as new, mozilla insert the raw data of the original message as the body of a new message wich result of duplication of the head inside the message body. Probably an editor matter!
jfd: you insert it all into the body? That sounds very, very, VERY wrong. http-equiv and others tags are in the <head> and should not be added to the <body>. Perhaps you should be doing something like Composer does when it converts from html source --> Normal view via its tabs.
I am not sure at 100% yet. But if I correctly understand, I should cut the header and keep only the body part! But I need to preserve the charset and potentially some other attribute like background colors and image. Let me take a deeper look...
I can confirm that in the case of edit draft/template/new, we insert the whole HTML of the original message into editor. I don't know yet for forward inline which use the same code. I thing the right thing to do is to extract the charset from the META tag and set it into the compose field (it's not the case currently, editing a japanese message give a compose window with the charset set to US-ASCII) and keep only the body tag and it's content. Editor knows how to deal with the body tag and we have special code in message compose to take care of the body's attribute.
Attached patch Proposed fix, v1 (obsolete) — — Splinter Review
The solution I have choosen is to remove the <head> tag before inserting HTML into the editor. I did not find the need to fetch the charset set in the META tag (inside the <head>) has we already get the charset really used in mime. In the case where the user select a japanese charset, type couple english word, save the message, close it and then later re-edit it, the charset will have been reset to US_ASCII as japanese wasn't needed. Naoki, let me know if it's ok with you?
I think I understand JFD's logic in simply removing the <head> region. I'd just like to point out that if you wanted to use the new head content instead of original and avoid duplication, you could use nsIHTMLEditor::ReplaceHeadContentsWithHTML(). That's what we use in Composer to get around the problem of having <head> content when using InsertHTML(). See RebuildDocumentFromSource() in nsHTMLEditor.cpp for details.
Comment on attachment 96248 [details] [diff] [review] Proposed fix, v1 I'm concerned (but maybe I misunderstand the bug). If I receive a message that has Shif-JIS encoding and then I edit as new, shouldn't I retain the shif-jis encoding? It seems wrong to me to delete the existing head in favor of whatever the editor has in it. Perhaps I don't understand some key things about mail though.
if the data of the message really contains Shif-JIS, the charset in the content-type header would have been set to Shif-JIS. In that case we will correctly set the charset for editing. Now, if we really need to preserve the characters set used during editing, I would have to sniff the META tag. I18n folk, please advice...
Attached patch Proposed fix, v2 (obsolete) — — Splinter Review
This time, instead of just cuttng the header tag, I save the header content and re-insert it using the editor function nsIHTMLEditor::ReplaceHeadContentsWithHTML as suggested by cmanske.
Attachment #96248 - Attachment is obsolete: true
Whiteboard: have fix
Comment on attachment 96760 [details] [diff] [review] Proposed fix, v2 looks good to me - r=varada
Attachment #96760 - Flags: review+
Comment on attachment 96760 [details] [diff] [review] Proposed fix, v2 ok, I'll admit I haven't read alecf's latest string document in a month or two... Is this a memory leak or is nsCRT::strndup not allocating anything new? + PRUnichar* headData = new PRUnichar[end - start - 6]; + if (headData) + { + headData = nsCRT::strndup(&data[start + 6], end - start - 6);
good catch Katy, strdup does allocate the needed memory, not like strcpy.
Attachment #96760 - Flags: needs-work+
Attached patch Proposed fix, v3 (obsolete) — — Splinter Review
I fix the memory leak in the new code as well in the old code I did indeed copy.
Attachment #96760 - Attachment is obsolete: true
Comment on attachment 96861 [details] [diff] [review] Proposed fix, v3 your patch uses these api: + aEditorShell->ReplaceHeadContentsWithHTML(headContent.get()); + aEditorShell->InsertSource(aBuf.get()); editorshell is deprecated and will be going away. Since you already have an editor, can you use the editor api instead? Thanks!
If I want to stop using nsIEditorShell, I would need to have an nsIHTMLEditor object and what I have is an nsIEditor object in hand. If nsIEditorShell is going away in the future, I'll have to change a lot of code which is behind the scoop of this bug. At that point, see that I will be gone for 3 weeks at the end of this week, I would rather limit the patch to te strict minimum. I'll file a new bug for replacing nsIEditorShell in msg compose.
I filled bug 164967 for the replacement of nsIEditorShell in message compose
Comment on attachment 96861 [details] [diff] [review] Proposed fix, v3 r=cavin. It would be nice if you can use #define for the constant 6 (stelen of "<head>" I think) because it's used multiple times.
Attachment #96861 - Flags: review+
Attached patch Proposed fix, v4 (obsolete) — — Splinter Review
I simplified the arithmetic...
Attachment #96861 - Attachment is obsolete: true
Comment on attachment 96904 [details] [diff] [review] Proposed fix, v4 carrying over R=cavin
Attachment #96904 - Flags: review+
Comment on attachment 96904 [details] [diff] [review] Proposed fix, v4 + bodyAttributes.Adopt(nsCRT::strndup(&data[start], end - star)); this can't compile, can it? star should be start, shouldn't it?
oops, I've attached the wrong patch. You are right, it should be start and not star.
Attached patch Proposed fix, v5 — — Splinter Review
Same than before but without the typo!
Attachment #96904 - Attachment is obsolete: true
Comment on attachment 96918 [details] [diff] [review] Proposed fix, v5 Carry over R=cavin
Attachment #96918 - Flags: review+
Comment on attachment 96918 [details] [diff] [review] Proposed fix, v5 sr=bienvenu
Attachment #96918 - Flags: superreview+
Fixe checked in the trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
*** Bug 153761 has been marked as a duplicate of this bug. ***
Blocks: 157673
Whiteboard: have fix → have fix [ish1+]
Depends on: 180372
No longer blocks: 157673
Using the test case in comment #7 I was able to reproduce the problem with the 7.0 rtm build so that I could verify the problem is fixed with the 12-6-2002 trunk builds on winxp, mac osx and linux. Verified.
Status: RESOLVED → VERIFIED
This fix caused a preformance regression: bug 184550
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: