Closed Bug 70728 Opened 23 years ago Closed 12 years ago

Mail Editor adds two blank lines at the end on saving as Draft

Categories

(MailNews Core :: Composition, defect)

x86
All
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: holgermetzger, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [patchlove][has draft patch][needs new assignee])

Attachments

(3 files, 8 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows 98)
BuildID:    20010302

The Mail/News editor adds two blank lines at the end of a message/posting.

Reproducible: Always
Steps to Reproduce:
Write a message, save it as a file, load it in your favorite text editor. There 
are two lines at the end. This Outlook Express behaviour is shocking :)

Actual Results:  It senselessly(?) adds two blank lines at the end of each 
mail/posting.

Expected Results:  Mozilla Mail/nEws shouldn't add in the body part of an 
email. After the last character is typed in by the user and there's an EOF 
there should be an EOF, not two blank lines. I think it is in some RFC, but I'm 
not sure.
Status: UNCONFIRMED → NEW
Ever confirmed: true
This may be related to bug 71433.
This adding of two lines is quite tragic, because there are some people in
usenet who filter rigorously for GNKSA errors. For example a common standard for
signatures is 4 lines, with the two extra lines it becomes 6 lines = *red alert*
signature too long = filter rules apply = Mozilla users won't be read by the
fanatics... which of course might not be too bad, but... :)
This is an editor problem. This append in both plain text and html compose. I
can reproduce this problem using the regular composer (editor).
Assignee: ducarroz → kin
Component: Composition → Editor: Core
Keywords: mailtrack
Product: MailNews → Browser
QA Contact: esther → sujay
*** Bug 71433 has been marked as a duplicate of this bug. ***
I'm having trouble reproducing this bug. Can someone give me clear steps on how 
to reproduce this, or add some sample output to this bug so I can see what 
exactly was typed?

I know ducarroz@netscape.com said that he can repro in Composer .. so I'm
assuming that HTML Mail Compose was used by the reporter and not Plaintext mail
compose, correct?
Whiteboard: Need more info
Bug 71433 includes clear steps to reproduce.
Using plaintext editor.
1.) Write an email.
2.) Send email.
3.) Save sent email as plaintext file. It has two blank lines at the end.

It gets worse. If you re-edit a file in the Unsent Messages folder each time you
edit it, another 2 lines will be added.
Another thing: Just a couple of days ago I was asked in a newsgroup why Mozilla
adds empty lines at the end and if this is the first step in becoming an Outlook
Express clone (OE has this "feature" for quite some time now, comes very handy
if you want to see how many times the sender edited the email).
This is not an editor bug. This is a bug in the method mail compose uses to 
convert the HTML output from the editor, used by message compose, into 
plaintext.

It uses an nsParser/CNavDTD with an nsPlainTextSerializer content sink, and the 
interaction between the 2 produces different PlainText output than the method 
regular Composer uses to output text. In particular, using the parser to 
serialize plain text seems to allow HTML file formatting whitespace, which is 
normally not visible on screen, to be written out as part of the plaintext 
content.

In the case where we are sending a message with a single word "test" followed by 
a return, the editor has written out the following:


<html>
<body>
test<br>
<br>
</body>
</html>

The 2 new lines being written out during the plaintext conversion are coming 
from the newlines after the <br> tags.

Here's a short stack that shows how message compose is converting the editor's 
HTML output into plaintext on send:


