Relevant Thunderbird Bugs: [Bug 244455] Help prevent forgetting inclusion of attachments. [Bug 492438] Attachment reminder dialog should explain what triggered it. [Bug 492485] Missing attachment check should only look at text, not element names and attribute names and values. [Bug 492555] attachment reminder: Too common keywords, many false triggers. [Bug 492573] Attachment reminder dialog needs opt-out checkbox. [Bug 492647] Attachment reminder should .test, not .exec. [Bug 495943] Attachment reminder should not search quoted text in replies (plain-text case). [Bug 511417] Attachment reminder should not search signatures. [Bug 514850] Ignore contents of forwarded mail that lacked attachment in attachment detection. [Bug 518215] pref for more prominent attachment reminder. [Bug 520706] Attachment checker doesn't work with CJK keyword. [Bug 527018] Attachment reminder does not works with Greek (and other non-latin)keywords. [Bug 613195] No keyword detection for missing attachment feature.

This is a WIP patch in terms that the pref-attachmentReminder.[xul/js] don't function. There are no css rules for mac. There are a few optimizations and "Remind Me later" button left from implementation (bug 521158, bug 938829). Tests are left (bug 939700) and maybe a few more (I am unable to recall them right now). But this patch does what it is supposed to. Please let me know if this functions as expected. Thanks.

Comment on attachment 8391854 [details] [diff] [review] WIP patch v1 Review of attachment 8391854 [details] [diff] [review]: ----------------------------------------------------------------- I do not build Seamonkey, but the patch looks like a copy of the code from TB's MsgComposeCommands.js. So it should be fine, just needs testing for proper integration from SM users. New Binary File: suite/themes/classic/messenger/icons/compose-toolbar.png New Binary File: suite/themes/modern/messenger/icons/compose-toolbar-aero.png I don't think SM's "modern" theme equals TB's windows aero theme. Maybe you need to use the same compose-toolbar.png file in classic and modern themes. But I'm sure SM theme guys will comment on that. ::: mailnews/mailnews.js @@ +110,5 @@ > +pref("mail.compose.attachment_reminder_keywords", "chrome://messenger/locale/messengercompose/composeMsgs.properties"); > +// When no action is taken on the inline missing attachement notification, > +// show an alert on send? > +pref("mail.compose.attachment_reminder_aggressive", true); > + Good idea moving these to shared location. ::: suite/mailnews/modules/attachmentChecker.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Does this file differ from the Thunderbird one? Of not, could you just move the TB one into a shared location like /mailnews/compose/content ?

Made the suggested changes. Thanks.

