279745 - (erroneous?) quote as block comment triggers plain text conversion warning

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Find a plain text message that quotes another message
2. Reply to it in HTML format
3. Set your send format to auto-detect
4. Post your reply, but without adding your own formatting

Expected result: reply posted

Actual result: HTML conversion warning.

This appears to be triggered by an HTML comment inserted by mimetpla.cpp

Comment 1

[[e-mail]]

Attachment: Proposed patch

This works for me but then I don't understand why that code was there
originally.

Comment 2

[Ben Bucksch (:BenB)]

As the comment says, I had a case where Gecko didn't put out a linebreak (after
a quote when sender's text starts, I think), although it should have, and that
fixed it. Maybe the bug has been fixed by now, but I don't know the exact
probloem anymore to test it, and there are a variety of cases. Maybe I'll test
it, when I get around to test bug 130728.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Neil, Ben, ping on this bug with a patch pending reviews.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #3)
> Neil, Ben, ping on this bug with a patch pending reviews.

bienvenu too...

fwiw, I just tested this patch and (yay!) it has not bitrotted over the last 4 years. :)

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 172343 [details] [diff] [review]
Proposed patch

I couldn't trigger the error, but we shouldn't have to be doing this anyway, and if we do, it seems like a separate bug.

Comment 6

[[e-mail]]

Pushed changeset 5210f1319bcd to comm-central.

Comment 7

[Hartmut Figge]

Attachment: Missing blank lines

And now blank lines between quoted and normal text are missing. *g*
Left is without the patch and right with the patch.

Comment 8

[[e-mail]]

If you use DOM Inspector to delete the comment nodes from the DOM generated by the previous version, do the blank lines disappear there too?

Comment 9

[Hartmut Figge]

Sorry, i am not familiar with the DOM Inspector.

Comment 10

[[e-mail]]

In that case, would you mind attaching the message, so that I have a known test case, and also let me know whether you are using any non-default preferences e.g. for disabling format=flowed.

Comment 11

[Hartmut Figge]

Attachment: Another case

Hm, you do not see this missing blank lines? The first screenshot was from a posting in de.comm.software.mozilla.nightly-builds, the second is from mozilla.dev.apps.seamonkey.

<[e-mail]>

I assume that you have access to news.mozilla.org and can see this posting there. ;)

My preferences, which may affect this bug:

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

Comment 12

[Ben Bucksch (:BenB)]

Thanks Hartmut.

That is *precisely* why the <!----> was there. The source code comment even says that, and I said it in comment 2.

Please back out the patch until there's a better fix.

Comment 13

[Karsten Düsterloh]

(In reply to comment #8)
> If you use DOM Inspector to delete the comment nodes from the DOM generated by
> the previous version, do the blank lines disappear there too?

No. 
But without the comment node, the first #text inside the <pre> gets ripped of the leading empty line.

Comment 14

[[e-mail]]

Attachment: Regression fix

So, philor pointed out the spec which shows that HTML eats newlines before and after tags. Most of the time this isn't a problem, because newlines usually get ignored anyway, but obviously in this one case we need to deliberately put a newline in to stop the HTML parser from eating into our data.

Comment 15

[Ben Bucksch (:BenB)]

Ah, thanks!

You're sure they should be there in all these cases?

Please test with pref "mail.quoted_graphical" = true (default) and false, and
with multiple quote levels.
The other prefs mentioned should not have an influence.
Test with a normal plaintext mail, *not* format-flowed as Thunderbird emits.
(Testing f=f is useless in this bug, as f=f does not go through this class.)

Comment 16

[[e-mail]]

(In reply to comment #15)
> You're sure they should be there in all these cases?
The point is that adding the \n in all cases leads to reproducible behaviour (i.e. it ceases to matter what the following data is.)

> Please test with pref "mail.quoted_graphical" = true (default) and false, and
> with multiple quote levels.
Done.

> The other prefs mentioned should not have an influence.
> Test with a normal plaintext mail, *not* format-flowed as Thunderbird emits.
> (Testing f=f is useless in this bug, as f=f does not go through this class.)
Actually that is why the suggestion was to use
user_pref("mailnews.display.disable_format_flowed_support", true);
as then this code gets used even for f=f mail.

user_pref("mailnews.quoteasblock") seems to affect replying in HTML format.
user_pref("mailnews.quoted_graphical") only affects the display of the >s.

Comment 17

[[e-mail]]

Pushed changeset dc17a79d4b83 to comm-central for the regression fix.