Empty lines under quotes are doubled

RESOLVED FIXED in Future

Status

defect
--
major
RESOLVED FIXED
17 years ago
11 years ago

People

(Reporter: 3.14, Assigned: mrbkap)

Tracking

(Blocks 4 bugs)

Trunk
Future
x86
All
Dependency tree / graph
Bug Flags:
blocking1.7a -
blocking1.7b -
blocking1.7 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P0] [Testcase: see comment 56])

Attachments

(2 attachments, 12 obsolete attachments)

1.43 KB, text/html
Details
13.08 KB, patch
akkzilla
: review+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
This bug is a bit confusing. Sorry, but I only observe it from others and try to
find out what happens by asking them.

Assume you have a message looking like this:

---cut---
> bla bla bla

blubb
---cut---

In the reply/follow-up, it becomes:

---cut---
>> bla bla bla
> 
> 
> blubb
---cut---

This bug was observed in various Windows versions. I don't know if Linux, Mac
and others do it.

The problem only occurs for senders who use format flawed.

It only occurs for senders who use the plain text editor.

It happens in reaction to messages with and without f=f.

It happens in reaction to messages with Content-Transfer-Encoding 8bit and
quoted-printable.

Only empty lines directly under quotes are affected.

It also happens with paste as quotation.

pi
Whiteboard: [Hixie-P0]

Comment 1

17 years ago
I got the same problems here.
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.1) Gecko/20020826

Comment 2

17 years ago
When viewing some newsgroup messages, an empty line is added below quoted
material in the Mail/News display pane, but not in the Message Source window. If
saved to files, the extra line will be in the .txt file, but not the .eml file.
The extra line will break PGP signatures. I have a test case in a zip file ready
for anyone who's curious, but given the frequency with which I encounter
improperly rendered signed messages on alt.security.pgp, no one should have any
problems reproducing it.

This is not just a problem with Composition, but affects display of messages
from other people using other newsreaders, too. I suppose the component for this
bug might be changed, but I'm not sure to what.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1) Gecko/20020826

Comment 3

17 years ago
Joe--is this a bug for you (perhaps already fixed) or for Tanu or neither?
No, it still occurs with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.2b) Gecko/20021002

Comment 5

17 years ago
Reproduced with 2002111704-trunk/Linux.
Severity: normal → critical
OS: Windows 2000 → All

Comment 6

17 years ago
Brade: I *think* this is a serializer issue.  I beleive that no one is seeing
these blank lines until they save or send the message.

Comment 7

17 years ago
This is very simple to reproduce.  Just take the test case in the bug description and copy 
it to the clipboard. The copy in the clipboard is the mutilated copy.
(Reporter)

Updated

17 years ago
Keywords: nsbeta1
(Reporter)

Updated

17 years ago
Blocks: 108194
(Reporter)

Comment 8

17 years ago
This bug is highly visible and sheds a bad light on Mozilla.

"I beleive that no one is seeing these blank lines until they save or send the
message."

Is this true?

pi
not really, tested it again with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.3b) Gecko/20030125. Also i wanted to show another example of this bug, i
saw that Mozilla also doubles the line when copying to clipboard, example:

Message looks like this:
------------
Am 29.01.2003 02:00 tippte *Aljoscha Fleischer* in die Kiste:

> Hat jemand einen Tip?

im Profilverzeichnis die XUL.MFL loeschen. Das ist quasi ein
Allheilmittel 

------------

becomes when replying:
------------
Frank Widmaier wrote:
> Am 29.01.2003 02:00 tippte *Aljoscha Fleischer* in die Kiste:
> 
> 
>>Hat jemand einen Tip?
> 
> 
> im Profilverzeichnis die XUL.MFL loeschen. Das ist quasi ein
> Allheilmittel :-)
> 
------------
(and becomes when copy&paste the news article
------------
Am 29.01.2003 02:00 tippte *Aljoscha Fleischer* in die Kiste:


>> Hat jemand einen Tip?


im Profilverzeichnis die XUL.MFL loeschen. Das ist quasi ein
Allheilmittel 
------------)

Comment 10

17 years ago
Mail triage team: nsbeta1-
Keywords: nsbeta1-

Updated

17 years ago
Keywords: nsbeta1

Comment 11

16 years ago
Posted patch patch v1 (obsolete) — Splinter Review
This patch add some changes to nsPlainTextSerializer.
This changes work only for blockquote with type="cite".

