261467 - Spaces not stripped from end-of-line on Send (plain-text compose, wrap column=0)

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Jim Kilmer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910

When composing & sending a message, if a line has one or more trailing spaces
before a carriage return, the return is stripped before the message is saved or
sent.  The message saved in the "Sent" mailbox is identical to what the receiver
sees.  However, if that same message is then re-opened using the "Edit Message
as New" function, the return is again present -- but it is again stripped if
that message is sent.

This has been occurring consistently since at least Mozilla 1.4, but seemed
random enough that I was never able to nail it down.  This seems very similar to
Bug 99922, but that bug reported (back in 2001) that the _spaces_ were stripped.
 Currently, it seems like the spaces are retained, but the [return] is stripped.

Reproducible: Always
Steps to Reproduce:
1.  Create a message similar to the following.  Each line's text indicates what
non-printable characters are placed at the end of the line.

no trailing spaces, 1 return
no trailing spaces, 2 returns

1 trailing space, 1 return 
1 trailing space, 2 returns 

4 trailing spaces, 1 return    
4 trailing spaces, 2 returns    

end of test

2) Send the message

Actual Results:  
The message saved in the "Sent" mailbox, and the message received by the
recipient looked like:

no trailing spaces, 1 return
no trailing spaces, 2 returns

1 trailing space, 1 return 1 trailing space, 2 returns
4 trailing spaces, 1 return    4 trailing spaces, 2 returns    
end of test 

Note that the trailing spaces are maintained, but in all of the "1 return" cases
with trailing spaces, the return is missing.  In the 2 return cases, the second
return is still retained, leaving 1 CR in the final result.

Expected Results:  
The received (and saved) message should have been identical to the entered message.

Comment 1

[Jim diGriz]

Duplicate of Bug 197130 ? 
"Mail window doesn't show word wrap of plain text message as generated by mail
compose window"

In Bug 251671 "Failed to wrap line in message window." I wrote:

Help_>About:config

Check your setting for "mailnews.display.disable_format_flowed_support"
Try setting this to true.

<http://oceanpark.com/notes/howto_mozilla-mail-plain-text-word-wrap.html>

Comment 2

[Mike Cowperthwaite]

HTML composition, with the message converted to plain text?

This looks like bug 125928, but that only occurs when more than one space is at 
the end of the line.  And in fact, using your test data in an HTML compose 
window, I see these results:
=====
no trailing spaces, 1 return
no trailing spaces, 2 returns

1 trailing space, 1 return
1 trailing space, 2 returns

4 trailing spaces, 1 return    4 trailing spaces, 2 returns    
end of test
=====

Comment 1 is correct that this is related to format=flowed -- the failure to 
strip all spaces from the end of the line when converting to plain causes the 
lines to be interpreted as flowable.


With plain text composition, I see six separate lines.  Same with HTML 
composition sent *as* HTML.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a4) Gecko/20040924


Jim Kilmer, if on retesting you see the same results as I have described, please 
mark this bug as a dupe of 125928.  Otherwise, please provide more information 
on how to reproduce this problem.

Comment 3

[Jim Kilmer]

I don't believe this is a dup of bug 125928, as it definitely, consistently
occurs with only one space at the end of the line -- not just multiple spaces. 
Also, this occurs with plaintext composition, not HTML composition.  So, while
almost certainly related, it doesn't seem to be a clear dup.  I have confirmed
that the issue does NOT occur when using HTML composition.

The suggestion in Comment 1 regarding setting
"mailnews.display.disable_format_flowed_support" to true _did_ correct this
behavior.  However, reading the reference link provided, it seems that this was
intended to fix issues with auto-linebreaks when wrapping lines.  It even states
"Only hard newlines that I actually entered manually were properly displayed." 
In this case, it's the hard newlines that _aren't_ being properly displayed.

The same could be said for Bug 168420, and the mini-FAQ attached there.  It
dances around describing this behavior, but focuses on the auto-linewrapping and
the whole 72-column thing.

From my perspective, this is still a bug as it provides unexpected and arguably
erroneous behavior from an "out of the box" Mozilla install set to compose
messages in plaintext.  I think there's probably an issue here that has been
obscured in the whole discussion over what's correct or not for _wrapped_ lines.
 For a plaintext message whose line content doesn't extend to the 72-column
mark, I don't know if it can be considered correct to strip or ignore a hard
newline entered by the user during composition.  

Comment 4

[Mike Cowperthwaite]

Are you running with any extensions in place that might interfere with the plain 
text composition mode (such as Enigmail)?  I know you say you've got an "out of 
the box" installation, but I do not, and never have, seen the behavior you're 
reporting.

Comment 5

[Jim Kilmer]

No extensions.  I'm quite confused as to why it would not be replicatable, as I
did a fresh install on my new laptop over the weekend, and it yielded identical
results to my desktop @ work.  I would be happy to create an attachment
containing my config settings, or any other information, if it would be of
assistance.  If you can't duplicate this, I have no idea how to helpfully
proceed... 

