669073 - Reply (with selected text containing quote) results in wrong quoting level indication

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Oliver Prygotzki]

* Steps to Reproduce:

1) Given the stored plain text mail:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Example wrote:
> Lorem ipsum dolor sit amet
Foo bar
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2) Select the first two lines, trigger Reply

This is the build-in feature 'Reply quoting selected text only'.

* Actual Results:

In the mail compose window you’ll see:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Sender wrote:
> Example wrote:
>> > Lorem ipsum dolor sit amet
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Please note the messed up quoting level in the last line.

* Expected Results:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Sender wrote:
> Example wrote:
>> Lorem ipsum dolor sit amet
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Every reply should increase the quoting level just by 1.

* Build Date & Platform:

Latest release (Thunderbird 5.0 with Gecko 20110624 on MacOS X 10.6.7).

* Additional Information:

· The mails, to which I reply, are stored on disk (POP3).

· The setting in 
Tools › Account Settings › Mail Account › Composition & Addressing › Composition is:
[ ] (= deselected) Compose messages in HTML format
[X] (= selected) Automatically quote the original message when replying
  Then › Start my reply below the quote.

· This bug is still there in safe mode (all add-ons disabled).

Comment 1

[Ludovic Hirlimann [:Usul]]

if the email is in html do you see the bug ?

Comment 2

[Oliver Prygotzki]

(In reply to comment #1)
> if the email is in html do you see the bug ?

I'll test later.

* Additional information

My 'User set' settings:

mailnews.send_plaintext_flowed = false
mail.quoted_graphical = false
mail.quoteasblock = false
mail.display_glyph = false

Comment 3

[Oliver Prygotzki]

Note: I just updated to the latest TB (6.0); the bug is still there.

Comment 4

[Ludovic Hirlimann [:Usul]]

Did you try in -safe-mode too ? (see http://support.mozillamessaging.com/en-US/kb/Safe-Mode) ?

Comment 5

[Oliver Prygotzki]

@Ludovic Hirlimann: Yes, still there.

Would someone from mozilla.org please confirm this bug?!

Comment 6

[Ludovic Hirlimann [:Usul]]

(In reply to Oliver Prygotzki from comment #5)
> @Ludovic Hirlimann: Yes, still there.
> 
> Would someone from mozilla.org please confirm this bug?!

I would If I saw the issue - can you attach an example email to this bug ?

Comment 7

[Oliver Prygotzki]

Hello back!

I just reproduced this bug again, with the latest TB (Gecko/20120129 Thunderbird/10.0 on MacOS X 10.7.2), all add-ons disabled.

Wayne, what would a testcase be?

Comment 8

[Joe Sabash [:JoeS1]]

My results on your example:
On 2/12/2012 6:56 PM, JoeS wrote:
> Example wrote:
>> >  Lorem ipsum dolor sit amet
So indeed the quote level is screwed up. Don't know though if the "quote selected" feature was ever intended to handle selection across a quote boundary.
Tested with:
Mozilla/5.0 (Windows NT 5.0; rv:12.0a1) Gecko/20120131 Thunderbird/12.0a1 ID:20120131030036

Comment 9

[Jorg K (CEST = GMT+2)]

This bug is starting to annoy me since I have to remove the erroneous citation marks manually every time. I should put it onto my to-do list.

Comment 11

[Jorg K (CEST = GMT+2)]

Note that the erroneous citation mark is inserted as:
<span class="moz-txt-citetags">&gt; </span>.

Looking for moz-txt-citetags, I can see only one possible culprit here:
https://dxr.mozilla.org/comm-central/search?q=moz-txt-citetags&redirect=false
https://dxr.mozilla.org/comm-central/rev/8c7ddccf95c4168995fa6e57182133f22d21be03/mailnews/mime/src/mimetpla.cpp#386

So maybe this isn't an M-C serialiser problem after all, so it's under C-C control to fix which makes matters much simpler. Well, let's hope it's not in the ScanTXT() call (in M-C: mozTXTToHTMLConv.cpp).

Comment 12

[Jorg K (CEST = GMT+2)]

Sadly comment #11 is about 100% wrong. The "moz-txt-citetags" are placed into the original message when it is converted to HTML for viewing from plain text message content in mimetpla.cpp.

The extra citation marks are most likely an M-C serialiser problem when the selection is serialised to be placed into the reply.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Test message

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Debug patch (v1).

The message contains:
Select the four lines below and hit reply:
>> double cited line1
>> double cited line2
> single cited line
Not cited.

Debug shows:
=== Plain text selection |>> double cited line1
>> double cited line2
> single cited line
Not cited.
|
=== String received back |<blockquote type="cite" style="color: #000000;"><blockquote type="cite" style="color: #000000;"><pre wrap="">double cited line1
<span class="moz-txt-citetags">&gt;&gt; </span>double cited line2
</pre></blockquote><pre wrap=""><span class="moz-txt-citetags">&gt; </span>single cited line
</pre></blockquote><pre wrap="">Not cited.

</pre>|

Looks like the M-C encoder did the right thing by detecting nested block quotes, but the <span class="moz-txt-citetags">&gt; </span> which *were* in the original HTML need to be removed. The M-C encoder has no knowledge of them since they are placed in MIME, see comment #12.

Hmm, that's not too hard to do. Patch coming up, basically an exercise in string manipulation.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a).

(printf will be removed before landing).

This does the trick and fixes a long-standing nuisance.

Resulting HTML:

=== Html |<blockquote type="cite" style="color: #000000;"><blockquote type="cite" style="color: #000000;"><pre wrap="">double cited line1
double cited line2
</pre></blockquote><pre wrap="">single cited line
</pre></blockquote><pre wrap="">Not cited.

</pre>|

Works for HTML replies and plain text replies. Beautiful.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8787424 [details] [diff] [review]
Proposed solution (v1a).

Review of attachment 8787424 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM (remember to remove the printf). r=mkmelin

Would be nice with a test

Comment 17

[Jorg K (CEST = GMT+2)]

Thanks for the quick review. Landed without the "printf":
https://hg.mozilla.org/comm-central/rev/cad10ca8704c

Yes, in an ideal world we'd have more tests, but this one is not so likely to break.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8787424 [details] [diff] [review]
Proposed solution (v1a).

[Approval Request Comment]
Regression caused by (bug #): No regression, long-standing nuisance bug.
User impact if declined: Can't reply properly to plaintext e-mails with quotations.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Small local change, not risky. Good to have for TB 50 beta.

Comment 19

[Jorg K (CEST = GMT+2)]

Aurora (TB 50):
https://hg.mozilla.org/releases/comm-aurora/rev/6e41f3a00c6b

Comment 20

[Kent James (:rkent)]

Comment on attachment 8787424 [details] [diff] [review]
Proposed solution (v1a).

[Approval Request Comment]
This is tracking-tb45 and has been through beta.

Comment 21

[Kent James (:rkent)]

Comment on attachment 8787424 [details] [diff] [review]
Proposed solution (v1a).

https://hg.mozilla.org/releases/comm-esr45/rev/5caf951a02fc