511417 - Attachment reminder should not search signatures

Status: RESOLVED FIXED

Description

[David Waring]

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

[Michael Kolb]

I have the same issue. A fix would be great. Thanks.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

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?

Comment 3

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 4

[Magnus Melin [:mkmelin]]

Right you are. I'll just add a comment to node that then. (Won't bother uploading a new patch for that.)

Comment 5

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Gah, i guess it "worked" by not working at all when i tested.

Comment 7

[Blake Winton (: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 8

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: Yeah, none of those words should trigger the notification, iiuc.

But I'm sure you'll fix it all up.  :)

Thanks,
Blake.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Nice catch!

Comment 11

[Blake Winton (: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.

Comment 12

[Magnus Melin [:mkmelin]]

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

Approving; well contained fix.

Comment 13

[Magnus Melin [:mkmelin]]

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

-> FIXED