nsPlainTextSerializer::DoAddLeaf(int 114, const nsAString & {...}) line 914
nsPlainTextSerializer::AddLeaf(nsPlainTextSerializer * const 0x0790734c, const 
nsIParserNode & {...}) line 417 + 22 bytes
CNavDTD::AddLeaf(const nsIParserNode * 0x0791c0b0) line 3767 + 22 bytes
CNavDTD::HandleDefaultStartToken(CToken * 0x07933368, nsHTMLTag eHTMLTag_text, 
nsCParserNode * 0x0791c0b0) line 1305 + 12 bytes
CNavDTD::HandleStartToken(CToken * 0x07933368) line 1715 + 22 bytes
CNavDTD::HandleToken(CNavDTD * const 0x07934700, CToken * 0x07933368, nsIParser 
* 0x07abf508) line 881 + 12 bytes
CNavDTD::BuildModel(CNavDTD * const 0x07934700, nsIParser * 0x07abf508, 
nsITokenizer * 0x079344c8, nsITokenObserver * 0x00000000, nsIContentSink * 
0x0790734c) line 517 + 20 bytes
nsParser::BuildModel() line 2023 + 34 bytes
nsParser::ResumeParse(int 0, int 0) line 1889 + 11 bytes
nsParser::Parse(const nsAString & {...}, void * 0x00000000, const nsString & 
{...}, int 0, int 1, nsDTDMode eDTDMode_autodetect) line 1748 + 15 bytes
ConvertBufToPlainText(nsString & {...}, int 1) line 2046
nsMsgAttachmentHandler::UrlExit(unsigned int 0, const unsigned short * 
0x00000000) line 1093 + 28 bytes
Assignee: kin → harishd
Component: Editor: Core → DOM to Text Conversion
Whiteboard: Need more info
If the file has "test" followed by return, why are there two br tags in what the
editor writes out?  Shouldn't there be only one?  Could this problem be coming
from the editor's moz br tags?
BRs only mark the end of a line ... that is, it doesn't create a 2nd line which
is what user's expect visually. So the 2nd BR is what creates the 2nd blank line.
May be we need to normalize newlines ( rather CRLFs ) in the plain text
serializer. Giving bug to peterv.
Assignee: harishd → peterv
Harish: Are you saying that CRLFs somehow got into the dom tree?  They're not
supposed to be allowed in there (the DOM spec says only newlines), so the
serializer assumes that in most places (though the plaintext serializer may have
some redundant code for CRLF which is no longer needed).
This is a multi-platform bug, not just Win98 on PC. This also affects Mozilla
0.9.6 on Mac OS X 10.1.
Priority: -- → P3
Target Milestone: --- → mozilla1.2
OS: Windows 98 → All
*** Bug 120883 has been marked as a duplicate of this bug. ***
There is only one extra new line at the end of body now, probably by the fix 
for bug# 75283.

In nsPlainTextSerializer.cpp we treat BR's in following manner:
  else if (type == eHTMLTag_br) {
    // Another egregious editor workaround, see bug 38194:
    // ignore the bogus br tags that the editor sticks here and there.
    nsAutoString typeAttr;
    if (NS_FAILED(GetAttributeValue(nsHTMLAtoms::type, typeAttr))
        || !typeAttr.Equals(NS_LITERAL_STRING("_moz"))) {
      EnsureVerticalSpace(mEmptyLines+1);
    }
  }

where EnsureVerticalSpace makes sure that required new lines are also added. 
This is generic behaviour of Plain Text Serializer and most likely being used 
at places other than Mails too. So if we can have only a single <br>(comment# 
9) or "br" with type "_moz" that would solve the problem. Kin, would you please 
provide your suggestions!
#16, I still see two lines at the end.
I doubt that BRs cause this bug.

Try my patch below and test as following steps:
1. open text mail composer
2. type message body
3. do "Save as Draft"
4. see terminal window
My patch outputs mail body string in "BODY:%s:END" format at nsMsgCompose.cpp.

First, I typed "1[Enter]2" as message body.
Then, the output is
----------
BODY:1
2
:END
----------
Second, I typed "1[Enter]3[Enter]" as message body.
The output is 
----------
BODY:1
3
:END
----------
Both case has *one* new line char at last line.
These new line chars are made by EnsureVerticalSpace(0) of </BODY> or </HTML> at
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsPlainTextSerializer.cpp#829
.

Other routine adds one more new line char.
It is not nsPlainTextSerializer in my opinion.
This dumps message body string.
I realized this bug contains two bugs.
1. On saving composed message as Draft, one line break is added.
2. On receiving message via POP, line breaks are added.

