Plaintext signatures are not received properly due to lack of BR nodes.

VERIFIED FIXED in Thunderbird 14.0

Status

Thunderbird
Message Compose Window
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: ohghoo8n, Assigned: mconley)

Tracking

({regression})

13 Branch
Thunderbird 14.0
regression

Thunderbird Tracking Flags

(thunderbird13 fixed, thunderbird14 verified)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 607153 [details]
A sample (redacted) showing a received message where the newlines have been stripped from my signature, and stay on line after the "--".

Using Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120318 Thunderbird/14.0a1 ID:20120318030022 for a standard account on a Mirapoint IMA4rev1 server.  I have noticed the following issue where a newline is not inserted between the "--" and the signature, or that and every line of the signature does not have their respective newlines.

Signature format:
Firstname A Lastname
My Work Position
My Department
My Institution
Email: me@company.tld
Office: +123 12345678
Cell: +123 12345678

My vCard format (snipped from the problematic message source):

--------------020207090705040603090106
Content-Type: text/x-vcard; charset=utf-8;
 name="myworkemailhandle.vcf"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="myworkemailhandle.vcf"

begin:vcard
fn:Firstname A Lastname
n:Lastname;Firstname
org:My Institution;My Department
adr:My Corporate Campus;;P.O. Box 12345;City;;12345;Country
email;internet:me@company.tld
title:My Work Position
tel;work:+123 12345678
tel;cell:+123 12345678
url:http://www.linkedin.com/in/myuserprofile
version:2.1
end:vcard


--------------020207090705040603090106--

Steps to recreate the problem:

1) Click Ctrl+N to compose a new message.

2) Enter message recipient.

3) Enter message subject.

4) Type message body content before the --, where the cursor is placed by default. After the --, Daily has automatically injected my text-only signature (which has defined per-account in Tools > Accounts Settings > Account Name > Signature text; additionally, a vCard is specified and configured to automatically attach appropriately). The signature at this point *appears correctly*.

5) Click Ctrl+Enter to send.

Desired Result:

Message has appropriately formatted signature, looking like so.
--
Firstname A Lastname
My Work Position
My Department
My Institution
Email: me@company.tld
Office: +123 12345678
Cell: +123 12345678

Actual result:

The newlines are stripped or never added. (See attached screenshot, signature.newline.1.png)

-- Firstname A Lastname My Work Position My Department My Institution Email: me@company.tld Office: +123 12345678 Cell: +123 12345678

Notes:

I had not reported this for a few days because it is actually difficult to recreate.  It is not happening consistently, and I am not particularly sure why.
(Reporter)

Comment 1

5 years ago
Created attachment 607157 [details]
A screenshot showing the newlines are properly inserted in the composition window.
(Reporter)

Updated

5 years ago
Severity: normal → minor

Comment 2

5 years ago
Confirming with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120319 Thunderbird/14.0a1 with a minimal example (1-line signature "test", no vCard).
Content-Type: text/plain; charset=ISO-8859-1; format=flowed.

Using plain-text editor. Last bugs affecting signature spacing were bug 698925 (BigFiles) with a regression resolved in bug 735380.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: Thunderbird Daily Does Not Add Newline Between -- And Signature → Thunderbird Daily Does Not Add Newline Between -- And Signature (plain text composition)
Assignee: nobody → mconley
Blocks: 698925
OS: Windows 7 → All

Updated

5 years ago
Severity: minor → normal
status-thunderbird13: --- → affected
status-thunderbird14: --- → affected
tracking-thunderbird13: --- → ?

Comment 3

5 years ago
Created attachment 607252 [details] [diff] [review]
tweak mconley's patch to get it to compile
Summary: Thunderbird Daily Does Not Add Newline Between -- And Signature (plain text composition) → Plaintext signatures are not received properly due to lack of BR nodes.
Created attachment 607309 [details] [diff] [review]
Patch v1

Sorry this took me so long - string conversion in C++ is not something I'm really used to.

Here's my first run at a solution.  I've sent a plaintext message to myself with a multi-line plaintext signature and it seems to work.

I had to update a test to make it pass properly.

I'm splitting the string based on the "\n" character...am I missing any corner cases?
Attachment #607252 - Attachment is obsolete: true
Attachment #607309 - Flags: review?(dbienvenu)

Comment 5