Comment 6

[Mike Cowperthwaite]

OK, try this: 
 - Compose the message with the test data; save as draft; save the message from 
Drafts as a .eml file.  
 - Send the message; save the message from the Sent folder as a .eml file.  
 - Attach both files to this bug.

Comment 7

[Jim Kilmer]

Attachment: test message, saved as draft

Comment 8

[Jim Kilmer]

Attachment: test message, saved from Sent mailbox

Comment 9

[Jim Kilmer]

Attachment: Screenshot of results inside Mozilla mail

As the contents of the .eml files attached to this bug appear correct and
identical, I'm attaching this screenshot of that message as viewed inside
Mozilla Mail as well, if only to prove that I'm not going insane...

Comment 10

[Mike Cowperthwaite]

I'm confirming this bug based on the attachments, but I can't reproduce the 
problem myself with Moz 1.8a4-0924 or TB 0.8 (20040913), Win2K.

Comment 11

[Mike Cowperthwaite]

Just to double check, I retested after setting
  mailnews.send_plaintext_flowed
to false -- I still get the same results of spaces stripped from end of line, 
and the same display of ten lines (three blank, seven with text).

Comment 12

[Mike Cowperthwaite]

On a hunch from reading bug 180451 (which appears to be about quite the opposite 
behavior), I reproduced this bug after setting
  mailnews.wraplength   to    0

Jim Kilmer, please confirm that you've got your wrap column set to 0.

Comment 13

[Jim Kilmer]

Confirmed - my mailnews.wraplength config option is set to 0.

Comment 14

[Mike Cowperthwaite]

xref bug 125928

Comment 15

[Mike Cowperthwaite]

*** Bug 184981 has been marked as a duplicate of this bug. ***

Comment 16

[Jason Jackson]

