Closed
Bug 511417
Opened 14 years ago
Closed 14 years ago
Attachment reminder should not search signatures
Categories
(Thunderbird :: Message Compose Window, defect)
Thunderbird
Message Compose Window
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0rc1
People
(Reporter: david.waring, Assigned: mkmelin)
Details
Attachments
(1 file, 3 obsolete files)
2.77 KB,
patch
|
bwinton
:
review+
mkmelin
:
approval-thunderbird3+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
I have the same issue. A fix would be great. Thanks.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → mkmelin+mozilla
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 3.0rc1
Comment 3•14 years ago
|
||
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•14 years ago
|
||
Right you are. I'll just add a comment to node that then. (Won't bother uploading a new patch for that.)
Updated•14 years ago
|
Attachment #402875 -
Flags: review?(bwinton) → review-
Comment 5•14 years ago
|
||
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•14 years ago
|
||
Gah, i guess it "worked" by not working at all when i tested.
Attachment #402875 -
Attachment is obsolete: true
Attachment #404528 -
Flags: review?(bwinton)
Comment 7•14 years ago
|
||
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•14 years ago
|
||
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-
Comment 9•14 years ago
|
||
But I'm sure you'll fix it all up. :) Thanks, Blake.
Assignee | ||
Comment 10•14 years ago
|
||
Nice catch!
Attachment #404528 -
Attachment is obsolete: true
Attachment #404737 -
Attachment is obsolete: true
Attachment #404883 -
Flags: review?(bwinton)
Comment 11•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 404883 [details] [diff] [review] proposed fix, v3 Approving; well contained fix.
Attachment #404883 -
Flags: approval-thunderbird3+
Assignee | ||
Comment 13•14 years ago
|
||
changeset: 4105:bdf866b86b41 http://hg.mozilla.org/comm-central/rev/bdf866b86b41 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•