Closed Bug 192557 Opened 21 years ago Closed 20 years ago

Each instance of editing an HTML draft/template copies the <head> to the <body>

Categories

(MailNews Core :: Composition, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: colin, Assigned: neil)

References

Details

(Whiteboard: fixed-aviary1.0)

Attachments

(2 files, 1 obsolete file)

I have a mail message in my Drafts folder which I keep editing. I add stuff and
I delete stuff, and overall the number of characters in the mail doesn't change
much. But over time the size of the mail message continues to grow. Originally
it was 9K, and now it is 74K, even thought the actual text is no bigger.

It would appear that each time I edit the message it gets another:

<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1">
<title></title>

because there must be 200 or so instances of these two lines.

After all these meta and titles I have about 500 "blank lines", only each
"blank" line consists of about 72 spaces.

Finally I get to the HTML containing the text of my mail. And interleaved in
with the text are more 72-char "blank" lines.

Where is all this crud coming from, and is there a way to force the HTML to get
rewritten?
see also bug #70728

this sounds like an editor issue, so cc jfrancis.
Component: Mail Window Front End → Composition
Depends on: 70728
Note that the superfluous <meta> and <title> tags are appearing in the <body> of 
the message, not the <head>.  (Problem still occurring in 1.6b.)
As a result of investigating bug 234210, I've done a little more research into 
this bug.  What is happening is that every time you open an HTML template or 
draft for editing, all of the contents of the <head> are moved into the <body> 
and the <head> is reinitialized to contain the <meta> and the empty <title>.  If 
you open a draft repeatedly, then there are multiple copies of the <meta> and 
empty <title> tag in the <body>, as well as the (most recent) one in the <head>.

For those intrepid souls who manage to create a template with a <link> or 
<style> in the <head>, this problem short-circuits their efforts to make a valid 
HTML document in their mail.  In addition, the symptom in bug 234210 manifests: 
any http: URLs contained within the <style> are subject to "url recognition" and 
morphed into a cid: URL.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040402

Updating summary.
OS: Windows 98 → All
Hardware: PC → All
Summary: Editing a draft causes it to continually grow in size → Each instance of editing an HTML draft/template copies the <head> to the <body>
Attached patch Proposed patch (obsolete) — Splinter Review
Assignee: sspitzer → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Attachment #145570 - Attachment is obsolete: true
Comment on attachment 145572 [details] [diff] [review]
Also remove unused methods

aBuf appears to be the HTML document template so instead of appending to the
empty editor and fixing up the body tag I just replace everything.
Attachment #145572 - Flags: superreview?(mscott)
Attachment #145572 - Flags: review?(daniel)
Neil do simple tests like applying an HTML background to message then saving as
a template or draft still work correctly after this patch (those styles might be
on a body element so we may be ok)? Just looking for possible gotchyas...
This also occurs if you 'edit as new' a saved message that was not
saved as a template.
Actually for the test I used this as my template:
<html>
<head>
  <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
  <title></title>
  <style>th { background: url(about:logo); }</style>
</head>
<body bgcolor="#000000" text="#ffffff">
<br>
</body>
</html>
because I wanted to see what the URL rewriter would do to my inline url, but
that's another bug ;-)
I asked Neil to do a few torture tests with his patch before giving my r=.

1. apply a red bg to the doc before saving it as a template; reopen the template
2. apply a style attribute to the body before saving; is it preserved ? (important
   for the future of templates)
...
Hmm... that's a bad test, MsgComposeCommands.js stomps on the text and bgcolor
attributes in loadHTMLMsgPrefs - in fact if you have the select quote preference
combined with font preferences then the entire quote gets reformatted.

And that's not the only existing bug. A manual style set on the body persists
after the compose window is recycled, the cure is to open a second window.
(In reply to comment #9)
> Actually for the test I used this as my template:
> <html>
> <head>
>   <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
>   <title></title>
>   <style>th { background: url(about:logo); }</style>
> </head>
> <body bgcolor="#000000" text="#ffffff">
> <br>
> </body>
> </html>
> because I wanted to see what the URL rewriter would do to my inline url, but
> that's another bug ;-)

May I ask how you got the style into the head section.
If you did this within message compose, you did something I could not do
in 16 or so hours of trying.
Ahem... I edited the Templates file in a text editor :-)