Updated

16 years ago
Attachment #114316 - Flags: review?

Updated

16 years ago
Attachment #114316 - Flags: review? → review?(t_mutreja)

Comment 12

16 years ago
I added printf() in ConvertBufToPlainText() and saw how
the messages were in HTML format.

(1) message in ISO-8859-1
************************************
On 02/22/2003 06:29 AM, Koike Kazuhiko wrote:
<br><blockquote type=cite>abc
<br></blockquote>def
<br></body>
</html>
</html>
************************************

(2) message in ISO-2022-JP
************************************
<pre wrap>On 02/22/2003 06:29 AM, Koike Kazuhiko wrote:
</pre><blockquote type=cite><pre wrap>abc
</pre></blockquote><pre wrap><!---->def

</pre></body>
</html>
</html>
************************************

kamio-san's patch doesn't work for (2).

Comment 13

16 years ago
Posted patch patch v1.1 (obsolete) — Splinter Review
(Reporter)

Comment 14

16 years ago
This bug really makes Mozilla users look the the dumbest luser. It is outright
embarrassing. We need to have this fixed.

pi
Flags: blocking1.4b?
(Reporter)

Updated

16 years ago
Attachment #115589 - Flags: superreview?(esther)
Attachment #115589 - Flags: review?(ducarroz)
That's nice, but esther is not a super-reviewer.  See
http://mozilla.org/hacking/reviewers.html

Comment 16

16 years ago
Cc akkana for comment on patch v1.1.
(Reporter)

Comment 17

16 years ago
Comment on attachment 115589 [details] [diff] [review]
patch v1.1

Thanks, Boris. Hope we get this fixed now.

pi
Attachment #115589 - Flags: superreview?(esther) → superreview?(bienvenu)
Attachment #115589 - Flags: review?(ducarroz) → review?(ssu)
(Reporter)

Updated

16 years ago
Blocks: 168420

Comment 18

16 years ago
If I run the test through the output tests (TestOutSinks.pl in dist/bin, run
automatically from tinderbox), I get two conflicts (which means this would turn
the tree red): one in mailquote and one in simplefmt.

The mailquote difference is that the close of a blockquote no longer creates its
own blank line like most block-level elements do.  That's only the case for cite
blockquotes, right?  Normal blockquotes will still get the usual treatment?  If
that's true, that should be okay, but we need to update mailquote.out by
deleting the blank line after "rogue and peasant slave" so T'box doesn't go red.  

(Looking at the before and after, though, it does make me wonder whether this is
the right fix, and whether we might not be better off fixing mail not to add the
additional blank line.  But you've looked at it in more detail than I have, so I
won't argue the point if you're convinced this is the right place to fix it.)

The other difference worries me, though: it changed
Here is a blockquote:

    The quick brown fox jumped over the lazy dog 
to
    Here is a blockquote:

    The quick brown fox jumped over the lazy dog 

in other words, the line preceding the blockquote, which is not part of the
blockquote, is now indented.  This is a normal blockquote, not a cited one; the
source, in simple.html, is this:
Here is a blockquote:
<blockquote>
The quick brown fox

Is there some way to tweak the patch so that only cited blockquotes are
affected, and not normal blockquotes?  I would love to see this bug fixed, but
it's not worth fixing quotations at the price of screwing up conversion of
non-mail html.

<evangelism>
Please do run the output tests when making a serializer change like this. 
They're perl and should work on any platform that can build mozilla, and they
guard against introducing regressions.  

Also please consider adding a test case when you fix bugs or add a new feature
(in this case, you could probably add a few new lines to mailquote.html and
mailquote.out) because that guards against someone else regressing your fix in
the future.  That's especially true for things like plaintext mail quotes which
aren't tested as often as they should be, so they're at high risk of regression.
 The tests live in htmlparser/tests/outsinks (for legacy reasons -- the
serializers used to live in htmlparser before they moved to content).
</evangelism>

Updated

16 years ago
Flags: blocking1.4b? → blocking1.4b-
Attachment #114316 - Flags: review?(t_mutreja)

Comment 19

16 years ago
Posted patch patch v2 (obsolete) — Splinter Review
I believe this patch affects cited blockquotes. But TestOutSinks.pl
failed about mailquote.out. Could somebody help me?
Attachment #114316 - Attachment is obsolete: true
Attachment #115589 - Attachment is obsolete: true

Comment 20

