125928 - HTML composition converted to PlainText fails to strip spaces before hard breaks with format=flowed

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Joe Infla]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002020406 (M0.9.8)

On viewing a plaintext message, a line with two or more trailing spaces followed
by two carriage returns causes one carriage return not to be displayed.

Reproducible: Always
Steps to Reproduce:
1.  Compose a message to yourself with the following body text:
"Hello.<space>

This line should have empty lines above and below.<space><space>

Does it?"
2.  Send the message in Plain Text Only format.
3.  View the message when it arrives or go to your Sent Messages folder.

Actual Results:  The message body will show the following:
"Hello. 

This line should have empty lines above and below.  
Does it?"

Expected Results:  Message body should appear as it was composed. (shown above)

Note that leaving fewer than two spaces at the end of a line doesn't cause a
problem, but two or more spaces (I tested up to five) causes the loss of a
carriage return.

Note that viewing any of these messages in Pine (or presumably another email
client) displays correctly.  I also opened a text file with appropriate examples
in the Composer and Browser apps, and they displayed fine.  But when I saved the
message in .eml format and viewed that in the browser, it displayed incorrectly.

The closest thing to this I could find was Bug 113120, but I can't tell what the
purpose of that bug is.

Comment 1

[Dac Chartrand]

WORKS FOR ME 

Windows 2000
./nightly/latest-1.0.0/
Build 2002041617

Reporter: Have you tried a new build? 

Can someone (reporter?) mark this as fixed or Works for me?

Comment 2

[Jean-Francois Ducarroz]

per user comment, WFM. Please reopen if you are still able to reproduce this
problem with a recent build.

Comment 3

[Joe Infla]

This is the reporter, now using trunk build 2002042510 on WinXP.

There is no change in behavior for me on this bug.  I followed the exact
instructions of the original report, and exactly the same thing happened.

If anyone's interested, I can send an email-example to you instead of just
reporting what the email shows.

Comment 4

[Joe Infla]

This is the reporter again.  I've seen this bug on the following builds/platforms:
Trunk-2002071708, Win NT 4.0
1.0-Branch-2002071206, Win XP Pro
and several others between the original report on 0.9.8 and now, reproduced on
three different computers and three POP3 accounts.  As such, I'm confirming the bug.

For easier testing, copy/paste the following four lines into an email and send
as plain text only:
----  
----  

----

Send through Mozilla mail and read in Mozilla mail, and you will see this:
----  ----  
----

Only part of this problem exists if the message is read through Hotmail (don't
know why), and a different but largely unnoticeable problem exists if you send
in rich text.

Comment 5

[Kathleen :Brade]

Akkana--is this a bug for Tanu?

Comment 6

[Akkana Peck]

It might be related to another bug Tanu is already working on ...

Comment 7

[Peter Tandler]

I can confirm this bug. (If this is of any interest to someone ...)

Comment 8

[Mike Cowperthwaite]

See also the related bug 195513.

Comment 9

[Joe Infla]

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

Comment 10

[Joe Infla]

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

Comment 11

[Mike Cowperthwaite]

Joe Infla, is this bug still a problem for you?  I just followed the 
instructions in the original report, using Moz1.6a-1003, Windows 2000, and the 
email came thru with the empty lines as specified.

I had the 'send_plaintext_flowed' preference set to True.  I put spaces into the 
text as indicated.  The received message had the trailing spaces stripped by 
Mozilla, and the line breaks were preserved.

If this is working for you, please resolve this bug as:  Resolved|WorksForMe

Comment 12

[Joe Infla]

In build 2003101104 winxp, the bug still exhibits exactly as described, even in
a new profile.  The pref you mentioned was also set to true (per default).

Please note the much simpler testcase in comment 4.  Highlight those four lines,
copy/paste them into the body of a new message, and select Options --> Format
--> Plain Text Only.  Send the message, and it arrives as described in comment
4.  The copy of the message in your Sent folder also exhibits the bug.