But without this (or a similar) patch whenever you edit the template in message
compose the style will end up back in the body again...
(In reply to comment #11)
> Hmm... that's a bad test, MsgComposeCommands.js stomps on the text and bgcolor
> attributes in loadHTMLMsgPrefs

Bug 234354?


> And that's not the only existing bug. A manual style set on the body persists
> after the compose window is recycled, the cure is to open a second window.

Bug 234436.  See also bug 147645.


Does the patch address any of these addtional bugs?  Where did that function 
RebuildDocumentFromSource() come from, anyway?  It only shows up as defined, not 
used, in LXR.
(In reply to comment #14)
>Bug 234354?
>Bug 234436.  See also bug 147645.
>Does the patch address any of these addtional bugs?
No, it does not, although I might be able to address those bugs now that I am
aware of them.
>Where did that function RebuildDocumentFromSource() come from, anyway?
It's used by the web page editor JS to switch from source to normal view.
Comment on attachment 145572 [details] [diff] [review]
Also remove unused methods

I ran with this for a while and it looks good to me.
Attachment #145572 - Flags: superreview?(mscott) → superreview+
Blocks: 240183
No longer blocks: HTML-compose-tracker
Daniel Glazman, would you please make time to review Neil's patch on this bug?

The "torture tests" you suggested in comment 10 aren't really related to this 
bug; each of those items is a known problem with its own bug already:
 - HTML color/bgcolor are overwritten by prefs (bug 234354)
 - Inline style is lost (bug 147645 comment 3)

Neil, do you see any reason not to remove the dependency on bug 70728?
Hmm... I overlooked the blank lines issue...
Comment on attachment 145572 [details] [diff] [review]
Also remove unused methods

Seems ok to me if you accept that for instance the bgcolor is not updated if
you changed the default bgcolor between your "Save as Draft" and the next "Edit
as new"...
r=daniel@glazman.org
Attachment #145572 - Flags: review?(daniel) → review+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: fixed-aviary1.0
Verified fixed with
  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040610

Both the reporter's original problem, and its root cause cited in comment 3, 
have been fixed.  Thanks, Neil!  (And thanks, Daniel, for responding.)
Status: RESOLVED → VERIFIED
This patch causes a bad regression preventing mail from handling mailto urls
anymore. 

If you pass the compose window a mailto url with a body attribute, we no longer
insert the body into the compose window.

This is becasue the new method this code uses:

htmlEditor->RebuildDocumentFromSource(aBuf);

bails if the HTML to be inserted doesn't contain a proper body AND head tag. See:

http://lxr.mozilla.org/mozilla/source/editor/libeditor/html/nsHTMLEditor.cpp#1834

re-opening for re-evaluation to address the regression.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-aviary1.0
(In reply to comment #22)
> If you pass the compose window a mailto url with a body attribute, we no
> longer insert the body into the compose window.

Hmm... sounds similar to bug 246579.
Attachment #150948 - Flags: superreview?(kinmoz)
Attachment #150948 - Flags: review?(daniel)
Neil, check out bug 247313, and consider how the rebuilding of the document 
affects an HTML reply to its attachment 151017 [details].
Bug 248219 (mail/news) both seem to be the result of the regression induced by 
the patch for this bug.  Maybe also bug 247817 (TB).
Also, in comment 22, Scott is referring to bug 246595.  (For those keeping 
track...)
Requesting blocking because of the bugs cascading off of this.
Flags: blocking1.8a2?
Comment on attachment 150948 [details] [diff] [review]
Make RebuildDocumentFromSource smarter

>+  if (foundhead) {
>+    if (foundclosehead)
>+      res = ReplaceHeadContentsWithHTML(Substring(beginhead, beginclosehead));
>+    else if (foundbody)
>+      res = ReplaceHeadContentsWithHTML(Substring(beginhead, beginbody));
>+    else
>+      res = ReplaceHeadContentsWithHTML(Substring(beginhead, endtotal));

This last line and....

>+  } else {
>+    nsReadingIterator<PRUnichar> begintotal;
>+    aSourceString.BeginReading(begintotal);
>+    NS_NAMED_LITERAL_STRING(head, "<head>");
>+    if (foundclosehead)
>+      res = ReplaceHeadContentsWithHTML(head + Substring(begintotal, beginclosehead));
>+    else if (foundbody)
>+      res = ReplaceHeadContentsWithHTML(head + Substring(begintotal, beginbody));
>+    else
>+      res = ReplaceHeadContentsWithHTML(head);

that one scare me a bit. I remind you that both HEAD and BODY start and end
tags are
optional in HTML. So since your code is not detecting the first non-sub-head
element,
you could end up erroneously placing all document's contents inside head.
Inline Forwarding of TEXT/PLAIN messages is broken also.  (tested in build
2004070208)
Comment on attachment 150948 [details] [diff] [review]
Make RebuildDocumentFromSource smarter

Now that I've fixed bug 249665 I've been able to throughly test this in 4
cases:
* Forward Inline of either text or HTML
* mailto: with a body parameter
* send link/page
* using additional patch not provided here, switch web page editor from source
view to normal view without requiring head or body substrings.
Since glazou is now on holiday I'm asking cmanske for review, but he might want
to take glazou's comments into account.
Attachment #150948 - Flags: review?(daniel) → review?(cmanske)
Flags: blocking1.8a2? → blocking1.8a2-
Comment on attachment 150948 [details] [diff] [review]
Make RebuildDocumentFromSource smarter

r=brade (with additional comments added per irc conversation)
Attachment #150948 - Flags: review?(cmanske) → review+
Attachment #150948 - Flags: superreview?(kinmoz) → superreview?(mscott)
Attachment #150948 - Flags: superreview?(mscott) → superreview+
Comment on attachment 150948 [details] [diff] [review]
Make RebuildDocumentFromSource smarter

Fixes visible regressions (send page/link, forward inline, some mailto: links)
Attachment #150948 - Flags: approval1.8a2?
Comment on attachment 150948 [details] [diff] [review]
Make RebuildDocumentFromSource smarter

a=sspitzer
Attachment #150948 - Flags: approval1.8a2? → approval1.8a2+
Fix to fix checked in [crosses fingers]
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Confirming that the mailto-with-subject, Send Page/Send Link and inline-forward 
of text/plain is working as expected, with
  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040710
That addresses bug 248219 and most of bug 246579.

Additionally, the gross rewriting of the HTML source on an HTML reply (as was 
seen on the attachment for bug 247313) is no longer a problem (altho that bug's 
issue still exists).  In Mozilla Mail/News, I am not seeing the symptom of bug 
247817 (loss of <img> on reload of draft), but that was reported under TB, which 
I haven't tested.

Inline forwarding of HTML messages is still missing the separator and headers 
table: bug 250291.

The original problem of the <head> data being copied into the <body> is *still* 
fixed!

Verifying.
Status: RESOLVED → VERIFIED
Still seeing head data repeated in body with TB version 0.7+ (20040711)
20040613 branch build fixed this issue. It would seem that this fix is
no longer in the branch builds.
Will try a trunk build.
Test current trunk build TB version 0.6+ (20040706)
Extra meta data in body (this bug) is fixed in trunk
Escape problems after html edit as per
http://bugzilla.mozilla.org/show_bug.cgi?id=224733
was not fixed with this patch.(nor was it expected)
Thanks for hanging in there on this fix Neil, this alone
will save me many keystrokes.
Just noticed that trunk version was too old, will re-check for
new trunk with this fix
FYI: I just checked both the original patch and the regression fix back into the
aviary 1.0 branch now that the regression has been fixed. 

Joe, can you test tomorrow's bits? 
Whiteboard: fixed-aviary1.0
Confirming Mike's observations in comment 36 are identical in
Tbird version 0.7+ (20040722)
http://bugzilla.mozilla.org/show_bug.cgi?id=247817 has been marked a dup of
http://bugzilla.mozilla.org/show_bug.cgi?id=224733 and was not affected by this
fix. This is the next biggest deterrent to the html user (in my opinion)
Additionally while this fix allows styles and scripts to be placed in the
head section where some say they should be, 'ampersand' and 'greater than' are
still being escaped and not being preserved within the script tags, even if the
script is placed in the head. 
http://bugzilla.mozilla.org/show_bug.cgi?id=229122
 
I was mistaken in thinking that bug 247817 was related to this bug, sorry for 
the misdirection.
Bug 147645 appears to have been resolved -- could it have been from the fix to 
this bug?
any reason to take this for 1.7?
Here's one good reason:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.2) Gecko/20040804
Netscape/7.2 (ax) Escalade
X-Accept-Language: en-us, en

This is a multi-part message in MIME format.
--------------070801080104010704090209
Content-Type: text/html; charset=us-ascii
Content-Transfer-Encoding: 7bit

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
  <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
  <title></title>
</head>
<body alink="#0000ff" background="cid:part8.05010401.00090508@xxx.com"
 bgcolor="#ffffff" link="#ff0000" text="#ff0000" vlink="#800080">
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>

<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
The above would not be in my inbox.
I have been using a version of mail/news with this patch in since it was
checked into trunk with no ill-effects whatsoever.
This is a good fix. Anyone want to put a full patch for the 1.7 branch together?
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

Created:
Updated:
Size: