144998 - Empty lines under quotes are doubled

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Boris 'pi' Piwinger]

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

Comment 1

[Raul Pinto]

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

Comment 2

[Chris Applegate]

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

[Kathleen :Brade]

Joe--is this a bug for you (perhaps already fixed) or for Tanu or neither?

Comment 4

[Frank Wein [:mcsmurf]]

No, it still occurs with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.2b) Gecko/20021002

Comment 5

[Koike Kazuhiko]

Reproduced with 2002111704-trunk/Linux.

Comment 6

[Joe Francis]

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

[Will Price]

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.

Comment 8

[Boris 'pi' Piwinger]

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

Comment 9

[Frank Wein [:mcsmurf]]

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

[Samir Gehani]

Mail triage team: nsbeta1-

Comment 11

[Shotaro Kamio]

Attachment: patch v1

This patch add some changes to nsPlainTextSerializer.
This changes work only for blockquote with type="cite".

Comment 12

[Koike Kazuhiko]

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

[Koike Kazuhiko]

Attachment: patch v1.1

Comment 14

[Boris 'pi' Piwinger]

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

pi

Comment 15

[Boris Zbarsky [:bzbarsky]]

That's nice, but esther is not a super-reviewer.  See
http://mozilla.org/hacking/reviewers.html

Comment 16

[kinmoz]

Cc akkana for comment on patch v1.1.

Comment 17

[Boris 'pi' Piwinger]

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

Thanks, Boris. Hope we get this fixed now.

pi

Comment 18

[Akkana Peck]

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>

Comment 19

[Koike Kazuhiko]

Attachment: patch v2

I believe this patch affects cited blockquotes. But TestOutSinks.pl
failed about mailquote.out. Could somebody help me?

Comment 20

[Akkana Peck]

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

[Koike Kazuhiko]

Attachment: modified mailquote.html

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

I modified mailquote.html to see the problem.

Comment 22

[Koike Kazuhiko]

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

[Koike Kazuhiko]

Attachment: patch v3

Kamio-san pointed out patch v2 affects all of <pre>.

Comment 24

[Koike Kazuhiko]

Attachment: patch v4

patch v3 doesn't work well.

Comment 25

[Koike Kazuhiko]

Attachment: (Av5) <nsPlainTextSerializer.*>

patch v4 doesn't work if mailnews.display.disable_format_flowed_support
is true.

Comment 26

[Jean-Francois Ducarroz]

Comment on attachment 123737 [details] [diff] [review]
(Av5) <nsPlainTextSerializer.*>

over to ssu for review...

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Clearing review requests on this obsolete patch.

Comment 28

[Frank Wein [:mcsmurf]]

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).

Comment 29

[Stewart Gordon]

Is this critical for any reason?

Comment 30

[Will Price]

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.

Comment 31

[Boris 'pi' Piwinger]

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

pi

Comment 32

[David :Bienvenu]

Unless someone claims it works, I don't see how reviews would be useful :-(

Comment 33

[Boris 'pi' Piwinger]

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

pi

Comment 34

[Serge Gautherie (:sgautherie)]

[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 35

[Serge Gautherie (:sgautherie)]

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.

Comment 36

[chris hofmann]

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

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Av5b) <nsPlainTextSerializer.*>

Av5, with comment 36 suggestion(s):

.cpp: v1.84 -> v1.92; .h: 1.33 -> 1.37; identical patch code.

Comment 38

[Serge Gautherie (:sgautherie)]

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...

Comment 39

[Boris Zbarsky [: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.

Comment 40

[Serge Gautherie (:sgautherie)]

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.

Comment 41

[Frank Wein [:mcsmurf]]

Actually this patch seems to work :) (i wonder why, because i tested this patch
some time ago and there it didn't)

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 141363 [details] [diff] [review]
(Av5b) <nsPlainTextSerializer.*>

Trying 'r?' again:
after successful test in comment 41...

Comment 43

[Akkana Peck]

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)?

Comment 44

[Boris Zbarsky [:bzbarsky]]

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

[Akkana Peck]

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.

Comment 46

[Boris Zbarsky [:bzbarsky]]

And apart from all that, the patch should have a nice long comment where the
boolean member is declared explaining what it does.

Comment 47

[Serge Gautherie (:sgautherie)]

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 ?

Comment 48

[Boris Zbarsky [:bzbarsky]]

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 49

[Boris Zbarsky [:bzbarsky]]

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.

Comment 50

[Andrew Hurt]

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!

Comment 51

[Frank Wein [:mcsmurf]]

Well, someone just needs to adress the comments from Boris and find someone
reviewing the patch.

Comment 52

[David :Bienvenu]

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

[David :Bienvenu]

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.

Comment 54

[Serge Gautherie (:sgautherie)]

(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

[David :Bienvenu]

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.

Comment 56

[Serge Gautherie (:sgautherie)]

(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

[David :Bienvenu]

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.

Comment 58

[Serge Gautherie (:sgautherie)]

Attachment: (Av5c) <nsPlainTextSerializer.*>

Av5b, with a few simpler rewrites.

No functional change: only for the record/future.

Comment 59

[Serge Gautherie (:sgautherie)]

(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 :-()

Comment 60

[chris hofmann]

1.7b is shipping...  will check back on this for 1.7final

Comment 61

[Asa Dotzler [:asa]]

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.

Comment 62

[Tomasz Piasecki]

Please do something with this almost 2-years-old bug. Deleting extra newlines
afer quoted text in each reply is driving me crazy.

Comment 63

[Mike Cowperthwaite]

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

Comment 64

[Mike Cowperthwaite]

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

Comment 65

[Mike Cowperthwaite]

This bug does not qualify as critical.
  https://bugzilla.mozilla.org/bug_status.html#severity

Comment 66

[Buddy McKee]

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?

Comment 67

[Stefan Seyfried]

(In reply to comment #22) 
 
> Is Thunderbird free of this bug? 
 
No. Thunderbird version 0.9 (20041104) suffers from the same embarassing 
bug :-( 

Comment 68

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch against current trunk

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.

Comment 69

[Philip Pawley]

Thunderbird 1.0 in Windows XP:-
I tried the test Serge provides in comment #56.
The bug is still there.  :-(

Comment 70

[Frank Wein [:mcsmurf]]

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 71

[Ben Bucksch (:BenB)]

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.

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Blake, you looked at the serializer recently. Would you be able to take a look
at this patch?

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

as described

Comment 75

[Akkana Peck]

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?

Comment 76

[Wolfgang Rosenauer [:wolfiR]]

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.

Comment 77

[Frank Wein [:mcsmurf]]

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 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 79

[Mike Cowperthwaite]

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

Comment 80

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 81

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I think there is a trivial fix to this problem. Let me check if what I have in
mind works or not.

Comment 82

[daneel]

'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

Comment 83

[Frank Wein [:mcsmurf]]

(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.

Comment 84

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: some more work

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.

Comment 85

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: potential patch

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.

Comment 86

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 184141 [details] [diff] [review]
potential patch

mcsmurf tells me this patch works for him.

Akkana, can you review?

Comment 87

[Akkana Peck]

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.

Comment 88

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 89

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated patch

This uses mCurrentLine and avoids the issue altogether.

Comment 90

[Akkana Peck]

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.

Comment 91

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 92

[Johnny Stenback (:jst)]

Comment on attachment 184332 [details] [diff] [review]
updated patch

sr=jst

Comment 93

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 94

[Mike Shaver (:shaver emeritus)]

Comment on attachment 184332 [details] [diff] [review]
updated patch

a=shaver

Comment 95

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in. I filed bug 296284 on the new issue.