173953 - can't display classical quotation sign ">" anymore

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Frank Widmaier]

Gecko/20021011 - 04

I defined in user.js

user_pref("mailnews.display.disable_format_flowed_support", true);
user_pref("mail.quoted_graphical", false);
user_pref("mail.quoteasblock", false);

with that it's been possible to show ">" at the beginning of a quotation

now the first quote-sign won't be displayed. I still see the blue line from top
to down. The other ">" will be displayed correctly (lower quoting levels)

Comment 1

[Frank Widmaier]

additional Info:

I'm using the plaintext-editor. But, it seems, that the border between plain and
html-editor are wiped away.

when quoting <ao6q11$jmf7n$[e-mail]> the link won't be quoted
when quoting <[e-mail]> the
HTML-Code will be displayed INTERPRETED as table in my posting. In the original
posting I see plaintext of that table

Comment 2

[John Levon]

bug 161978 ?

Comment 3

[Frank Widmaier]

@John

No - not at all.
The bug you mentioned was reported for 1.1b, not 1.2b (from today) and:
This bug is special in one thing - it's only the first quotation level, that's
shown as blue line. When I remember right, then normally will be shown all
quotation levels with blue lines, when activating quoted.graphical, not only the
first. [1]

When you read my additional text (comment #2) and
http://bugzilla.mozilla.org/show_bug.cgi?id=131536#c19 you will see, that the
actual nightly seems to have a "deeper problem" with the mailnews frontend.

{1]
e.g.

"|" represents the blue line

======================
normal, graphical view
======================
XYZ wrote:
| ABC wrote:
|| 123 wrote:
||| test test test
||| test test test
||
|| ssssss ssssss ssss ssss
|| ttttt ttttt ttt tttt
|
| blafaselblafaselblafaselblafasel

in my opinion.......

==============
classical view
==============
normal, graphical view
======================
XYZ wrote:
> ABC wrote:
>> 123 wrote:
>>> test test test
>>> test test test
>>
>> ssssss ssssss ssss ssss
>> ttttt ttttt ttt tttt
>
> blafaselblafaselblafaselblafasel

in my opinion.......

===============
the "bug-view"
===============
normal, graphical view
======================
XYZ wrote:
| ABC wrote:
| > 123 wrote:
| >> test test test
| >> test test test
| >
| > ssssss ssssss ssss ssss
| > ttttt ttttt ttt tttt
|
| blafaselblafaselblafaselblafasel

in my opinion.......

Comment 4

[Frank Widmaier]

sorry - I copied too much text
in the examples 2 and 3 you have to ignore the "normal, graphical view"

and sorry about the comment number - I counted the wrong way. It was my ccomment
two :o). John's was _number_ two, my number was comment #1

Comment 5

[Boris 'pi' Piwinger]

Seen with Linux with mozilla-source.tar.bz2 10/10/02 14:58:00.

pi

Comment 6

[parish]

CVS build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b)
Gecko/2002101118 on WinXP

Viewing mails/news I have the correct '>' quoting at all levels but in the
composition window when replying I see the blue bar for the first level and '>'
for all deeper levels.

Comment 7

[K Chayka]

win32 trunk build 2002101108, win98se.

In the compose window, Options->Rewrap magically removes the vertical bar and
restores the > for the first quote level.  Strange.  Even stranger, though, is
that this also blows my sig away. :(

Comment 8

[K Chayka]

from comment #3
> nightly seems to have a "deeper problem" with the mailnews frontend

An understatement. ;)  The vertical bar appears to be a symptom of a bigger
problem: the plain-text composer is actually interpretting html instead of plain
text.

1. Using the plain-text composer, send a message to yourself.  Include in the body
<h1 style="color:#f00;background-color:#0f0;">heading</h1> (or any other html).
2. Receive that message.  It looks like the plain text that was sent.
3. Pick Reply.  The vertical bar appears and the embedded code sample is now
rendered as html.
4. Pick Options->Rewrap.  The vertical bar is replaced by ">", the html tags are
stripped out and only "heading" shows in its place.  ish

This makes plain-text composition unusable in many cases.  Suggest upping the
severity of this.

