Closed
Bug 192557
Opened 22 years ago
Closed 21 years ago
Each instance of editing an HTML draft/template copies the <head> to the <body>
Categories
(MailNews Core :: Composition, defect)
MailNews Core
Composition
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: colin, Assigned: neil)
References
Details
(Whiteboard: fixed-aviary1.0)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
glazou
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Brade
:
review+
mscott
:
superreview+
sspitzer
:
approval1.8a2+
|
Details | Diff | Splinter Review |
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?
Comment 1•22 years ago
|
||
see also bug #70728
this sounds like an editor issue, so cc jfrancis.
Component: Mail Window Front End → Composition
Depends on: 70728
Comment 2•21 years ago
|
||
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.)
Comment 3•21 years ago
|
||
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.
Blocks: HTML-compose-tracker
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>
Assignee | ||
Comment 4•21 years ago
|
||
Assignee: sspitzer → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•21 years ago
|
||
Attachment #145570 -
Attachment is obsolete: true
Assignee | ||
Comment 6•21 years ago
|
||
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)
Comment 7•21 years ago
|
||
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...
Comment 8•21 years ago
|
||
This also occurs if you 'edit as new' a saved message that was not
saved as a template.
Assignee | ||
Comment 9•21 years ago
|
||
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 ;-)
Comment 10•21 years ago
|
||
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)
...
Assignee | ||
Comment 11•21 years ago
|
||
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.
Comment 12•21 years ago
|
||
(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.
Assignee | ||
Comment 13•21 years ago
|
||
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...
Comment 14•21 years ago
|
||
(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.
Assignee | ||
Comment 15•21 years ago
|
||
(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 16•21 years ago
|
||
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+
Updated•21 years ago
|
Comment 17•21 years ago
|
||
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?
Assignee | ||
Comment 18•21 years ago
|
||
Hmm... I overlooked the blank lines issue...
Comment 19•21 years ago
|
||
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+
Assignee | ||
Comment 20•21 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Whiteboard: fixed-aviary1.0
Comment 21•21 years ago
|
||
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
Comment 22•21 years ago
|
||
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
Comment 23•21 years ago
|
||
(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.
Assignee | ||
Comment 24•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #150948 -
Flags: superreview?(kinmoz)
Attachment #150948 -
Flags: review?(daniel)
Comment 25•21 years ago
|
||
Neil, check out bug 247313, and consider how the rebuilding of the document
affects an HTML reply to its attachment 151017 [details].
Comment 26•21 years ago
|
||
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).
Comment 27•21 years ago
|
||
Also, in comment 22, Scott is referring to bug 246595. (For those keeping
track...)
Comment 28•21 years ago
|
||
Requesting blocking because of the bugs cascading off of this.
Flags: blocking1.8a2?
Comment 29•21 years ago
|
||
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.
Comment 30•21 years ago
|
||
Inline Forwarding of TEXT/PLAIN messages is broken also. (tested in build
2004070208)
Assignee | ||
Comment 31•21 years ago
|
||
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)
Updated•21 years ago
|
Flags: blocking1.8a2? → blocking1.8a2-
Comment 32•21 years ago
|
||
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+
Assignee | ||
Updated•21 years ago
|
Attachment #150948 -
Flags: superreview?(kinmoz) → superreview?(mscott)
Updated•21 years ago
|
Attachment #150948 -
Flags: superreview?(mscott) → superreview+
Assignee | ||
Comment 33•21 years ago
|
||
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 34•21 years ago
|
||
Comment on attachment 150948 [details] [diff] [review]
Make RebuildDocumentFromSource smarter
a=sspitzer
Attachment #150948 -
Flags: approval1.8a2? → approval1.8a2+
Assignee | ||
Comment 35•21 years ago
|
||
Fix to fix checked in [crosses fingers]
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Comment 36•21 years ago
|
||
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
Comment 37•21 years ago
|
||
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.
Comment 38•21 years ago
|
||
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.
Comment 39•21 years ago
|
||
Just noticed that trunk version was too old, will re-check for
new trunk with this fix
Comment 40•21 years ago
|
||
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
Comment 41•20 years ago
|
||
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
Comment 42•20 years ago
|
||
I was mistaken in thinking that bug 247817 was related to this bug, sorry for
the misdirection.
Comment 43•20 years ago
|
||
Bug 147645 appears to have been resolved -- could it have been from the fix to
this bug?
Comment 44•20 years ago
|
||
any reason to take this for 1.7?
Comment 45•20 years ago
|
||
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.
Comment 46•20 years ago
|
||
This is a good fix. Anyone want to put a full patch for the 1.7 branch together?
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•