On this bug, let's discuss about #1.
(I filed #2 as Bug 163433.)
Summary: Mail Editor adds two blank lines at the end → Mail Editor adds two blank lines at the end on saving as Draft
It's not receiving, it's _sending_.

When Mozilla sends a message empty line(s) are added at the end.
It's true.
But, receiving via POP also adds.
I can easily prove it (see Bug 163433).

If we find the evidence which the cause of sending problem is different from one
of saving as Draft, we should file it as other bug.
Oh, sending message has a bug.
I found the evidence. (I filed as Bug 163783)

Here is the new list to see which part adds new line.
1. On saving composed message as Draft, one line break is added. (this bug)
2. On receiving message via POP, line break(s) is/are added. (Bug 163433)
3. On sending message, one line break is added. (Bug 163783)

This bug tracks #1.
This bug is not ASSIGNED, but has a target milestone in the past. Is there a
chance to get this fixed? How does the test patch work out? There is still a
chance for 1.2b.

pi
Using Build ID 2002111512,
this bug also happend when you sent an email using a .txt signature.
The signature is a windows plain text file with no empty line at the end.
Look at bug #180558 for more info.

Also, there are many bugs about new lines, maybe we can put all this bugs
together in one big bug:
bug #163433
bug #71433
bug #180558
bug #163783

Also, should this bug be in the MailNews?
Because when people has this problem, they search in the MailNews product, not
in the Browser product.
Attached file test-signature.txt
the signature used in my testcase
After testing in WinXP Build ID: 2002111512

I propose this, Change bug #70728 to:
Summary: Mail Editor adds blank lines at the end when "Saving as Draft" and "Sent"
Product: MailNews
Description:
Mail Editor adds,
0 lines when saving as draft, format txt,  without signature
0 lines when saving as draft, format html, without signature
2 lines when saving as draft, format txt,  with signature
2 lines when saving as draft, format html, with signature

1 line when sending an email, format txt,  without signature
1 line when sending an email, format html, without signature
2 line when sending an email, format txt,  with signature
2 line when sending an email, format html, with signature

When 2 lines are added, there is also 1 line add before the signature.

For this test, I used the "File->Save as->File" method, when saving the draft
and when sending the email. All operation where done in Mozilla.
The signature used is in attachment #106566 [details]
Akkana: is this a known serializer issue?  Should Tanu be cc'd?  (I can't
remember her email).
Yes, Tanu should be cc'ed, but her email address has changed and I haven't been
able to figure out the new one.  Anyone know?
Adding Tanu.
That darned underscore.  Sorry I forgot, Tanu.
Keywords: mozilla1.3
Target Milestone: mozilla1.2alpha → Future
[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.3a) Gecko/20021212]

Confirmed: saving as draft a (modified) plain-text message (with no configured
signature) adds 1 new line, each time.

I too suggest to change the Summary: for example to
"in MailNews Composer, 'Save as Draft' adds blank line(s) at the end of the
message body"

I suggest to change the Severity from 'Normal' to 'Minor',
as the obvious workaround is to delete the added line(s) manually.

I support comment 25 for making this bug visible for Product/Component =
MailNews/Composer (!?), may be by creating a fake bug which would depend on the
current one !?
Attached patch patch v1 (obsolete) — Splinter Review
This patch fixes this bug.

In my investigation, Mozilla adds one new line at the end in saving as Draft.
Probably, this new line is used as the separator of messages in mbox file.
So, when you re-edit the draft, Mozilla should remove the last new line for the
consistency.
But it does this even on an IMAP server, where it doesn't know about the mailbox
format. I think it shouldn't add the line at all, and mbox handling should take
care of its own format.
Comment 34 seems worth looking at.

I have POP3 (== drafts on local disk, in 'mbox' format) only, so I can't comment
on IMAP4.
Comment on attachment 115079 [details] [diff] [review]
patch v1

Oh, that's true.
My patch is meaningless for IMAP.
Attachment #115079 - Attachment is obsolete: true
Attached patch local folder patch v1 (obsolete) — Splinter Review
How about this one?
Attachment #115381 - Flags: review?
Reply to comment 37:

(I'm not qualified to review the code in details; but, just curious:)

You move the empty line addition from the message itself to the mbox writing: it
seems right.
But shouldn't you add an empty line removal to the mbox reading too, more like
what you proposed in your previous patch ?
Comment on attachment 115381 [details] [diff] [review]
local folder patch v1

Sorry, again.
My patch doesn't work in adding an empty line correctly.
Then, local mail box has no empty lines as separator.
But it does work. I'm confused.

Does anyone know about specification of mbox format?
Does it need an empty line or no empty lines as separator?
Attachment #115381 - Attachment is obsolete: true
Attachment #115381 - Flags: review?
Some reasoning of my own: at the moment mbox folders work fine, whether you
download, copy or save draft mail there. Either the mbox writer takes care of
the format (which is good), or everywhere else an empty line is added (which is
bad). Without taking a look at the code I think it might be sufficient to just
remove the code your local folder patch v1 removed from nsMsgSend. 
And then, if the removal of addition of empty line causes a problem in mbox, the
mbox writer should be fixed. What I mean is that something like nsMsgSend should
not need to be aware of the underlying storage.
Mbox need a blank line.
There is the format spec. (Thanks, Karsten!)
http://www.qmail.org/qmail-manual-html/man5/mbox.html

MESSAGE FORMAT
     A message encoded in mbox format begins with a  From_  line,
     continues  with a series of non-From_ lines, and *ends with a blank line*.
Huh, scary. I didn't know nsMsgSend handles the mbox format :/ Then, I guess it
will have to add the empty line for mbox when saving the message and remove when
loading the message, but I think it still shouldn't do that for IMAP.
Attached patch local folder new patch v1 (obsolete) — Splinter Review
This is what we want.
Reply to comment 44:

At least, this third patch attempt looks much more like what I imagined :-)