NOTE: View message source shows the correct content (that is, the bug is in
message display, rather than in how it's sent or copied).

Comment 13

[Mike Cowperthwaite]

I did try both test cases, and both looked correct on arrival and in the Sent 
folder.

> NOTE: View message source shows the correct content

Does the text in the message source include trailing spaces on those lines, or 
not?  In my case, which is displaying correctly, the answer is Not.

I suspect this problem is a preference setting, but I have no idea which one 
might be affecting this.  Could you save one of the messages that's failing for 
you as a .eml file and attach it to this bug?

Comment 14

[Joe Infla]

Attachment: Message shows bug - sent from Mozilla

Just for grins, I tested this via an aol account under Netscape 7.1 also.  The
bug occurs if sent within mozilla mail, but not if sent by the webmail.aol.com
web interface.	Both messages will be attached, but testing showed that the
format=flowed part of the content-type is the culprit.	Without that argument,
the .eml file displays normally in a browser window; with it, the bug crops up.

Comment 15

[Joe Infla]

Attachment: Message doesn't show bug - sent from webmail.aol.com

Comment 16

[Joe Infla]

Also,
> Does the text in the message source include trailing spaces on those lines,
> or not?  In my case, which is displaying correctly, the answer is Not.

The message source does show the trailing spaces.  In fact, both message display
and message source for both the attached .eml files show the trailing spaces. 
Why is my setup different from yours in that regard?

Comment 17

[Mike Cowperthwaite]

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

Comment 18

[Joe Infla]

Regarding the pref mentioned in comment 11:
Changing the *other* format=flowed pref:
mailnews.display.disable_format_flowed_support to True
causes this behavior to cease.  But still, the new profile default for that pref
is false, eliciting the bug.

Comment 19

[Mike Cowperthwaite]

Joe: I don't think that preference is the key, either; I generally have that set 
to false (altho I have been setting it True occasionally during recent tests).  
I just tried a specific test with that preference set False, and I still see the 
same results: end-of-line spaces stripped, text of received message displayed as 
expected.

You aren't by any chance composing the text with the HTML editor and then seeing 
these results in messages that have been converted to text?  I tried that and 
saw the same results you are seeing.

Comment 20

[Joe Infla]

As a matter of fact, that's exactly what I've been doing.  I have "Compose
messages in HTML format" checked in account settings, but frequently write to a
domain that I've designated as a plaintext domain, so it gets converted when I
send.  Unchecking the pref and then composing/sending yields no bug.

I should note, since I haven't before, that the reason this bug is so bothersome
is that I was trained to type two spaces following the period at the end of a
sentence.  So paragraphs run together in my emails unless I specifically
remember to delete the paragraph-ending spaces.

Comment 21

[Mike Cowperthwaite]

Ah-ha.  Updating summary.

I figured out to ask that question by browsing the code.  Conversion of HTML to 
plain takes a different path than simply copying a plain-text edit buffer to a 
message; the former goes thru 
  QuotingOutputStreamListener::ConvertToPlainText(PRBool formatflowed)
  [nsMsgCompose.cpp]
whereas the latter goes thru
  nsPlaintextEditor::OutputToString(...)
  [nsPlainTextEditor.cpp]
and the final mechanisms that handle the f=f formatting are in two different 
objects.  The redundancy bites Mozilla in the butt.

Comment 22

[Mike Cowperthwaite]

Additional information: during the HTML->plain text conversion with f=f, lines 
beginning with spaces are not correctly space-stuffed -- that is, they are not 
prefixed with an extra space.

At the end of the line, one space is stripped from the text (rather than all 
spaces, as with the plain-text composition).  So the original problem of this 
bug only shows up with two or more spaces at the end.

Comment 23

[Mike Cowperthwaite]

Requesting blocking.  HTML->Plain happens a lot, and the result of such mail can 
easily be sent out with invalid flowed formatting.

Comment 24

[Mike Cowperthwaite]

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

Comment 25

[Mike Cowperthwaite]

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

Comment 26

[Mike Cowperthwaite]

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

Comment 27

[Mike Cowperthwaite]

The space-stuffing symptom mentioned in comment 22 apparently has been fixed; at 
any rate, I'm not seeing it anymore in 1.6 Final.

Comment 28

[Christian Franke]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6) Gecko/20040113]

The problem still occurs when text from a received plain text (no format=flowed)
mail with trailing spaces is copied to a new mail and resent (with f=f).

Workaround: "Paste Without Formating" preserves the formatting ;-)

The bug is possibly in the conversion of <pre>...</pre> to plaintext.

