Closed Bug 511417 Opened 14 years ago Closed 14 years ago

Attachment reminder should not search signatures


(Thunderbird :: Message Compose Window, defect)

Not set


(Not tracked)

Thunderbird 3.0rc1


(Reporter: david.waring, Assigned: mkmelin)



(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv: 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: 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.
I have the same issue. A fix would be great. Thanks.
Assignee: nobody → mkmelin+mozilla
OS: Linux → All
Hardware: x86_64 → All
Ever confirmed: true
Attached 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)
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.  :)

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");


>+    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>".

Attached 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.)

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

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

Attached patch proposed fix, v3Splinter Review
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.

Attachment #404883 - Flags: review?(bwinton) → review+
Comment on attachment 404883 [details] [diff] [review]
proposed fix, v3

Approving; well contained fix.
Attachment #404883 - Flags: approval-thunderbird3+
changeset:   4105:bdf866b86b41

Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.