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

RESOLVED FIXED in Thunderbird 51.0

Status

Thunderbird
General
RESOLVED FIXED
6 years ago
6 months ago

People

(Reporter: Oliver Prygotzki, Assigned: Jorg K (GMT+2))

Tracking

Thunderbird 51.0

Thunderbird Tracking Flags

(thunderbird49 wontfix, thunderbird50 fixed, thunderbird_esr4550+ fixed, thunderbird51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
* 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).
if the email is in html do you see the bug ?
(Reporter)

Comment 2

6 years ago
(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
(Reporter)

Comment 3

6 years ago
Note: I just updated to the latest TB (6.0); the bug is still there.
Did you try in -safe-mode too ? (see http://support.mozillamessaging.com/en-US/kb/Safe-Mode) ?
(Reporter)

Comment 5

6 years ago
@Ludovic Hirlimann: Yes, still there.

Would someone from mozilla.org please confirm this bug?!
(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 ?
Keywords: testcase-wanted
Whiteboard: [closeme 2012-02-23]
(Reporter)

Comment 7

5 years ago
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?
Whiteboard: [closeme 2012-02-23]

Comment 8

5 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [closeme 2012-02-23]
Keywords: testcase-wanted
Whiteboard: [closeme 2012-02-23]
Summary: Reply (with selected text) results in wrong quoting → Reply (with selected text containing quote) results in wrong quoting level indication
(Assignee)

Comment 9

9 months ago
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.
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1161337
(Assignee)

Comment 11

9 months ago
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).
(Assignee)

Comment 12

9 months ago
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.
(Assignee)

Comment 13

9 months ago
Created attachment 8787380 [details]
Test message
(Assignee)

Comment 14

9 months ago
Created attachment 8787391 [details] [diff] [review]
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.
(Assignee)

Comment 15

9 months ago
Created attachment 8787424 [details] [diff] [review]
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.
Assignee: nobody → jorgk
Attachment #8787391 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8787424 - Flags: review?(mkmelin+mozilla)

Comment 16

9 months ago
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
Attachment #8787424 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 17

9 months ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-thunderbird49: --- → wontfix
status-thunderbird50: --- → affected
status-thunderbird51: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
(Assignee)

Comment 18

9 months ago
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.
Attachment #8787424 - Flags: approval-comm-aurora+
(Assignee)

Comment 19

9 months ago
Aurora (TB 50):
https://hg.mozilla.org/releases/comm-aurora/rev/6e41f3a00c6b
status-thunderbird50: affected → fixed

Comment 20

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

[Approval Request Comment]
This is tracking-tb45 and has been through beta.
Attachment #8787424 - Flags: approval-comm-esr45?

Comment 21

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

https://hg.mozilla.org/releases/comm-esr45/rev/5caf951a02fc
Attachment #8787424 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

6 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 50+
You need to log in before you can comment on or make changes to this bug.