Steps to Reproduce:
1. Compose a new mail in HTML Mode.
2. Enter the following text with Insert|HTML.. (Underline = Blank)
<pre>
Line 1_
Line 2_
Line 3_
</pre>
3. Send the mail to yourself as plain text with f=f.
4. View the received message.

Actual Results: "Line 1 Line 2 Line 3"

Expected Results: 3 Lines

Comment 29

[Mike Cowperthwaite]

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

Comment 30

[Frank Griffin]

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

Comment 31

[Mike Cowperthwaite]

Bug 265803 provides a bit of additional information relevant to this bug: the 
stripping of a single space from the end of the lines of text is part of the 
DOM->Text conversion, and probably shouldn't be happening at all.  (And doesn't 
happen at all for <pre> text, as noted in comment 28.)

What that means is, if that bug gets fixed, this bug will be *more* visible 
because lines with only a single trailing space would then keep it, causing the 
f=f renderer to reflow more lines that were not expected to be flowed.

Comment 32

[Mike Cowperthwaite]

(In reply to comment #31)
> the  stripping of a single space ... doesn't 
> happen at all for <pre> text

Since plain-text sigs are maintained within a <pre> block, this problem occurs 
more often in sigs -- e.g. bug 200641.


(In reply to comment #27)
> The space-stuffing symptom mentioned in comment 22 apparently has been fixed;
> at any rate, I'm not seeing it anymore in 1.6 Final.

I don't know what led me to post that, but this problem is still quite present, 
and on retesting 1.6, I see it there too; this is bug 215068.


Now that I understand why a single space was being stripped, it's clear that the 
problem of this bug is that the plain text message body is not being subject to 
*any* format=flowed formatting.

Comment 33

[Mike Cowperthwaite]

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

Comment 34

[Mike Cowperthwaite]

Another clue from bug 261467: for plain-text composition, spaces are not 
stripped from the end of lines, and lines beginning with spaces are not stuffed 
with an additional space, if the preference
   mailnews.wraplength   is set to   0

Comment 35

[Mike Cowperthwaite]

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

Comment 36

[Mike Cowperthwaite]

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

Comment 37

[Mike Cowperthwaite]

Additional symptom: a leading quotemark ('>') is not space-stuffed if the line 
is contained within a <pre>, but *is* space-stuffed for a line in <body> text.

Comment 38

[Mike Cowperthwaite]

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

Comment 39

[Mike Cowperthwaite]

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

Comment 40

[Mike Cowperthwaite]

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

Comment 41

[Mike Cowperthwaite]

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

Comment 42

[Mike Cowperthwaite]

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

Comment 43

[Mike Cowperthwaite]

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

Comment 44

[jettlogic-cul]

When is this bug (converting HTML to plaintext in reply-compose in Thunderbird 1.5 messes up lines on sending) going to get fixed?  Someone should mark this as a blocker for the next release, because it's really irritating  and has been around for years (I would just use plaintext as default except there is NO option to convert to HTML as necessary).

Also, after converting to plaintext the editing should be in monospace, not the variable-width font from the HTML mode.

Plus all the other quirks mentioned.

Comment 46

[Andriy Tkachuk]

Attachment: proposed patch

Comment 47

[Andriy Tkachuk]

Sorry, the patch is buggy - the approach to check the length of current line is incorrect, it won't work correctly for example for such lines:

123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789

Comment 48

[Andriy Tkachuk]

Attachment: proposed patch2

here is correct patch

Comment 49

[Andriy Tkachuk]

BTW - the bug is still there if composing in preformatted mode
(Format -> Paragraph -> Preformat)

Comment 50

[Andriy Tkachuk]

Attachment: proposed patch3

Here is the patch that work for preformatted text as well.

Comment 51

[Mike Cowperthwaite]

(In reply to comment #50)
> Created an attachment (id=263153) [details]
> proposed patch3
> 
> Here is the patch that work for preformatted text as well.

Bug 215068, right?

Thanks so much for working on this, Andriy!

Comment 52

[Boris Zbarsky [:bzbarsky]]

So...

1)  I'm probably the wrong person to review this patch for correctness; you want someone familiar with editor or mailnews composition, or at least with the details of how our space handling in the serializer works.  Someone like Simon ("smfr") or Daniel ("glazman"), I guess, or mscott or bienvenu.  Or maybe peterv?  I'm not sure he's that much happier than I am with this code.
2)  This patch looks conceptually wrong to me: why should non-breaking spaces be removed in some cases?  It feels like it's working around a bug elsewhere.  It might be good to explain why this is the right patch, either in the bug or in the comments.

Comment 53

[Andriy Tkachuk]

(In reply to comment #51)
> (In reply to comment #50)
> > Here is the patch that work for preformatted text as well.
> 
> Bug 215068, right?

Mike, I'm not sure.

Comment 54

[Andriy Tkachuk]

Boris, I’m sorry for the lack of explanations for the patch. Let me fix this a bit.

The problem here is that HTML composer puts non-breakable spaces (nbsp-s) if user enters more than one space (at the end of paragraphs as well). And I would not say that this is wrong behavior of HTML composer.

From the other side nsPlainTextSerializer::Write() for non-preformatted text process all ordinary spaces (for example, performs spaces compressing between words and spaces cutting at paragraphs ends) and at the end (in Output()) just replace all nbsp-s w/ ordinary spaces. This replacement cause problem with format=flowed mails we have here, because after replacement we still have the odd spaces at the end of paragraph (in case user entered several spaces at the end of paragraph) that will be interpreted wrongly by viewer.

I suppose that nothing wrong will happen if we will change nbsp-s at end of paragraphs to ordinary spaces and let them a chance to be included in spaces processing afterward. Note – not remove nbsp-s at all (as you mention) but again – shift them w/ ordinary spaces. We don’t want to do this change for all nbsp-s in text before spaces processing because ordinary spaces between words will be compressed then – I’m not sure this is what mail composer wants and probably other composers as well. In non-HTML composing mode there is no such spaces compressing between words, because in such case mPreFormatted flag is set.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Thank you for the explanation.  I still think I'm the wrong person to be reviewing this, but some comments:

The patch needs to be checking the OutputPersistNBSP flag, otherwise it'll regress bug 218277.

Perhaps we should in fact introduce yet another serializer flag for this behavior and have whatever code is serializing mails use that, if this is only a problem for format=flowed?  Or would this be an issue for consumers in general?

Do you want to be putting this code where you did, or at the 

1683   // XXX Copy necessary to use nsString methods and gain
1684   // access to underlying buffer
1685   nsAutoString str(aString);

part?  That is, do you want to affect the the "preformatted and not wrapping, or in pre, or ..." case?

Comment 56

[Andriy Tkachuk]

(In reply to comment #55)
> The patch needs to be checking the OutputPersistNBSP flag, otherwise it'll
> regress bug 218277.

Mike, I don't think it will regress the bug 218277 because nbsp-s are changed only at end of lines in the patch.

> 
> Perhaps we should in fact introduce yet another serializer flag for this
> behavior and have whatever code is serializing mails use that, if this is only
> a problem for format=flowed?

The problem is visible when format=flowed is set. For safety we can just run this code when OutputFormatFlowed is set.

> Or would this be an issue for consumers in
> general?

I'm not sure.

> 
> Do you want to be putting this code where you did, or at the 
> 
> 1683   // XXX Copy necessary to use nsString methods and gain
> 1684   // access to underlying buffer
> 1685   nsAutoString str(aString);
> 
> part?  That is, do you want to affect the the "preformatted and not wrapping,
> or in pre, or ..." case?

Yes, I do want to put the code where I did this. There might be some spaces processing before EOL in preformatted codepath as well. Moreover I put such line in "proposed patch3":

stringpart.Trim(" \t\r\n", PR_FALSE, PR_TRUE, PR_TRUE);

to fix the same bug (when odd spaces are not stripped at EOL) in preformatted text.

Comment 57

[Andriy Tkachuk]

Attachment: proposed patch4

Here is the patch w/ checking for OutputFormatFlowed for safety (to avoid possible regressions).

Comment 58

[Boris Zbarsky [:bzbarsky]]

> because nbsp-s are changed only at end of lines in the patch

There's no reason you can't have ends of lines in a <textarea>...

> For safety we can just run this code when OutputFormatFlowed is set.

That would seem prudent to me, yes.  Given that this is when we do the space stuffing also, right?  I assume that's what ends up breaking with the nbsp's.

> There might be some spaces processing before EOL in preformatted codepath as
> well

Might, or is?  Can that codepath even be hit when we're outputting format=flowed stuff?  I guess there's nothing really stopping it...

Basically, making string copies is a pretty inefficient thing to be doing.  We want to avoid making that extra copy if at all possible, especially since later we go ahead and make a copy _anyway_ (so we could use that one).  If we have to make this copy up front, take out the line where we make the other copy and just keep using this one?  Basically move the |1685   nsAutoString str(aString);| to where you're adding your code.

I guess with that, and if someone familiar with format=flowed ("mscott@moz", maybe?) reviews, I'd be happy to sr.

By the way, there are a few ways you could make this patch easier to review:

1)  More context (-u8 is good).
2)  Using the -p option to diff.
3)  Diffing against something like current trunk (so the line numbers will
    match up better).  Your diff seems to be against the version of this file
    from 2005-06-16; there have been 16 revisions since then...  Are you
    diffing against the 1.8 branch or something?

Comment 59

[Andriy Tkachuk]

(In reply to comment #58)
> > For safety we can just run this code when OutputFormatFlowed is set.
> 
> That would seem prudent to me, yes.  Given that this is when we do the space
> stuffing also, right?

Exactly.

> 
> > There might be some spaces processing before EOL in preformatted codepath as
> > well
> 
> Might, or is? 

In my patch - it is with this line:
stringpart.Trim(" \t\r\n", PR_FALSE, PR_TRUE, PR_TRUE);

> Can that codepath even be hit when we're outputting
> format=flowed stuff?  I guess there's nothing really stopping it...

It is hit. There is same issue w/ preformatted text as well.

> 
> Basically, making string copies is a pretty inefficient thing to be doing.  We
> want to avoid making that extra copy if at all possible, especially since later
> we go ahead and make a copy _anyway_ (so we could use that one).  If we have to
> make this copy up front, take out the line where we make the other copy and
> just keep using this one?  Basically move the |1685   nsAutoString
> str(aString);| to where you're adding your code.

Though I'm not sure this will noticeable improve the processing speed - basically I agree with you. I will adjust the patch.

> By the way, there are a few ways you could make this patch easier to review:
> 
> 1)  More context (-u8 is good).
> 2)  Using the -p option to diff.
> 3)  Diffing against something like current trunk (so the line numbers will
>     match up better).  Your diff seems to be against the version of this file
>     from 2005-06-16; there have been 16 revisions since then...  Are you
>     diffing against the 1.8 branch or something?