16 years ago
I've been wanting to write up a howto on those tests, how to add new ones or
modify existing ones, but haven't had time yet.  But for now:

TestOutSinks.pl is a script, so you can read it to get the exact command for the
particular test that's failing.  (TestOutSinks.pl will print the name of the
test that's failing (a command starting with ./TestOutput), and you can match
that in the script.)

Then run that command by hand, but omit the "-o OutTestDate/filename" part to
see the actual output.  Perhaps capture it in a file to make it easier to debug.

The script tells you how many characters in the error was (sorry, it's not smart
about lines) so with emacs I go to the beginning of the captured output and do
ctrl-U <number of chars> ctrl-f, and it puts me approximately where the error
happened.

Then compare the file at that point with the comparison file (the one that was
specified with -o) and you'll see what the change is.

If the change is actually correct behavior (it's behaving better now than
before), then just modify the comparison file (they live in
htmlparser/tests/outsinks/*.out and get installed into dist/bin/OutTestData) and
check that in along with the patch (be sure the reviewer knows about that part
of the change).  If it's an error and the old behavior was better, then you've
caught a bug with the patch which should be fixed before the patch is checked in.

Hope this helps!  I really will try to write up something longer and check it
into the mozilla doc tree soon.

Comment 21

16 years ago
Thank you, Akkana. Now I see what the problem is.

I modified mailquote.html to see the problem.

Comment 22

16 years ago
TestOutput converts attachment 123346 [details] as the following.
--------------------

  normal blockquote

    /(The next line does not end with a *br* tag.)./
    Oh, what a rogue and peasant slave am I. 

/(Neither does the next line:)/
The observed of all observers, quite, quite down!


  cited blockquote

> /(The next line does not end with a *br* tag.)./
> Oh, what a rogue and peasant slave am I. 
/(Neither does the next line:)/
The observed of all observers, quite, quite down!
--------------------

Without my patch, a blank line is inserted before "Neither does..."
in the cited blockquote. Which is right? I believe my patch is.

Comment 23

16 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Kamio-san pointed out patch v2 affects all of <pre>.
Attachment #123252 - Attachment is obsolete: true

Comment 24

16 years ago
Posted patch patch v4 (obsolete) — Splinter Review
patch v3 doesn't work well.
Attachment #123373 - Attachment is obsolete: true

Comment 25

16 years ago
patch v4 doesn't work if mailnews.display.disable_format_flowed_support
is true.
Attachment #123462 - Attachment is obsolete: true

Updated

16 years ago
Attachment #123737 - Flags: review?(ducarroz)
Comment on attachment 123737 [details] [diff] [review]
(Av5) <nsPlainTextSerializer.*>

over to ssu for review...
Attachment #123737 - Flags: review?(ducarroz) → review?(ssu)
Comment on attachment 115589 [details] [diff] [review]
patch v1.1

Clearing review requests on this obsolete patch.
Attachment #115589 - Flags: superreview?(bienvenu)
Attachment #115589 - Flags: superreview?
Attachment #115589 - Flags: review?(ssu)
Attachment #115589 - Flags: review?
Attachment #123737 - Flags: superreview?(bienvenu)
Attachment #123737 - Flags: review?(ssu)
Attachment #123737 - Flags: review?(scott)
This patch doesn't work for me (i was the orginal reporter, Boris asked me in a
newsgroup about this bug; hadn't a bugzilla account at that time).
Attachment #115589 - Flags: superreview?
Attachment #115589 - Flags: review?

Comment 29

16 years ago
Is this critical for any reason?

Comment 30

16 years ago
Aside from the many CCs and votes, this bug causes seemingly random failures when using PGP's 
current window feature on cleartext digital signatures because Mozilla gives back the corrupt copy 
rather than the correct copy. Basically, any message with a quoted section will fail to verify because 
of this bug.
(Reporter)

Comment 31

16 years ago
This patch is really old. How about it? Frank says it does not work (probably
bitrotted). How about the reviews?

pi

Comment 32

16 years ago
Unless someone claims it works, I don't see how reviews would be useful :-(
Attachment #123737 - Flags: superreview?(bienvenu)
Attachment #123737 - Flags: review?(mscott)
(Reporter)

Comment 33

16 years ago
Very emberassing for Mozilla users. So this should really be solved. Asking for
blocking.

pi
Flags: blocking1.7a?
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6) Gecko/20040113] (W98SE)

(Only to "update" the "test" release, from v1.3b to v1.6)
Bug still there.
Comment on attachment 123737 [details] [diff] [review]
(Av5) <nsPlainTextSerializer.*>


(for the record)
This patch has not "bitrotted" yet:
{
R:\!Drive_X\Mozilla\Cvs\Checkout\mozilla>patch -p0 < 2830.diff
patching file content/base/src/nsPlainTextSerializer.cpp
Hunk #1 succeeded at 106 (offset -1 lines).
Hunk #2 succeeded at 628 (offset 7 lines).
Hunk #3 succeeded at 711 (offset 7 lines).
Hunk #4 succeeded at 817 (offset 7 lines).
Hunk #5 succeeded at 998 (offset 15 lines).
Hunk #6 succeeded at 1045 (offset 15 lines).
patching file content/base/src/nsPlainTextSerializer.h
}

However I could post an updated patch against the current CVS versions, if
asked for.
Attachment #123737 - Attachment description: patch v5 → (Av5) <nsPlainTextSerializer.*>

Comment 36

15 years ago
yes, serge. sounds useful to get an updated patch against the trunk, and then
request reviews...   to late to try and get this in for 1.7a, but we should
shoot for some good testing and land early in 1.7b
Flags: blocking1.7b?
Flags: blocking1.7a?
Flags: blocking1.7a-
Attachment #123737 - Attachment is obsolete: true
Av5, with comment 36 suggestion(s):

.cpp: v1.84 -> v1.92; .h: 1.33 -> 1.37; identical patch code.
Comment on attachment 141363 [details] [diff] [review]
(Av5b) <nsPlainTextSerializer.*>


'r=?':
There seems to be a doubt on whether this patch works or not !? Take it easy...
Attachment #141363 - Flags: review?(bzbarsky)
Comment on attachment 141363 [details] [diff] [review]
(Av5b) <nsPlainTextSerializer.*>

I really don't have time to review patches that haven't even been tested. 
Please make sure this is tested and works before asking for reviews.
Attachment #141363 - Flags: review?(bzbarsky) → review-
Comment on attachment 141363 [details] [diff] [review]
(Av5b) <nsPlainTextSerializer.*>


Allow me to mark it again as "not yet reviewed" rather than 'r-';

Then, could someone (I can't) test and comment on this patch ?

Thanks.
Attachment #141363 - Flags: review-
Actually this patch seems to work :) (i wonder why, because i tested this patch
some time ago and there it didn't)
Comment on attachment 141363 [details] [diff] [review]
(Av5b) <nsPlainTextSerializer.*>

Trying 'r?' again:
after successful test in comment 41...
Attachment #141363 - Flags: review?(bzbarsky)

Comment 43

15 years ago
Does this new patch leave the output tests unchanged?  If not, then it needs to
include the necessary changes to the test samples, or else the tree will go red.

In response to comment 22, I think Koike is probably right and the new behavior
is better, so if that causes a test failure, the test output (mailquote.out?)
should be changed to match the new output.

Are normal blockquotes (and the line preceeding them) now unaffected (ref. "Here
is a blockquote" that I mentioned in comment 18)?
So what does this patch actually do?  I don't really know this code (so I'm a
little confused about why you're asking me for review), so if you want me to
review the patch someone clearly needs to spell out what it does (and it would
still need an sr from someone who _does_ know this code).

Comment 45

15 years ago
It might also be helpful to see an explanation of why this is a serializer bug
and not a mailnews bug (i.e. why the fix is in the serializer and not in the
mailnews code that calls the serializer).  I'm not saying the fix doesn't belong
there; I just think an explanation would be good before adding more special-case
code to the serializer.
And apart from all that, the patch should have a nice long comment where the
boolean member is declared explaining what it does.
Boris:
I asked you for review, as a |content/base| peer, nothing more...

My only purpose was to get some attention to this bug/patch ... which I have :->

Koike:
Are you still around to answer all the comments about your patch ?
Serge, in general it makes sense to look at who has reviewed previous changes to
the file when asking for review.  There are a lot of things in content/base, and
not every peer knows them all equally well.
Comment on attachment 141363 [details] [diff] [review]
(Av5b) <nsPlainTextSerializer.*>

r- per my comments.  This patch needs some documentation of what the heck is
going on.

I won't be able to do any reviews for the foreseeable future, by the way.
Attachment #141363 - Flags: review?(bzbarsky) → review-

Comment 50

15 years ago
Greting, all.

I don't have the wherewithall to contribute to the resolution of the situation
here with any expertise.  I have no real experience with programming, and use
only the pre-compiled builds.

I do, however, use this product (TBird, sp.) on a daily basis, and find this
problem(s?) very frustrating--so frustrating that I'm willing to assist
financially:  can I pay someone to resolve this?  What would it take were I to
do so?

Also, in the meantime, if there is any other way I can help (testing, etc.),
please let me know.

Thanks for your work!
Well, someone just needs to adress the comments from Boris and find someone
reviewing the patch.

Comment 52

15 years ago
I think fixing the serializer is much better than changing the mailnews code in
the ways that have been proposed in an other bug - the proposed changes to
mailnews code changed really fundamental things about the way mailboxes and
messages are parsed. Fixing the serializer is a lot less scary.

I'll go add a comment to this patch explaining what's going on. 

Comment 53

15 years ago
I applied this patch, but I wasn't able to see any difference in the quoting
behaviour. But I wasn't able to reproduce the bug as described either...

I set my identity to use plain text compose, and tried both the > bla bla blubb
test and the German one, and I didn't see an extra line in the middle of the
quote when I replied. I did see two extra blank lines at the end of the message,
however, with and w/o this patch.
(In reply to comment #53)
> I did see two extra blank lines at the end of the message,
> however, with and w/o this patch.

This must be bug 70728 !?
(for which "we" are expecting a "solution" from you too... ;->)

Comment 55

15 years ago
yes, thx, that's the related bug I mentioned earlier. It's also a serializer
bug, and should be fixed in the serializer. But I still need to know why I can't
recreate this bug, because if I can't recreate it, I can't test the fix. I could
try to fix the serializer not to bug 70728, but I suspect the fixes would collide.
Target Milestone: --- → Future
(In reply to comment #53)
> I wasn't able to reproduce the bug as described either...

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219] (W98SE)

Try this:

1. Compose a new message
2. Cut & Paste the 3 "> bla ... blubb" lines directly into the body
3. Quit and Save as Draft

(3a. Try to Reply to it: NO doubled lines :-) Abort.)

4. Edit [it] as New
5. Type a letter in the subject (Anything which will trigger the Save feature at
6 will do)
6. Quit and Save as Draft

6a. Try to Reply to it: DOUBLED lines :-(

I'll let you figure out what is going on...

Comment 57

15 years ago
yes, that causes problems, but I see the problems when i run the the Av5b patch.
So, as near as I can tell, we have a couple reproducible problems, but the patch
doesn't fix them. Maybe things are worse without the patch, but I can't tell if
that's true or not.
Av5b, with a few simpler rewrites.

No functional change: only for the record/future.
Attachment #141363 - Attachment is obsolete: true
(In reply to comment #41)
> Actually this patch seems to work :) (i wonder why, because i tested this patch
> some time ago and there it didn't)

frank:
Can you try "this" patch again, comment ...

(In reply to comment #57)
> but the patch doesn't fix them.

... and eventually get in touch with david.

(I "revived" this patch & bug, but I can't help on explaning & testing :-()
Whiteboard: [Hixie-P0] → [Hixie-P0] [Testcase: see comment 56]

Comment 60

15 years ago
1.7b is shipping...  will check back on this for 1.7final
Flags: blocking1.7b?
Flags: blocking1.7b-
Flags: blocking1.7?

Updated

15 years ago
Flags: blocking1.4b-
not going to block the release for this. if you get a fully reviewed patch
before we ship, please request approval to land on the branch.
Flags: blocking1.7? → blocking1.7-

Comment 62

15 years ago
Please do something with this almost 2-years-old bug. Deleting extra newlines
afer quoted text in each reply is driving me crazy.
*** Bug 238496 has been marked as a duplicate of this bug. ***
*** Bug 263729 has been marked as a duplicate of this bug. ***
This bug does not qualify as critical.
  https://bugzilla.mozilla.org/bug_status.html#severity
Severity: critical → major
Product: MailNews → Core

Comment 66

15 years ago
PGP 8.1 RLS notes refer to this bug as causing failure of signature verification:

"Mozilla does not correctly implement standard Edit --> Copy command. Spurious
extra lines are inserted after quoted text blocks when copying text from a
Mozilla message window. PGP's Use Current Window feature can report failure on
signature verifications of signed messages containing quoted text in Mozilla due
to this. This is Mozilla bug #144998 opened over 2 years ago."

Is Thunderbird free of this bug?

Updated

15 years ago
Blocks: 109935

Updated

15 years ago
No longer blocks: 109935
Keywords: 4xp

Updated

15 years ago
Blocks: 109935

Comment 67

15 years ago
(In reply to comment #22) 
 
> Is Thunderbird free of this bug? 
 
No. Thunderbird version 0.9 (20041104) suffers from the same embarassing 
bug :-( 
Posted patch patch against current trunk (obsolete) — Splinter Review
This is the ported patch for current trunk which work at least partly.
My main problem of inserting extra lines under cited blockquotes is fixed with
this patch.
Attachment #167622 - Flags: superreview?
Attachment #167622 - Flags: review?(bienvenu)

Comment 69

15 years ago
Thunderbird 1.0 in Windows XP:-
I tried the test Serge provides in comment #56.
The bug is still there.  :-(
BTW: There is a new workaround which has NOT to do with disabling format=flowed.
Set mail.quoteasblock to false and then the empty lines won't be doubled.
This pref should only be for the HTML editor (it can quote with the html tag
<blockquote>), but somehow it has a effect on the plaintext editor, too (the
said doubled empty lines).
Comment on attachment 167622 [details] [diff] [review]
patch against current trunk

Any patch for the serializer should go through Akkana or Daniel Bratell
(bratell lysator.liu.se)

FYI, setting quoteasblock to false means that the HTML editor will show nested
quotes (originating from plaintext mail) with ugly >s, so it's pretty bad for
users of the HTML editor.
Attachment #167622 - Flags: review?(bienvenu) → review?
Blake, you looked at the serializer recently. Would you be able to take a look
at this patch?
For what it's worth, I reran the output tests, and the only change is the change
described in comment #22 that Akkana agrees is an improvement. I'll attach an
updated patch that fixes the testcase.
Posted patch updated patch (obsolete) — Splinter Review
as described
Attachment #167622 - Attachment is obsolete: true
Attachment #173142 - Flags: superreview?(roc)
Attachment #173142 - Flags: review?(akkzilla)

Comment 75

14 years ago
I can look at the code.  Thanks, roc, for the fix to the tests.

Has anyone besides the patch author tested this?  I can't test it myself at the
moment (issues with mail setup), and "work at least partly" makes me wonder. 
Ben, roc, David, have any of you tried it?
I use this patch since some time and for me it works. But I didn't test all side
effects. I use only the plaintext editor with mailnews. It would be good if
others would test it as well.
Plain Text Editor (for all posts, format flowed on): No doubled empty lines
anymore, BUT when creating the third reply to the original post (so when the 3rd
quoting level is introduced) one empty line under that 3rd quoting level is
deleted. For every additional quote another (if it applys) empty line is
deleted, so for example another empty line under the 4th quote level and a empty
line under the 3rd quote level is deleted
Plain Text Editor (for all posts, format flowed off): Same as with with f=f on

HTML Text Editor, sending as plain text (for all posts, format flowed on): Patch
deletes the empty line after "xyz wrote:" and the empty line after the quote
(this is wrong behaviour), empty lines though visible in Compose window
HTML Text Editor, sending as plain text (for all posts, format flowed off):
Patch deletes the empty line after "xyz wrote:", the empty line after the quote
gets a quote sign (">  ") with two spaces behind it.

Here i stopped testing, i already found enough bugs ;).
Comment on attachment 173142 [details] [diff] [review]
updated patch

Okay, sounds like we can't cargo-cult this patch. Anyone who actually knows the
serializer going to work on this?
Attachment #173142 - Flags: superreview?(roc)
Attachment #173142 - Flags: superreview-
Attachment #173142 - Flags: review?(akkzilla)
Attachment #173142 - Flags: review-
*** Bug 281631 has been marked as a duplicate of this bug. ***
I don't know the serializer very well, but I'll take this bug and try to fix it.
If someone wants to take this from me, please feel free.
Assignee: ducarroz → mrbkap
I think there is a trivial fix to this problem. Let me check if what I have in
mind works or not.

Comment 82

14 years ago
'tis bloody annoying!

strange thing is, when the excess blanks are removed when a reply is composed
and posted- after 2-3 further follow-ups, TB puts the excess blanks back!

reg 23 April 2005
(In reply to comment #81)
> I think there is a trivial fix to this problem. Let me check if what I have in
> mind works or not.

BTW: This trivial fix Daniel thought of didn't work.
Posted patch some more work (obsolete) — Splinter Review
This patch seems to work for plain text compose, format=flowed ON, but has
problems for format=flowed being OFF. I'm trying to figure out why these
settings have an effect on it. I'm attaching it so I don't lose it.
Attachment #143438 - Attachment is obsolete: true
Attachment #173142 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Posted patch potential patch (obsolete) — Splinter Review
This patch works as far as I can tell. I've updated the outsink tests to cover
the new cases in this bug also.

This patch needs testing! Please tell me of any problem with it.
Attachment #184048 - Attachment is obsolete: true
Comment on attachment 184141 [details] [diff] [review]
potential patch

mcsmurf tells me this patch works for him.

Akkana, can you review?
Attachment #184141 - Flags: review?(akkzilla)

Comment 87

14 years ago
Actually that looks pretty clean. I only made a quick pass through it and
haven't tried applying it (deadlines looming, I should have more time later in
the week) but I had a couple of questions:

I don't understand now mIsInCiteBlockquote and isInCiteBlockquote interrelate,
why they're both needed, and where mIsInCiteBlockquote gets reset.

I'm not familiar with PRPackedBool functions like GetLastBool: I'll take it on
faith that those are doing the right thing.

I don't really understand the "if (!aString.IsEmpty())" clause even with the
comment.
(In reply to comment #87)
> I don't understand now mIsInCiteBlockquote and isInCiteBlockquote interrelate,
> why they're both needed, and where mIsInCiteBlockquote gets reset.

mIsInCiteBlockquote is a stack of booleans that notes whether or not the last
blockquote open was a <blockquote type="cite"> or not. Note that
isInCiteBlockquote is in DoOpenContainer() and is inspecting the current
(opening) blockquote (so that the information hasn't been pushed onto
mIsInCiteBlockquote yet, and isInCiteBlockquote is used for that purpose later).

> I'm not familiar with PRPackedBool functions like GetLastBool: I'll take it on
> faith that those are doing the right thing.

These are actually functions defined by nsPlaintextSerializer, they allow an
nsAutoVoidArray to act like an array of booleans.

> 
> I don't really understand the "if (!aString.IsEmpty())" clause even with the
> comment.

I've updated the comment to:
      // XXX OutputQuotesAndIndent expects the line to be outputted in
      // mCurrentLine, so if it's empty, it refuses to output a trailing space.
      // This hack makes sure that it outputs '> ' (instead of just '>').

In other words, this makes the current line look like it isn't empty, so that
OutputQuotesAndIndent doesn't think it is. Actually, now that I look at this,
there may be a problem with empty lines in <pre> with this change (we'd output
'> ' which would be a wrapped line in f=f). I'll attach a new patch which fixes
this.
Posted patch updated patchSplinter Review
This uses mCurrentLine and avoids the issue altogether.
Attachment #184141 - Attachment is obsolete: true
Attachment #184332 - Flags: review?(akkzilla)
Attachment #184141 - Flags: review?(akkzilla)

Comment 90

14 years ago
Comment on attachment 184332 [details] [diff] [review]
updated patch

Okay, I think I grok it now, and it looks okay. I'm not sure how much you've
tested this latest patch: do try to come up with some edge cases, but I'm
marking it reviewed.
Attachment #184332 - Flags: review?(akkzilla) → review+
Comment on attachment 184332 [details] [diff] [review]
updated patch

Note that the only edge case that I believe can cause a "problem" is if someone
uses a <blockquote type=cite> followed by a <pre> later in the page. I think
there's a simple fix (which I'll check shortly), but I'd rather tackle that in
another bug.

jst, can you sr?
Attachment #184332 - Flags: superreview?(jst)
Comment on attachment 184332 [details] [diff] [review]
updated patch

sr=jst
Attachment #184332 - Flags: superreview?(jst) → superreview+
Comment on attachment 184332 [details] [diff] [review]
updated patch

The more testing this gets, the better. Also, if Thunderbird wants to release
off of 1.8, it probably wants this patch.
Attachment #184332 - Flags: approval1.8b3?
Comment on attachment 184332 [details] [diff] [review]
updated patch

a=shaver
Attachment #184332 - Flags: approval1.8b3? → approval1.8b3+
Depends on: 296284
Fix checked in. I filed bug 296284 on the new issue.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Blocks: 298918
Depends on: 311679
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.