5 years ago
Some code in nsMsgCompose::ProcessSignature() compares on all platform variants of line terminators, i.e., "\n", "\r", and "\r\n" (the first form applies to Linux but not necessary to Mac OSX and Windows, I don't know though if any automated conversion is applied by now, e.g., when reading the signature from a file).

There was a discussion I had with Neil back in bug 428040 that signatures are allowed to either have or have not a trailing newline at the end, where both cases are considered equal to produce the same result (hence the handling of CRLF cases in the resulting fix there, which to some extent were removed in bug 698925 or bug 735380; you may want to verify that the logic envisioned there still applies after the various changes in the wake of the BigFiles implementation).

Comment 6

5 years ago
Comment on attachment 607309 [details] [diff] [review]
Patch v1

so, I think either SplitString should be in nsMsgUtils.cpp/h where other code could use it (and probably take a PRUnichar delimiter), or we should just do it inline, like the patch I'm about to attach.
Attachment #607309 - Flags: review?(dbienvenu) → review-

Comment 7

5 years ago
Created attachment 607443 [details] [diff] [review]
parse string inline

Comment 8

5 years ago
Comment on attachment 607443 [details] [diff] [review]
parse string inline

and remove SplitString, of course :-)  This is the approach I was trying to describe over IRC...
Created attachment 607532 [details] [diff] [review]
Patch v2

Thanks for the assist on this, David - I've removed SplitString in nsMsgCompose.h/.cpp as suggested.
Attachment #607309 - Attachment is obsolete: true
Attachment #607443 - Attachment is obsolete: true
Attachment #607532 - Flags: review?(dbienvenu)

Comment 10

5 years ago
Comment on attachment 607532 [details] [diff] [review]
Patch v2

great, thx.
Attachment #607532 - Flags: review?(dbienvenu) → review+
Comment on attachment 607532 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 698925
User impact if declined: Plaintext signatures will not be received correctly.
Testing completed (on c-c, etc.): comm-central.
Risk to taking this patch (and alternatives if risky): None that I can think of - the patch is pretty straight-forward.
Attachment #607532 - Flags: approval-comm-aurora?

Updated

5 years ago
Attachment #607532 - Flags: approval-comm-aurora? → approval-comm-aurora+
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/342f2874aa70
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/62d1ea4c543c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-thunderbird13: affected → fixed
status-thunderbird14: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
status-thunderbird14: fixed → ---
tracking-thunderbird13: ? → +

Comment 13

5 years ago
Seems fine now with:
Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20120321 Thunderbird/14.0a1
Severity: normal → minor
Status: RESOLVED → VERIFIED
status-thunderbird13: fixed → ---
tracking-thunderbird13: + → ---
OS: All → Windows 7
Target Milestone: Thunderbird 14.0 → ---
Joe:

Awesome, thanks!

-Mike

Comment 15

5 years ago
Looks like bugzilla screwed up the flags with comment #13, restoring them except for tracking-thunderbird13+ which I can't set (but hopefully no longer relevant).
Severity: minor → normal
status-thunderbird13: --- → fixed
status-thunderbird14: --- → verified
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → Thunderbird 14.0
Version: 14 → 13
(Reporter)

Comment 16

5 years ago
Works for me after applying update. Thanks guys.
Severity: normal → minor
status-thunderbird13: fixed → ---
status-thunderbird14: verified → ---
OS: All → Windows 7
Hardware: All → x86
Version: 13 → 14
(Reporter)

Comment 17

5 years ago
(In reply to rsx11m from comment #15)
> Looks like bugzilla screwed up the flags with comment #13, restoring them
> except for tracking-thunderbird13+ which I can't set (but hopefully no
> longer relevant).

Ugh. I am an ass and mashed buttons quickly.  I did not mean to reset flags specifically after you mentioned it.  Apologies and mea culpa.  Let me know if you want me to do anything.

Comment 18

5 years ago
Easy to "re-fix" but I'm wondering why this happened twice in a row... :-\
Severity: minor → normal
status-thunderbird13: --- → fixed
status-thunderbird14: --- → verified
OS: Windows 7 → All
Hardware: x86 → All
Version: 14 → 13
Duplicate of this bug: 737736

Comment 20

5 years ago
+  const kSeperator = "-- ";

This probably wanted to be kSeparator?
(In reply to :aceman from comment #20)
> +  const kSeperator = "-- ";
> 
> This probably wanted to be kSeparator?

Good catch!  Feel like throwing together a patch?

Comment 22

5 years ago
Surely. Can I attach it here?
Sure - and then I'll commit it as a followup.

Comment 24

5 years ago
Created attachment 608054 [details] [diff] [review]
fix typo in kSeperator
Attachment #608054 - Flags: review?(mconley)
Comment on attachment 608054 [details] [diff] [review]
fix typo in kSeperator

Great, thanks!
Attachment #608054 - Flags: review?(mconley) → review+

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [checkin patch 608054]
aceman:

Thanks, follow-up checked in to comm-central as http://hg.mozilla.org/comm-central/rev/8a8a82bf4a24

-Mike
Keywords: checkin-needed
Whiteboard: [checkin patch 608054]
Created attachment 609355 [details] [diff] [review]
Use ints instead of iterators for external API compatibility
Attachment #609355 - Flags: review?(dbienvenu)

Updated

5 years ago
Attachment #609355 - Flags: review?(dbienvenu) → review+
Comment on attachment 609355 [details] [diff] [review]
Use ints instead of iterators for external API compatibility

Pushed changeset 6208e3c32bad to comm-central.
You need to log in before you can comment on or make changes to this bug.