Thanks for hints. Yes - I'm diffing against 1.8 branch (SeaMonkey-1.1.1). But sorry for inconvenience - I don't have the ability to compile & check the trunk/ on my Windows box (VC6 is not able to do this).

Comment 60

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Yeh, I think with -p the line numbers will matter less. I doubt that this particular part of the file has changed much...

Ask mscott to review?

Comment 61

[Andriy Tkachuk]

Attachment: proposed patch5

Here is the patch with strings copy improvement. It was made against trunk.

Comment 62

[Mike Cowperthwaite]

I can't take the time right now to delve into the details of this proposed patch, but I am still very worried.  Andriy's patch at the related bug 215068 appeared to be doing exactly the wrong thing -- rather than fix the DOM->text problem, he suggests simply removing the space-unstuffing code in the f=f viewer.  That is wrong, and it strongly implies that Andriy doesn't understand f=f, which in turn means this patch is suspect.

CC'ing Ben, who might possibly have some idea about this.

Comment 63

[Andriy Tkachuk]

Mike, please, explain how this bug relates to bug 215068? I don't see any relations.

In bug 215068 viewer just performs one space stripping from begin of line in f=f text for some reason. If you will look on message sources - they looks good.

Here we have the case when not all spaces are stripped at end of paragraphs, and as result wrong f=f text is made.

