Attachment reminder should not search signatures

RESOLVED FIXED in Thunderbird 3.0rc1

Status

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: david.waring, Assigned: mkmelin)

Tracking

unspecified
Thunderbird 3.0rc1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.9.1.2) Gecko/20090803 Fedora/3.5.2-2.fc11 Firefox/3.5.2
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.9.1.1) Gecko/20090814 Fedora/3.0-2.6.b3.fc11 Thunderbird/3.0b3

One of my signatures has a usual disclaimer about the message and any attachment. When sending a message with this signature the attachment reminder pops up even though the message body does not contain any keywords.

Reproducible: Always

Steps to Reproduce:
1. Create a signature containing the word "attachment".
2. Compose a message using the signature.
3. Try to send

Actual Results:  
Attachment Reminder pops up

Expected Results:  
Attachment Reminder should not appear unless the keywords are in the message body.

Comment 1

10 years ago
I have the same issue. A fix would be great. Thanks.
Assignee

Updated

10 years ago
Assignee: nobody → mkmelin+mozilla
OS: Linux → All
Hardware: x86_64 → All
Assignee

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 2

10 years ago
Posted patch proposed fix (obsolete) — Splinter Review
BTW, what's 

var brs = ... supposed to do. Was added in bug 498519, but surely node.textContent will change the brs to newlines for you anyway?
Attachment #402875 - Flags: review?(bwinton)
Assignee

Updated

10 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 3.0rc1
I believe it didn't, and just removed the nodes, so "abc<br/>def" got turned into "abcdef".  (Otherwise, textContent would be changing the text nodes, which sounds kind of wrong.)

If it works in html and text replies without the "var brs", then please remove that line.  :)

Thanks,
Blake.
Assignee

Comment 4

10 years ago
Right you are. I'll just add a comment to node that then. (Won't bother uploading a new patch for that.)
Attachment #402875 - Flags: review?(bwinton) → review-
Comment on attachment 402875 [details] [diff] [review]
proposed fix

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -1309,22 +1309,30 @@ function ShouldShowAttachmentNotificatio
>+    // Ignore signatures.
>+    let sigs = mailBodyName.getElementsByClassName("moz-signature");

s/mailBodyName/mailBodyNode

>+    for (let i = 0; i < sigs.length; i++) {
>+      sigs[i].parentNode.removeChild(sigs[i]);
>+    }

The patch doesn't seem to strip the signature from plain-text messages.

I'm guessing that's going to be "non-trivial"?  :)

Maybe we could either skip everything after the first "-- ", or check to
see what the user's signature text is, and remove the first copy of "-- \n<user's sig text>".

Thanks,
Blake.
Assignee

Comment 6

10 years ago
Posted patch proposed fix, v2 (obsolete) — Splinter Review
Gah, i guess it "worked" by not working at all when i tested.
Attachment #402875 - Attachment is obsolete: true
Attachment #404528 - Flags: review?(bwinton)
Just some notes to myself for when I review this tonight:

1) I wonder how that'll work for those old folk among us who still reply in-line.  Maybe searching for "<br>-- <br>" (or something similar) would let us skip quoted signatures?

2) Does searching for "-- <br>" work?  It seems odd that we would use <br> in plaintext replies…  (Not wrong, just odd.)

Thanks,
Blake.
Comment on attachment 404528 [details] [diff] [review]
proposed fix, v2

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>     let brs = mailBodyNode.getElementsByTagName("br");
>     for (let i = 0; i < brs.length; i++) {
>       brs[i].parentNode.replaceChild(document.createTextNode("\n"), brs[i]);
>     }

Yeah, that code really doesn't do what I thought it did.  :)

Flipping it to:
for (let i = brs.length - 1; i >= 0; i--) {
makes it work a lot better, as would changing the body to:
brs[0].parentNode.replaceChild(document.createTextNode("\n"), brs[0]);

Specifically, it seems that the return values from getElementsBy{Tag,Class}Name() are live collections, so when you replace the first one, the length goes down by 1, and so we end up only replacing half of the instances in those for loops.

By the same token, we should probably flip the test in:
for (let i = 0; i < sigs.length; i++) {
And the rest of the for loops, too.

>+    // Ignore signature (plain text compose mode).
>+    let mailBodyinnerHTML = mailBodyNode.innerHTML;
>+    let sigIndex = mailBodyinnerHTML.indexOf("-- <br>");

And, of course, once we make that fix, and we're actually changing all the "<br>"s to "\n"s, this call always returns -1.  :)  So we'll need to change that constant to something like "\n-- \n".

(I tested those changes with the plaintext reply, and they seemed to work.  I didn't make all the changes, but I can confirm that the html reply is still broken, likely due to the "blockquote" and "span" and "pre" for loops earlier in the method.  Picture of the broken html reply to be posted shortly.)

Thanks,
Blake.
Attachment #404528 - Flags: review?(bwinton) → review-
But I'm sure you'll fix it all up.  :)

Thanks,
Blake.
Assignee

Comment 10

10 years ago
Nice catch!
Attachment #404528 - Attachment is obsolete: true
Attachment #404737 - Attachment is obsolete: true
Attachment #404883 - Flags: review?(bwinton)
Comment on attachment 404883 [details] [diff] [review]
proposed fix, v3

Looks awesome, and passes every degenerate test case I could think of.  :)

Well, almost every case.  One thing I noticed it didn't handle was when I copied and pasted quoted text in a plain-text reply, but then, the spell-checker also spell-checks that text, so I'm not too concerned about that behaviour.

Later,
Blake.
Attachment #404883 - Flags: review?(bwinton) → review+
Assignee

Comment 12

10 years ago
Comment on attachment 404883 [details] [diff] [review]
proposed fix, v3

Approving; well contained fix.
Attachment #404883 - Flags: approval-thunderbird3+
Assignee

Comment 13

10 years ago
changeset:   4105:bdf866b86b41
http://hg.mozilla.org/comm-central/rev/bdf866b86b41

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.