The default bug view has changed. See this FAQ.

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
4 months ago

People

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

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

7 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

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

Comment 11

7 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

7 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

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

Comment 14

7 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

7 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

7 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

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

7 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

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

Comment 20

4 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

4 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

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