Comment 64

[Andriy Tkachuk]

Mike, you were right - my fix to bug 215068 was wrong. I wasn't aware of space-stuffing/unstuffing technique in f=f. However this doesn't mean that fix to this bug is wrong - it is different bug.

Comment 65

[Mike Cowperthwaite]

(In reply to comment #64)
> Mike, you were right - my fix to bug 215068 was wrong. I wasn't aware of
> space-stuffing/unstuffing technique in f=f. However this doesn't mean that
> fix to this bug is wrong - it is different bug.

Andriy, sorry to leave this unaddressed for all of last week, I just haven't had the time to look into this.

Looking at this patch and the ones for bug 215068 and bug 261467, you may well be right that these are different bugs.  I think you've hit upon something with the substitution of 'nbsp'.


What exactly is being accomplished here?

+        if (mFlags & nsIDocumentEncoder::OutputFormatFlowed)
+          stringpart.Trim(" \t\r\n", PR_FALSE, PR_TRUE, PR_TRUE);

I'm assuming 'Trim' removes any character from the argument string -- so, why remove Tab, CR, LF?  I suspect the similar code in the patch for bug 261467 is the better approach.


Also, if you haven't already, take a look at bug 265803.  That bug's symptom resonates with this comment from nsPlainTextSerializer::Write():
 // We have two major codepaths here. One that does preformatted text and one
 // that does normal formatted text. The one for preformatted text calls
 // Output directly while the other code path goes through AddToLine.

Comment 66

[Andriy Tkachuk]

Attachment: patch5.1

(In reply to comment #65)
> What exactly is being accomplished here?
> 
> +        if (mFlags & nsIDocumentEncoder::OutputFormatFlowed)
> +          stringpart.Trim(" \t\r\n", PR_FALSE, PR_TRUE, PR_TRUE);

The same bug but for Preformatted text. But you right, only space in the set will be enough.

Comment 67

[Scott MacGregor]

Comment on attachment 265489 [details] [diff] [review]
patch5.1

this looks good to me but needs moa.

Comment 68

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265489 [details] [diff] [review]
patch5.1

sr=bzbarsky.

Comment 69

[Reed Loden [:reed]]

Comment on attachment 265489 [details] [diff] [review]
patch5.1

bug 215068 required approval1.9+ for changes to this same file, so I would think this patch needs it, too...

Comment 70

[Ben Bucksch (:BenB)]

r=BenB

Comment 71

[Boris Zbarsky [:bzbarsky]]

Reed, this patch doesn't affect Firefox.... and more importantly, this component is not getting approvals done at all.  So if you block on the approval, this patch will never land (unless you escalate it to the 1.9 drivers).

Comment 72

[Reed Loden [:reed]]

Checking in content/base/src/nsPlainTextSerializer.cpp;
/cvsroot/mozilla/content/base/src/nsPlainTextSerializer.cpp,v  <--  nsPlainTextSerializer.cpp
new revision: 1.126; previous revision: 1.125
done

Comment 73

[Kai Engert (:KaiE:)]

I think your checkin introduced a crash, please see bug 398547

Comment 74

[Frederic Bezies]

I think this patch is guilty for bug 398729 : cannot compose message in text mode, and only HTML mode is ok :(

Comment 75

[Andriy Tkachuk]

Frederic, you are right, but this was fixed in bug 398547. Sorry for inconvenience.

Comment 76

[Mike Cowperthwaite]

Verified fixed in TB 3a1p-1013, Win2K.  Thank you so much, Andriy & Reed!

Comment 77

[Mike Cowperthwaite]

Whoops... really verifying this time.

Comment 78

[Andriy Tkachuk]

(In reply to comment #66)
> Created an attachment (id=265489) [details]
> patch5.1
>
> (In reply to comment #65)
> > What exactly is being accomplished here?
> >
> > +        if (mFlags & nsIDocumentEncoder::OutputFormatFlowed)
> > +          stringpart.Trim(" \t\r\n", PR_FALSE, PR_TRUE, PR_TRUE);
>
> The same bug but for Preformatted text. But you right, only space in the set
> will be enough.
>

Sorry guys - this one is buggy change - it does not take into account "-- " case that brakes signature delimiter convention (RFC 2646, 4.3).

There is following code in nsPlainTextSerializer::EndLine() that intentionally ignores Preformatted text:

  // In non-preformatted mode, remove SPACE from the end
  // of the line, unless we got "-- " in a format=flowed
  // output. "-- " is the sig delimiter by convention and
  // shouldn't be touched even in format=flowed
  // (see RFC 2646). We only check for "-- " when it's a hard line
  // break for obvious reasons.
  if(!(mFlags & nsIDocumentEncoder::OutputPreformatted) &&
     (aSoftlinebreak || !mCurrentLine.EqualsLiteral("-- "))) {
    // Remove SPACE:s from the end of the line.
    while(currentlinelength > 0 &&
          mCurrentLine[currentlinelength-1] == ' ') {
      --currentlinelength;
    }
    mCurrentLine.SetLength(currentlinelength);
  }

Does anybody knows why?

Comment 79

[Boris Zbarsky [:bzbarsky]]

Uh... so why is this bug "verified incomplete"?  If it needs to be reopened, reopen it.  If a followup bug needs to be filed, file it.

As for the question in comment 78, I suspect you now know this code better than anyone else active with the project does.  That said, it would seem odd to me to remove trailing space in preformatted mode.  Preformatted means keep all whitespace, no?

Comment 80

[Mike Cowperthwaite]

(In reply to comment #79)
> it would seem odd to me to remove trailing space in preformatted mode. 
> Preformatted means keep all whitespace, no?

That reasoning may be why the preformatted exception is in the current code.  The problem with this approach is that format=flowed (which is mail-specific, to my knowledge) uses trailing spaces as key parts of the encoding.  If the EOL spaces are left in during the DOM->f=f conversion, the f=f viewer will reflow those lines together, which isn't generally the desired result; it's typically more important to preserve the line breaks than the trailing spaces.

Comment 81

[Boris Zbarsky [:bzbarsky]]

So this check should be modified to check the format-flowed flag, right?

Comment 82

[Ben Bucksch (:BenB)]

Reopening per above comments. Breaking "-- " in either f=f or normal plaintext is not acceptable. Please back the change out, if it does that, thanks.

Comment 83

[Andriy Tkachuk]

Attachment: patch5.2

Further investigation revealed that EndLine() is not called from preformatted text codepath of Write() anyway, so we have to fix this at hand somehow. I propose just to add the check for "-- " like this:

-        nsAutoString stringpart(Substring(aString, bol, newline-bol));
+        nsAutoString stringpart(Substring(str, bol, newline-bol));
+        if ((mFlags & nsIDocumentEncoder::OutputFormatFlowed) &&
+            !stringpart.EqualsLiteral("-- ")) 
+          stringpart.Trim(" ", PR_FALSE, PR_TRUE, PR_TRUE);

(This patch also includes bug 398547 fix.)

Comment 84

[Boris Zbarsky [:bzbarsky]]

> (This patch also includes bug 398547 fix.)

Which makes it effectively unreviewable.  Please post a patch for just the problem you're fixing.

Comment 85

[Andriy Tkachuk]

Attachment: fix to patch5.1 (from patch5.2)

Here it is.

Comment 86

[Andriy Tkachuk]

The patch to this bug is depend on the patch to bug 215068. There I provided the proper patch to this bug as well.

Comment 87

[Andriy Tkachuk]

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 outputLineBreak case fixes this bug.

Comment 89

[Magnus Melin [:mkmelin]]

(In reply to comment #86)
Andriy: So this should be fixed now that bug 215068 is fixed?

Comment 90

[Ben Bucksch (:BenB)]

Using Thunderbird 2.0.0.12 (20080213), I still saw this yesterday.

Comment 91

[Ben Bucksch (:BenB)]

hm, bug 215068 has been checked in 2008-01-05, but I don't think on the 2.0 branch, so nevermind.

Comment 92

[Andriy Tkachuk]

(In reply to comment #89)
> (In reply to comment #86)
> Andriy: So this should be fixed now that bug 215068 is fixed?
> 

exactly

Comment 93

[Magnus Melin [:mkmelin]]

Let's mark it FIXED again then. Thanks for fixing it!

Comment 94

[Boris Zbarsky [:bzbarsky]]

This needs a unit test...

Comment 95

[Jonathan]

I'm still seeing this problem on version 2.0.0.19 (20090105)
OS: Linux kubuntu 2.6.27-9-generic #1 SMP Thu Nov 20 21:57:00 UTC 2008 i686 GNU/Linux

If the fix is just on the 3.x branch, any chance this will be backported?

Comment 96

[Boris Zbarsky [:bzbarsky]]

Not really, no.  Only security fixes, major compatibility issues, or regression fixes for issues the security fixes cause get backported to stable branches.

Comment 97

[Wayne Mery (:wsmwk)]

This FIXED bug is flagged with in‑testsuite?   It would be great if assignee or someone else can clear the flag if a test is not appropriate.  And if appropriate, create a test and plus the flag to finish off the bug.