(In reply to comment #15)
> *** Bug 184981 has been marked as a duplicate of this bug. ***

I'm the original reporter of 184981.  Reading the comments here, it sounds
completley different to me, but I'm not a programmer.  I've seen the bug in many
versions of seamonkey, and now in thunderbird 1.0.x.

I have a new way of clarifying my bug.  When I view any of the messages with the
bug in the "Message Pane" or in its own window, the empty line(s) is missing. 
When I right click the message and "Edit As New" (or when I originally was
composing it), the empty line(s) is there.

When or how seamonkey/thunderbird generates these "bugged" messages is still a
mystery to me, but once they're created, they display this behavior consistantly
forever.  I already uploaded a bugged message (.eml) with this behavior in #
184981.  If you ever want more, let me know.

Comment 17

[Jason Jackson]

(In reply to comment #16)

Apparently I was supposed to post my previous comment under 184981.  We've
followed up with it there, but it appears it was the fault of having a setting
of mailnews.wraplength to 0.

Comment 18

[Mike Cowperthwaite]

*** Bug 323217 has been marked as a duplicate of this bug. ***

Comment 19

[Mike Cowperthwaite]

*** Bug 336583 has been marked as a duplicate of this bug. ***

Comment 20

[Mike Cowperthwaite]

*** Bug 347074 has been marked as a duplicate of this bug. ***

Comment 21

[Torben Jensen]

*** Bug 352559 has been marked as a duplicate of this bug. ***

Comment 22

[Andriy Tkachuk]

I think it is not correct behavior of composer to set format=flowed for saved/sent mails that were created with manual wrapping (i.e. when automatic wrapping is switched off - mailnews.wraplength=0).

Comment 23

[Andriy Tkachuk]

The possible workaround for this i think would be to set mailnews.send_plaintext_flowed=false if you don't use automatic wrapping (i.e. if mailnews.wraplength=0)

Comment 24

[Andriy Tkachuk]

Attachment: proposed patch

Comment 25

[Boris Zbarsky [:bzbarsky]]

Please ask someone else for sr; I'm not going to get to this in a reasonable time frame (months, at least).

Comment 26

[timeless]

Comment on attachment 265424 [details] [diff] [review]
proposed patch

i understand strings and can understnad mailnews at times. however i'd rather leave format=flowed stuff to people who understand it

Comment 27

[Scott MacGregor]

Comment on attachment 265424 [details] [diff] [review]
proposed patch

I know someone who is very good at understanding the impact to format-flowed :)

Comment 28

[J Luh]

 (In reply to comment #24)
> Created an attachment (id=265424) [details]
> proposed patch
> 

Andriy Tkachuk, can you briefly explain what this patch does, and how it fixes this bug? It's not obvious from the code (comments may help). It doesn't seem to do what you described in comment 23; anyway, please explain further. Thanks.

Comment 29

[Andriy Tkachuk]

Sure. The patch just strips spaces from end of lines if wrap_column=0 and f=f is used. Without this the text in f=f aware mail viewers will be interpreted incorrectly, i.e. lines w/ spaces at the end will be joined w/ next lines (that is incorrect if wrap_column was set to 0, as far as I understand).

Comment 30

[J Luh]

(In reply to comment #29)
> Sure. The patch just strips spaces from end of lines if wrap_column=0 and f=f
> is used. Without this the text in f=f aware mail viewers will be interpreted
> incorrectly, i.e. lines w/ spaces at the end will be joined w/ next lines (that
> is incorrect if wrap_column was set to 0, as far as I understand).
> 
Andriy Tkachuk, thanks for the additional explanation. If I understand you correctly, this patch is not designed to fix the behavior described in comment 1 (at least not in all cases), but was designed to address a more specific situation when wrap_column=0 and format=flowed.

I do not think the behavior of your patch is correct. As I understand it, if Thunderbird is set to (1) force manual wrapping and (2) use format=flowed, your patch would trim trailing spaces from each line. You are correct that in  format=flowed content, trailing spaces before user-inserted hard line breaks should be trimmed (see RFC 3676 section 4.2). And you are correct that this means all trailing spaces in a manually wrapped message will be trimmed. But the reason this is true is because all hard line breaks in a manually wrapped message are "user-inserted"--it is not because the wrap method was set to "manual" instead of "automatic."

To show what I mean, suppose I create the following message (# means a hard line break inserted using the Enter key)
Hello #
world #

If I am using format=flowed, the trailing spaces in both lines of the message should be trimmed regardless of whether I have the wrap method set to manual or automatic (the default wrap method doesn't make a difference, because both the line breaks in the message were "user-inserted" hard line breaks). However, your patch would trim the spaces only if I had the wrap method set to manual; it would not trim the spaces if I had it set to automatic.

Please let me know if I am misunderstanding your patch or the spec. I am not familiar enough with the Composer code to suggest another approach, but maybe others can help. Thanks.

Comment 31

[Andriy Tkachuk]

(In reply to comment #30)
> However,
> your patch would trim the spaces only if I had the wrap method set to manual;
> it would not trim the spaces if I had it set to automatic.

Exactly. In automatic wrapping trimming is performing well, the bug is appearing only if wrap_column=0. When wrap_column=0 preformatted code path is executing that does not perform trimming (see comment at http://lxr.mozilla.org/mozilla1.8/source/content/base/src/nsPlainTextSerializer.cpp#1671)

Comment 32

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 33

[J Luh]

(In reply to comment #31)

Sorry, I have been busy and have not had time to look at this lately. The logic sounds correct as you describe it, but I have not been able to look into the composer code more deeply. Perhaps someone who is more familiar with the composer code or has more time can take a look at this sooner; otherwise I will try to look into it more when I have more time.

Comment 34

[J Luh]

Comment on attachment 265424 [details] [diff] [review]
proposed patch

I think this behavior is correct--thanks and sorry it took so long.

Comment 35

[Ben Bucksch (:BenB)]

I wonder who still really groks this code (there's so much newline- and space- searching and skipping), but at least the change matches the change from bug 125928 in the |else| block.

Comment 36

[Magnus Melin [:mkmelin]]

Ben, did you intend to r+ this? Or was that a still thinking about it?

Comment 37

[Mike Cowperthwaite]

This fix looks like it works for the trailing-space issue.  However, I notice that lines with leading spaces, or leading '>' signs, are sent thru unaltered
-- that is, they do not get the additional 'stuffing' space -- with wrap=0, and there's nothing in the patch that appears to address that.

xref bug 215068.

Comment 38

[Andriy Tkachuk]

Attachment: patch2

Previous patch did not honor "-- " (signature delimiter - RFC 2646, 4.3)

Comment 39

[Andriy Tkachuk]

Comment on attachment 284993 [details] [diff] [review]
patch2

proper fix provided in patch to bug 215068

Comment 40

[Andriy Tkachuk]

Sorry, I was mistaken - there is no fix to this bug there and patch2 is not the right one - it does not take into account signature delimiter in PGP-signed mails and also it removes leading space(s?) in first mail line in HTML <pre> compose. I hope I will provide proper fix soon.

Comment 41

[Andriy Tkachuk]

Ok, from now find the right patch to this bug in bug 215068. The reason is that fixes are connected.

The actual fix is in these lines:

+        if ((outputLineBreak || !spacesOnly) && // bugs 261467,125928
+            !stringpart.EqualsLiteral("-- ") &&
+            !stringpart.EqualsLiteral("- -- "))
+          stringpart.Trim(" ", PR_FALSE, PR_TRUE, PR_TRUE);

the case is when there is !spacesOnly in input line. The outputLineBreak case fixes bug 125928.

Comment 43

[Mike Cowperthwaite]

I can confirm that the end-of-line spaces are now properly stripped from a plain-text compose message sent as f=f when the wrap column is 0.

Marking FIXED: patch at bug 215068, per Andriy's comment 40.

However, there is still a bug seen with wrap-column = 0: Lines that are typed in with a leading '>' are not space-stuffed and so are treated as quotes rather than text.  I've opened bug 441020 for this.

Comment 45

[Ben Bucksch (:BenB)]

VERIFIED to be fixed on trunk with both plaintext composer and HTML composer.