FYI, 100808 is the last good win32 build before the troubles began.  The first
two win32 builds of 10/9 have a completely blank Reply window, the 100910 build
is the first showing the above symptoms. 

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  This is a blocker.  It completely munges outgoing replies; if you're
replying to a plaintext mail that included HTML examples the composition window
will randomly drop some of the examples from the quoted text (eg a lone <tr>)
and interpret others as HTML, making them not usefully quotable.

This is completely preventing the use of Mozilla for following www-*@w3.org
mailing lists, if nothing else.  If we still had a dogfood keyword, this would
definitely be dogfood.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, and this worked in 2002-10-08-08 but is broken in 2002-10-10-14.  2002-10-09
morning builds had a completely horked reply setup (no quoting happened at all)
and I can find no other Linux nightlies between 2002-10-08-08 and 2002-10-10-14.

Comment 12

[Boris Zbarsky [:bzbarsky]]

.

Comment 13

[Frank Widmaier]

@Boris Z

not only HTML-examples. I lost a link (it's been a bugzilla-link ;-)   ) when
replying to a posting. I thought mozilla tried to create a complete <a
href..>-link of that link. Because there was no "text", it created something
like that: <a href..></a> - with the consequence that you "loose" the link
because it embraces no characters that could be shown

Comment 14

[Frank Widmaier]

what happened to the status? I don't understand why - I have removed it? sorry

Comment 15

[Frank Widmaier]

ok.. found it.. sorry - seems that I didn't read all of the crash

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

Akkana, any ideas here?  Seems related to bug 174369...

Comment 18

[Akkana Peck]

Attachment: A patch that fixes it

Here's a patch that fixes it.  But I don't understand why it worked before. 
Why is mailnews calling InsertAsCitedQuotation in the plaintext case?  Cited
quotations are only meaningful in html.  And why is mailnews always saying to
insert html when it calls InsertAsCitedQuotation?

I should perhaps take this bug, but I'd like to understand the answers to those
two questions better first.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 102882 [details] [diff] [review]
A patch that fixes it

Prior to your Oct 8 change to this code, it was using EditorShell instead of
editor.  If you look at nsEditorShell::InsertAsCitedQuotation, you'll see that
it completely ignores aInsertHTML in the case when the editor type is
ePlainTextEditorType.  In this case it just calls InsertAsQuotation() on the
editor, which turns around and calls InsertAsPlaintextQuotation().

As for why this function is being used.. it seems to be a prehistoric thing.
You added the "insert as html" flag at some point and changed nsMsgCompose.cpp
to pass it, always passing PR_TRUE (revision 1.151 of the file --
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/compose/src/nsMsgC
ompose.cpp&rev=1.151#255).  I have no idea what the relevant APIs looked like
around then and I'm not sure I want to find out.  ;)

Also, looks like QuotingOutputStreamListener::InsertToCompose has the same
problem (in the same file).

Comment 20

[Akkana Peck]

Aha!  Thanks, bz.  I got caught in the trap of assuming similarly named
functions in editorshell and editor did the same thing, rather than looking at
the functions to see what they actually did.  My bad.

Looking again, I don't think my proposed patch to nsMsgCompose.cpp is quite
right: the insertHTML argument should be set based on whether the quoted text is
html or plain, not whether the mail compose window will be html or plain.  I'm
not sure at this point how nsMsgCompose::ConvertAndLoadComposeWindo knows
whether aBuf is plaintext or html.  J-F, how can I tell that?

Taking the bug, will come up with a better patch tomorrow (coordinating with
cmanske, since he's redesigning the initialization sequence as part of
editorshell removal which may make some of this moot).  Though if we need a
quick fix, we could use this full patch, or we could leave nsMsgCompose.cpp
unchanged and use the editor portion minus the assert (since the assert will be
triggered due to nsMsgCompose.cpp passing aInsertHTML=PR_TRUE).

Comment 21

[Felix Miata]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Akkana Peck]

Attachment: Patch which tries to mimic old behavior

What we need at this point is just getting the old behavior back, so here's a
patch to nsMsgCompose.cpp that does what the old code used to do; plus a patch
to nsHTMLDataTransfer.cpp that guards against inserting html when we're
supposed to be editing plaintext, which would also have prevented the bug.

I'm a bit confused by a couple of points: (1) the mid: cite url is being
created even when we have no intention of using it -- wouldn't it be better not
to create it unless we're going to use it?  and (2) in the html case, the mail
code is always telling InsertAsCitedQuotation to insert html -- does that mean
that when the message being quoted is plaintext, it has been translated to html
before the compose window gets it? -- but neither of those are causing real
bugs, so they don't need to be addressed here.

Seeking review for this patch.	Mail people -- Ducarroz?  Varada?  Or someone
who uses it -- bz?

Comment 24

[Boris Zbarsky [:bzbarsky]]

What about QuotingOutputStreamListener::InsertToCompose?  Does that need to be
fixed?

sr=bzbarsky if not.

Comment 25

[Akkana Peck]

Attachment: Fix QuotingOutputStreamListener too

Oh, sorry, you asked about that before but I forgot about that part.

The code in QuotingOutputStreamListener::InsertToCompose doesn't even have a
cite, so it should just call InsertAsQuotation and let the editor handle the
rest (though under the hood it does the same thing, calling
InsertAsCitedQuotation with a null cite string).

I don't know where this code is used, so I'm not sure how to test this part (it
doesn't get called in my normal reply tests).  Ducarroz?  Varada?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 102969 [details] [diff] [review]
Fix QuotingOutputStreamListener too

sr=bzbarsky if JF or varada review...

Comment 27

[Jean-Francois Ducarroz]

Comment on attachment 102969 [details] [diff] [review]
Fix QuotingOutputStreamListener too

R=ducarroz.

You need to test reply as well the option "quote message" in ethe compose
window under the menu options.

Comment 28

[Akkana Peck]

Thanks!  I tested the quote menu item to quote messages after the compose window
was already up, and it works as expected in a plaintext editor.  But I hit a
problem quoting a multipart/related message into an html compose window: it
quoted the html source as plaintext, buried in an html quote.  Hitting reply
works correctly, it's just after-the-fact quoting that doesn't.

Comment 29

[Akkana Peck]

Attachment: Go back to using InsertCitedQuotation in the quoting listener

Well, it turns out the editor's InsertAsQuotation method always assumes
plaintext, and doesn't offer a way for the caller to specify html.  So it's
back to using InsertAsCitedQuotation and specifying html.  I've tested this
with {plain,html} compose windows quoting {plain,html} messages and all four
combinations work.

In the upcoming editor embedding api design work, I will make a point of making
sure that we offer more flexibility than we do now, so you don't have to make
an empty string and call InsertAsCitedQuotation unless you really want the
cite.

Comment 30

[Jean-Francois Ducarroz]

Comment on attachment 103001 [details] [diff] [review]
Go back to using InsertCitedQuotation in the quoting listener

R=ducarroz

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 103001 [details] [diff] [review]
Go back to using InsertCitedQuotation in the quoting listener

> +        mailEditor->InsertAsCitedQuotation(mMsgBody,
> +                                           empty, PR_TRUE,

As long as you're touching this, please use NS_LITERAL_STRING("") instead of
the nsAutoString temporary.  On Windows/Mac it's much faster (and smaller
footprint).

sr=bzbarsky with that change; no need for a new patch.

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 103001 [details] [diff] [review]
Go back to using InsertCitedQuotation in the quoting listener

a=asa for checkin to 1.2 (on behalf of drivers)

Comment 33

[Akkana Peck]

Fixed (including bz's NS_LITERAL_STRING suggestion).

Comment 34

[parish]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20021017

I've just installed the 2002-10-17-08 nightly (AFAIK the first to include this
fix) on W2K and a new bug has appeared that /maybe/ related to this fix. I filed
bug 175223 for it.

Comment 35

[Jerry Baker]

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

Comment 36

[Akkana Peck]

I can repro bug 175223 in a 2002101608 build, so I don't think it's caused by
this fix.  But I'll help with investigating that bug.

Comment 37

[Boris Zbarsky [:bzbarsky]]

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

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

Comment 39

[esther]

Using trunk build 20021029 on linux and 20021028 on winxp and 20021025 trunk on
Mac OSX this is fixed. Verified.

Comment 40

[Boris Zbarsky [:bzbarsky]]

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

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

Comment 42

[Boris Zbarsky [:bzbarsky]]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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