Comment on attachment 8394348 [details] [diff] [review] WIP Patch v2 > diff --git a/mailnews/compose/content/attachmentChecker.js b/mailnews/compose/content/attachmentChecker.js > new file mode 100644 > --- /dev/null > +++ b/mailnews/compose/content/attachmentChecker.js Please use hg move, hg copy, etc instead of OS level moves. This way you retain the history of the file. > --- /dev/null > +++ b/suite/mailnews/compose/prefs/pref-attachmentReminder.js Please spin off the Prefs UI changes to a separate bug in SeaMonkey::Preferences. > +function GetAttachmentKeywords(mailData,keywordsInCsv) Add a space after comma. function GetAttachmentKeywords(mailData, keywordsInCsv) > +<!ENTITY attachmentReminderOptions.label "Keywords…"> > +<!ENTITY attachmentReminderOptions.accesskey "K"> "K" clashes with _k_eyboard > Components.utils.import("resource:///modules/folderUtils.jsm"); > Components.utils.import("resource:///modules/iteratorUtils.jsm"); > +Components.utils.import("resource:///modules/attachmentChecker.js"); Might sort alphabetically > +/** > + * Handle ESC keypress from composition window (to close attachment reminder notification bar). Wrap at column 80 please. > + */ > +function handleEsc() > +{ > + // If there is an attachment reminder notification AND focus is in message body Wrap at column 80 please. > + // or on the notification, hide it. > + let activeElement = document.activeElement; > + let notification = document.getElementById("attachmentNotificationBox").currentNotification; Might be better to create a utility function e.g. function getNotificationBox() Also you should use: .getNotificationWithValue("attachmentReminder"); > + if (notification && (activeElement.id == "content-frame" || > + notification.contains(activeElement) || > + activeElement.classList.contains("messageCloseButton"))) { > + notification.close(); if (notification && (activeElement.id == "content-frame" || notification.contains(activeElement))) { ... } > + if (keywordsFound.length > 0) { if (keywordsFound.length) { > + msgKeywords.id = "attachmentKeywords"; > + msgKeywords.setAttribute("crop", "end"); > + msgKeywords.setAttribute("flex", "1000"); Sigh. flex=1000!? I'll address the notifictaionbox code in a later round. > + CheckForAttachmentNotification.shouldFire = true; Any idea why this property is attached to CheckForAttachmentNotification()? > + let blockquotes = mailBodyNode.getElementsByTagName("blockquote"); Use querySelectorAll() this returns a non-live nodelist so you don't have to iterate backwards. > + for (let i = blockquotes.length - 1; i >= 0; i--) { > + blockquotes[i].remove(); for (let blockquote of blockquotes) blockquote.remove(); > + let sigs = mailBodyNode.getElementsByClassName("moz-signature"); > + let brs = mailBodyNode.getElementsByTagName("br"); Same Same. > + let sigIndex = mailData.indexOf("-- \n"); I think this should be "\n-- \n" > + if (!sComposeMsgsBundle) > + sComposeMsgsBundle = document.getElementById("bundle_composeMsgs"); I think you can assume that sComposeMsgsBundle has been initialized by the time your code runs. > + if (sigIndex > 0) > + if (repIndex > 0) > + if (fwdIndex > 0) I think these should be > -1 > + let subject = GetMsgSubjectElement().value; > + if (subject && (gSubjectChanged || Wed Apr 16 2014 01:19:06 Error: ReferenceError: gSubjectChanged is not defined Source file: chrome://messenger/content/messengercompose/MsgComposeCommands.js Line: 1020 > + (gEditingDraft && > + (gComposeType == nsIMsgCompType.New || Following four lines need to be indented one space more. > + gComposeType == nsIMsgCompType.NewsPost || > + gComposeType == nsIMsgCompType.Draft || > + gComposeType == nsIMsgCompType.Template || > + gComposeType == nsIMsgCompType.MailToUrl)))) > + if (!async) > + return GetAttachmentKeywords(mailData, keywordsInCsv).length != 0; return !!GetAttachmentKeywords(mailData, keywordsInCsv); or return GetAttachmentKeywords(mailData, keywordsInCsv) > 0; > + document.addEventListener("keypress", awDocumentKeyPress, true); Why do we need this? > + init: function gAN_init(aDocument) { You don't need the "gAN_init" decoration bits any more. The current JS debugger knows the name of the methods/properties. > + GetMsgSubjectElement().addEventListener("input", > + this.subjectObserver, true); GetMsgSubjectElement().addEventListener("input", this, true); > + subjectObserver: function handleEvent() { handleEvent: function (event) { > + gAttachmentNotifier.timer.cancel(); > + gAttachmentNotifier.timer.initWithCallback(gAttachmentNotifier.event, 500, gAttachmentNotifier.timer.initWithCallback(gAttachmentNotifier, 500, > + Components.interfaces.nsITimer.TYPE_ONE_SHOT); > + }, > + event: { > + notify: function(timer) No need for the event: wrapper. Just use: notify: function(timer) { > + timer: Components.classes["@mozilla.org/timer;1"] > + .createInstance(Components.interfaces.nsITimer) I think this creates a new instance each time you call it. You should do something like: http://mxr.mozilla.org/comm-central/source/mail/components/search/content/searchCommon.js?rev=0b0ff12d6f09#163 > + shutdown: function gAN_shutdown() { > + if (this._obs) > + this._obs.disconnect(); > + gAttachmentNotifier.timer.cancel(); this.timer.cancel(); delete this.timer; > +<hbox> > + <notificationbox id="attachmentNotificationBox" flex="1" /> > +</hbox> This is wrong. a notificationbox is designed to wrap a browser or an iframe or some content. Also note that TB puts their notificationbox at the bottom becase their attachment pane is at the bottom. Since ours is at the top... See example: http://mxr.mozilla.org/comm-central/source/suite/mailnews/messenger.xul?rev=9a838a1dd341#234 So <vbox id="appcontent" flex="1"> <notificationbox id="attachmentNotificationBox"> <editor type="content-primary" id="content-frame" src="about:blank" name="browser.message.body" flex="1" ondblclick="EditorDblClick(event);" context="contentAreaContextMenu"/> </notificationbox> </vbox> Also please use class="browser-notificationbox" <notificationbox id="composeNotificationBox" class="browser-notificationbox" flex="1"> > diff --git a/suite/themes/classic/messenger/icons/compose-toolbar.png b/suite/themes/classic/messenger/icons/compose-toolbar.png Please DON'T copy the whole toolbar.png when you only use one button from there. > +#attachmentNotificationBox > notification > .notification-inner { Just .notification-inner should work. > + border: none; > + border-top: 1px solid ThreeDShadow; > + border-bottom: 1px solid ThreeDShadow; Why do we need these border styles? > +#attachmentNotificationBox > notification .messageImage { Use: .messageImage[value="attachmentReminder"] { > + width: 22px; > + height: 20px; Why these dimensions? > + background-image: url(chrome://messenger/skin/icons/compose-toolbar.png); This is XUL so use list-style-image: and (if necessary -moz-image-region:) > + background-position: top 33% left; FYI: This doesn't match the compose-toolbar.png(s) you copied in.

Cool, many of those cleanups would be useful for TB too.

> or > return GetAttachmentKeywords(mailData, keywordsInCsv) > 0; Ooops. I meant: return GetAttachmentKeywords(mailData, keywordsInCsv).length > 0; Sorry about that!

Okay, so removed the compose-toolbar.png. Will put another small image for the attachment. Stripped the prefs from the patch as per review comment. Thanks.

Comment on attachment 8410354 [details] [diff] [review] Patch v1 > + <notificationbox id="composeNotificationBox" class="browser-notificationbox" flex="1"> In new and changed code we prefer one attribute per line like: <notificationbox id="composeNotificationBox" class="browser-notificationbox" flex="1"> > +function getNotificationBox(aNotificationBoxId) > +{ > + return document.getElementById(aNotificationBoxId); > +} Perhaps my instructions were not clear. I meant a function like: function getNotificationBox() { return document.getElementById("composeNotificationBox"); } > + let activeElement = document.activeElement; > + let notification = getNotificationBox("composeNotificationBox") The SeaMonkey module owners prefer "var" to "let" except in subscopes where "let" makes sense. > + <key keycode="VK_ESCAPE" oncommand="handleEsc();"/> > +function handleEsc() Move this to gAttachmentNotifier.init() since you're already adding an event listener there: > + GetMsgSubjectElement().addEventListener("input", If you add the event listener to the notificationbox you should be able to listen to keypresses from the message body and the notification. > +{ > + // If there is an attachment reminder notification AND focus is in message > + // body or on the notification, hide it. > + let activeElement = document.activeElement; > + let notification = getNotificationBox("composeNotificationBox") > + .getNotificationWithValue("attachmentReminder"); > + if (notification && (activeElement.id == "content-frame" || > + notification.contains(activeElement) || > + activeElement.classList.contains("messageCloseButton"))) { Why is "messageCloseButton" needed? > + notification.close(); We can optimize this slightly by returning early if there is no notification: var notification = getNotificationBox().getNotificationWithValue("attachmentReminder"); if (!notification) return; var activeElement = document.activeElement; if (activeElement.id == "content-frame" || notification.contains(activeElement) || activeElement.classList.contains("messageCloseButton")) { notification.close(); } } > + msg.onclick = function(event) This should be a button in the button array passed to appendNotification(). The SeaMonkey Preferences window is different. > + openOptionsDialog("paneCompose", "generalTab", Use goPreferences(paneID) > + {subdialog: "attachment_reminder_button"}); We don't do subdialogs. > + if (!sComposeMsgsBundle) > + sComposeMsgsBundle = document.getElementById("bundle_composeMsgs"); Remove these two lines. > + * Determine whether we should show the attachment notification or not. > + * > + * @param async Whether we should run the regex checker asynchronously or not. > + * @return true if we should show the attachment notification > + */ > +function ShouldShowAttachmentNotification(async) There is only one caller and the parameter is always true. > + let keywordsInCsv = Services.prefs.getComplexValue( > + "mail.compose.attachment_reminder_keywords", > + Components.interfaces.nsIPrefLocalizedString).data; In SeaMonkey you can use: function GetLocalizedStringPref(aPrefName, aDefaultValue) > + let spans = mailBodyNode.querySelectorAll("span[_moz_quote]"); > + for (let i = spans.length - 1; i >= 0; i--) { > + spans[i].remove(); > + } var spans = mailBodyNode.querySelectorAll("span[_moz_quote]"); for (var span of spans) span.remove(); > + // Ignore signature (html compose mode). > + let sigs = mailBodyNode.getElementsByClassName("moz-signature"); > + for (let i = sigs.length - 1; i >= 0; i--) { > + sigs[i].remove(); var sigs = mailBodyNode.querySelectorAll(".moz-signature"); for (var sig of sigs) sig.remove(); > + // Assuming that sComposeMsgsBundle is initialized by now. remove unnecessary comment. > +function CheckForAttachmentNotification(event) > +{ > + if (!CheckForAttachmentNotification.shouldFire || gManualAttachmentReminder) if (!this.shouldFire || gManualAttachmentReminder) > + if (gManualAttachmentReminder || (getPref("mail.compose.attachment_reminder_aggressive") && > + getNotificationBox("composeNotificationBox").getNotificationWithValue("attachmentReminder"))) { Looks confusing. How about: var notification = getNotificationBox().getNotificationWithValue("attachmentReminder"); if (gManualAttachmentReminder || getPref("mail.compose.attachment_reminder_aggressive") && notification) { > + sComposeMsgsBundle.getString("attachmentReminderTitle"), > + sComposeMsgsBundle.getString("attachmentReminderMsg"), > + flags, > + sComposeMsgsBundle.getString("attachmentReminderFalseAlarm"), > + sComposeMsgsBundle.getString("attachmentReminderYesIForgot"), > + null, null, {value:0}); Wrong indentation. > +function toggleAttachmentReminder(aState = !gManualAttachmentReminder) Shouldn't there be a line like: toggleAttachmentReminder(gMsgCompose.compFields.attachmentReminder); somewhere in ComposeStartup()? > + this._obs = new MutationObserver(function gAN_handleMutations(aMutations) { > + gAttachmentNotifier.timer.cancel(); > + gAttachmentNotifier.timer.initWithCallback(gAttachmentNotifier.notify, 500, > + Components.interfaces.nsITimer.TYPE_ONE_SHOT); > + }); We can avoid having to use "gAttachmentNotifier" instead of "this". Option 1: (fat arrow syntax) this._obs = new MutationObserver(()=> { this.timer.cancel(); this.timer.initWithCallback(this.notify, 500, Components.interfaces.nsITimer.TYPE_ONE_SHOT); }); Option 2: (using bind()) this._obs = new MutationObserver(function(aMutations) { this.timer.cancel(); this.timer.initWithCallback(this.notify, 500, Components.interfaces.nsITimer.TYPE_ONE_SHOT); }.bind(this)); > + this._obs.observe(aDocument, { > + attributes: true, What attributes do you expect to change? > + shutdown: function() { .... > + delete this.timer; I don't think we need to delete the "this.timer" > + GetMsgSubjectElement().addEventListener("input", > + this.subjectObserver, true); Just pass "this" as the callback. This will work if "this" has a method called handleEvent(). The advantage of passing the whole gAttachmentNotifier as the callback is that "this" is the right "this" and we don't need to refer to the global gAttachmentNotifier. GetMsgSubjectElement().addEventListener("input", this, true); > + subjectObserver: function() { > + gAttachmentNotifier.timer.cancel(); > + gAttachmentNotifier.timer.initWithCallback( > + gAttachmentNotifier.notify, 500, Components.interfaces.nsITimer.TYPE_ONE_SHOT); > + }, Replace subjectObserver with handleEvent . handleEvent: function(aEvent) { this.timer.cancel(); this.timer.initWithCallback( this, 500, Components.interfaces.nsITimer.TYPE_ONE_SHOT); } > + gAttachmentNotifier.init(editor.document); You might want to call gAttachmentNotifier.shutdown() when the compose window closes. > +function updateAttachmentCheck() > +{ > + document.getElementById("attachment_reminder_button").disabled = > + !document.getElementById("attachment_reminder_label").checked; > +} All the preference changes should go into a separate bug or at least a separate patch. > +/* ::::: attachment reminder ::::: */ > +.notification-inner { > + border: none; > + border-top: 1px solid ThreeDShadow; > + border-bottom: 1px solid ThreeDShadow; Why do we need these three styles? > +} > + > +#attachmentReminderText { > + -moz-margin-start: 0px; Why do we need -moz-margin-start? > +#attachmentKeywords { > + font-weight: bold; > + -moz-margin-start: 0px; Why do we need -moz-margin-start? > + text-decoration: underline; > + cursor: pointer; > +} > + > +function toggleAttachmentReminder(aState = !gManualAttachmentReminder) > +{ > + gManualAttachmentReminder = aState; > + let nBox = getNotificationBox("composeNotificationBox"); > + let notification = nBox.getNotificationWithValue("attachmentReminder"); > + if (aState && notification) > + nBox.removeNotification(notification); You seem to have missed out all the changes in messengercompose.xul relating to Bug 521158: http://hg.mozilla.org/comm-central/rev/42d26678e7e6#l2.1 Shouldn't there be a check like this CheckForAttachmentNotification(null); at the end of AddAttachment(), RemoveAllAttachments(), RemoveSelectedAttachment() We should port this bit: http://hg.mozilla.org/comm-central/rev/d2c11a99b1e1#l1.48 attachmentChecker.js: > // Hiragana, Katakana and Kanaji > // Hiragana, Katakana and Kanaji s/Kanaji/Kanji/g > * Get the (possibly-empty) list of attachment keywords in this message. > * > * @return the (possibly-empty) list of attachment keywords in this message @return the list of keywords > function GetAttachmentKeywords(mailData,keywordsInCsv) Space after comma. > var keywordsArray = (keywordsInCsv) ? keywordsInCsv.split(",") : []; No need for brackets around keywordsInCsv. > Okay, so removed the compose-toolbar.png. Will put another small image for > the attachment. Stripped the prefs from the patch as per review comment. You can use the following for the time being: http://mxr.mozilla.org/comm-central/source/mail/themes/windows/mail/preferences/attachments.png

Many of the changes could be done in TB too (I'd suggest doing them there first and then copy the whole thing to SM again). But do you guys really want to differ on stuff like var/let? That may deter devs to port improvements to the notification from TB to SM in the future (which are already in the queue). If they even notice SM has the feature too (maybe a comment in the TB file would help).

(In reply to :aceman from comment #10) > Many of the changes could be done in TB too (I'd suggest doing them there > first and then copy the whole thing to SM again). But do you guys really > want to differ on stuff like var/let? That may deter devs to port > improvements to the notification from TB to SM in the future (which are > already in the queue). If they even notice SM has the feature too (maybe a > comment in the TB file would help). You have a point. However it's up to Mnyromyr and Neil to make the final decision. I guess Suyash can stick to let for the time being.