Now is the right time to ask for review !?
Attachment #115513 - Flags: review?
Comment on attachment 115513 [details] [diff] [review]
local folder new patch v1

Tried this on Linux and it works well. The message is forced to end with a
newline, but there are no extra newlines, even after several edits.
Attachment #115513 - Flags: superreview?(sspitzer)
Attachment #115513 - Flags: review?(bienvenu)
Attachment #115513 - Flags: review?
this change requires a lot of testing as it's a pretty fundamental change. If
I'm reading this right, msg hdrs no longer include the blank line at the end of
the message in the message size, or the line count, so lots of code is affected.
You'd need to test folder compaction, copying messages between local folders
(and up to imap folders), regenerating .msf files, etc.
Attached patch local folder patch v2 (obsolete) — Splinter Review
This patch has two major changes.
1. Parsing separators of mbox format.
   It can handle not only CRLF(Win) but also LF(UNIX) and CR(Mac).
2. Writing out separators with platform-dependent linebreak (MSG_LINEBREAK).
   This fixes Bug 36797, too.
Attachment #115513 - Attachment is obsolete: true
Attachment #117539 - Flags: superreview?(sspitzer)
Attachment #117539 - Flags: review?(bienvenu)
> 2. Writing out separators with platform-dependent linebreak (MSG_LINEBREAK).
>    This fixes Bug 36797, too.

A mistake.
This uses MSG_LINEBREAK not only for separators but also all linebreaks.
So, new mbox file has platform-dependent linebreaks only.
(Currently, windows linebreak (CRLF) is  used for all platforms. This is a wrong
implementation.)
Of course, if you continue to use old mbox files, it mixes linebreaks with CRLF
and MSG_LINEBREAK. (This patch can handle this situation also.)
There are a lot of people using the *same* mbox file with Linux and Windows on
the same PC - will this still work?
Though I haven't tested yet, it must work.
Reply to comment 48:

You may want the current bug to block bug 36797.
(unless you make two separate patches)
Blocks: 36797
> There are a lot of people using the *same* mbox file with Linux and Windows on
> the same PC - will this still work?

The answer is yes.
I tested this situation and it worked.
This bug has a proposed patch, and the "mozilla1.3" Keyword:
Asking for "blocking1.4b" Flag... (just in case)
Flags: blocking1.4b?
Comment on attachment 117539 [details] [diff] [review]
local folder patch v2

This patch has a problem in the compaction of folders with large attachment
file.
Attachment #117539 - Attachment is obsolete: true
Attached patch local folder patch v3 (obsolete) — Splinter Review
I fixed the problem in the compaction.
And it saves a mail message with attachments to a folder faster than v2 patch.
Attachment #120434 - Flags: superreview?(sspitzer)
Attachment #120434 - Flags: review?(bienvenu)
why is it not possible to fix this just in the code that saves a message as a
draft? Touching all that other code makes me very nervous.
Flags: blocking1.4b? → blocking1.4b-
Attached patch simplified local folder patch (obsolete) — Splinter Review
This is a simplified one.
What this patch does is:
1. write out the mbox separator in saving message to local mail folder.
2. skip the mbox separator in parsing local mail folder.
Attachment #121966 - Flags: superreview?(sspitzer)
Attachment #121966 - Flags: review?(bienvenu)
Attachment #115513 - Flags: superreview?(sspitzer)
Attachment #115513 - Flags: review?(bienvenu)
Attachment #117539 - Flags: superreview?(sspitzer)
Attachment #117539 - Flags: review?(bienvenu)
Attached patch simplified local-folder patch v2 (obsolete) — Splinter Review
This catches up with the trunk.
Attachment #120434 - Attachment is obsolete: true
Attachment #121966 - Attachment is obsolete: true
Attachment #124987 - Attachment description: up-to-date simplified patch → simplified local-folder patch v2
Attachment #124987 - Flags: superreview?(sspitzer)
Attachment #124987 - Flags: review?(bienvenu)
I don't understand all the fuss about platform dependend vs. independend line
breaks. But as far I could test this patch does what it should. Code to add a
mbox separator moved to the right place and skipping the separator when reading
was enabled.

The patch is here for over four month now without objections made. Shouldn't we
give it a try for 1.6a? That's what alpha version are for.
It doesn't look like peterv or bienvenu are reading review requests.  Cc'ing
mscott in case this bug affects thunderbird.
Component: DOM to Text Conversion → Composition
Product: Browser → MailNews
No one ever requested a review request on a patch in this bug from me, which is
quite logical since I don't do mailnews reviews.
Attached patch simplified local-folder patch v3 (obsolete) — Splinter Review
Up-to-date patch to fix the bug of adding the waste linebreak in saving as
Draft.

Details:
1. write out the mbox separator in saving message (nsMsgFolderCompactor.cpp,
nsLocalMailFolder.cpp)
 and do not add waste newline. (nsMsgSend.cpp)
2. recognize the mbox separator(CR+LF,LF,CR) in parsing local mail folders.
(nsParseMailbox.cpp, nsParseMailbox.h)
3. performance improvement (find_linebreak() in nsLocalMailFolder.cpp)
Attachment #124987 - Attachment is obsolete: true
Attachment #132997 - Flags: superreview?(sspitzer)
Attachment #132997 - Flags: review?(bienvenu)
Attachment #124987 - Flags: superreview?(sspitzer)
Attachment #124987 - Flags: review?(bienvenu)
Attachment #121966 - Flags: superreview?(sspitzer)
Attachment #121966 - Flags: review?(bienvenu)
Attachment #120434 - Flags: superreview?(sspitzer)
Attachment #120434 - Flags: review?(bienvenu)
Shotaro, should this really be in the final patch?
+    printf("m_last_empty_lines = %d\n", m_last_empty_lines); //debug
Oops. Removed debug messages.
Attachment #132997 - Attachment is obsolete: true
Attachment #132997 - Flags: superreview?(sspitzer)
Attachment #132997 - Flags: review?(bienvenu)
Attachment #132999 - Flags: superreview?(sspitzer)
Attachment #132999 - Flags: review?(bienvenu)
Comment on attachment 132999 [details] [diff] [review]
simplified local-folder patch v3.1


Any chance to get s= and sr= before v1.6b ?

(I wonder if r/sspitzer and sr/bienvenu would be better: sspitzer as module
owner, both as super-reviewers for it)
*** Bug 215134 has been marked as a duplicate of this bug. ***
*** Bug 227271 has been marked as a duplicate of this bug. ***
I still don't see why this bug isn't fixed in the code that saves a draft
message, instead of affecting every message copy, every mail folder parse, etc.
If saving as draft is adding an extra line, lets remove that extra line in the
save draft code.
David, as I understand it, the problem is, that currently the blank separator
line is read as part of the message body.
We cannot not write out the line as it's needed by the mbox format.
Mozilla clearly needs higher lever Mail API independed from the underlying 
storage. I must admit I don't know what is the state with this, but that tight 
binding to the "mbox" format causes all kind of problems as seen in Bug 194382.
 If this change gets checked in, it will potentially cause problems with forward
compatibility. If an older version of Mozilla reads db's generated by a newer
version of mozilla (there's nothing to prevent this, and I do it all the time -
run an older stable build some of the time, and a newer build some of the time),
the older version of Mozilla will think that the message size/line count
includes the blank line and thus the blank separator lines might not be
maintained across things like folder compaction. It's unfortunate that the
message size/line count incorrectly includes the blank line at the end, but
that's the situation we're in. We could rev the db version number, but that
would invalidate all .msf files out there, not a nice thing to do.

So I need to think how much trouble that's going to cause, and if there's not
another way of dealing with this.
Assignee: peterv → sspitzer
QA Contact: sujay → esther
*** Bug 247856 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Blocks: 290042
Comment on attachment 132999 [details] [diff] [review]
simplified local-folder patch v3.1

sorry, I really don't like working around the problem this way.
Attachment #132999 - Flags: review?(bienvenu) → review-
This also seems to affect the "Edit as new" feature. In this case two blank lines are added at the end, too.
*** Bug 290042 has been marked as a duplicate of this bug. ***
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Filter on "Nobody_NScomTLD_20080620"
QA Contact: esther → composition
Product: Core → MailNews Core
Comment on attachment 132999 [details] [diff] [review]
simplified local-folder patch v3.1

Clearing old superreview request - according to the review- this would need an new patch anyway.
Attachment #132999 - Flags: superreview?(sspitzer)
Severity: normal → minor
Priority: P3 → --
Whiteboard: [patchlove][has draft patch][needs new assignee]
Target Milestone: Future → ---
I do not see this in TB9. Is this still happening for anyone?
I can confirm that this behavior is no longer visible in TB 9.0.1, neither in Save As Draft nor in Edit As New.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: