Bug 742538 - Removing Filelink URLs from a message body should not cause subsequent URL insertions to fail. r=bienvenu.
--- a/mail/components/compose/content/cloudAttachmentLinkManager.js
+++ b/mail/components/compose/content/cloudAttachmentLinkManager.js
@@ -73,25 +73,30 @@ var gCloudAttachmentLinkManager = {
let attachment = event.target.attachment;
let provider = event.target.cloudProvider;
this.cloudAttachments.push(attachment);
this._insertItem(mailDoc, attachment, provider);
}
else if (event.type == "attachments-removed" ||
event.type == "attachments-converted") {
- let list = mailDoc.getElementById("cloudAttachmentList");
- let items = list.getElementsByClassName("cloudAttachmentItem");
+
+ let list, items;
+ list = mailDoc.getElementById("cloudAttachmentList");
+
+ if (list)
+ items = list.getElementsByClassName("cloudAttachmentItem");
for (let attachment in fixIterator(
event.detail, Components.interfaces.nsIMsgAttachment)) {
// Remove the attachment from the message body.
- for (let i = 0; i < items.length; i++)
- if (items[i].contentLocation == attachment.contentLocation)
- list.removeChild(items[i]);
+ if (list && items)
+ for (let i = 0; i < items.length; i++)
+ if (items[i].contentLocation == attachment.contentLocation)
+ list.removeChild(items[i]);
// Now, remove the attachment from our internal list.
let index = this.cloudAttachments.indexOf(attachment);
if (index != -1)
this.cloudAttachments.splice(index, 1);
}
this._updateAttachmentCount(mailDoc);
@@ -246,22 +251,44 @@ var gCloudAttachmentLinkManager = {
if (nodeIndex <= 0) {
editor.insertLineBreak();
nodeIndex = 1;
}
selection.collapse(mailBody, nodeIndex);
},
/**
+ * Attempts to find any elements with an id in aIDs, and sets those elements
+ * id attribute to the empty string, freeing up the ids for later use.
+ *
+ * @param aDocument the document to search for the elements.
+ * @param aIDs an array of id strings.
+ */
+ _resetNodeIDs: function(aDocument, aIDs) {
+ for each (let [, id] in Iterator(aIDs)) {
+ let node = aDocument.getElementById(id);
+ if (node)
+ node.id = "";
+ }
+ },
+
+ /**
* Insert the header for the cloud attachment list, which we'll use to
* as an insertion point for the individual cloud attachments.
*
- * @param aDocument the document to insert the header into
+ * @param aDocument the document to insert the header into.
*/
_insertHeader: function(aDocument) {
+ // If there already exists a cloudAttachmentListRoot,
+ // cloudAttachmentListHeader or cloudAttachmentList in the document,
+ // strip them of their IDs so that we don't conflict with them.
+ this._resetNodeIDs(aDocument, ["cloudAttachmentListRoot",
+ "cloudAttachmentListHeader",
+ "cloudAttachmentList"]);
+
let brandBundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
let editor = GetCurrentEditor();
let selection = editor.selection;
// Save off the selection ranges so we can restore them later.
let ranges = [];
for (let i = 0; i < selection.rangeCount; i++)
ranges.push(selection.getRangeAt(i));
@@ -352,16 +379,22 @@ var gCloudAttachmentLinkManager = {
* Insert the information for a cloud attachment.
*
* @param aDocument the document to insert the item into
* @param aAttachment the nsIMsgAttachment to insert
* @param aProviderType the cloud storage provider
*/
_insertItem: function(aDocument, aAttachment, aProvider) {
let list = aDocument.getElementById("cloudAttachmentList");
+
+ if (!list) {
+ this._insertHeader(aDocument);
+ list = aDocument.getElementById("cloudAttachmentList");
+ }
+
let node = aDocument.createElement("div");
node.className = "cloudAttachmentItem";
node.contentLocation = aAttachment.contentLocation;
if (gMsgCompose.composeHTML) {
node.style.border = "1px solid #CDCDCD";
node.style.borderRadius = "5px";
node.style.marginTop = "10px";
--- a/mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
+++ b/mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
@@ -203,22 +203,31 @@ function try_with_and_without_signature_
* in HTML mode, and again in plaintext mode.
*
* @param aTest a test that takes no arguments.
*/
function try_without_signature(aTest) {
let oldSig = Services.prefs.getCharPref(kSigPrefKey);
Services.prefs.setCharPref(kSigPrefKey, "");
+ try_with_plaintext_and_html_mail(aTest);
+ Services.prefs.setCharPref(kSigPrefKey, oldSig);
+}
+
+/**
+ * Helper function that runs a test function for HTML mail composition, and
+ * then again in plaintext mail composition.
+ *
+ * @param aTest a test that takes no arguments.
+ */
+function try_with_plaintext_and_html_mail(aTest) {
aTest();
Services.prefs.setBoolPref(kHtmlPrefKey, false);
aTest();
Services.prefs.setBoolPref(kHtmlPrefKey, true);
-
- Services.prefs.setCharPref(kSigPrefKey, oldSig);
}
/**
* Test that if we open up a composer and immediately attach a Filelink,
* a linebreak is inserted before the containment node in order to allow
* the user to write before the attachment URLs. This assumes the user
* does not have a signature already inserted into the message body.
*/
@@ -315,20 +324,17 @@ function test_inserts_linebreak_on_empty
Services.prefs.setBoolPref(kHtmlPrefKey, true);
}
/**
* Tests that removing all Filelinks causes the root node to be removed.
*/
function test_removing_filelinks_removes_root_node() {
- subtest_removing_filelinks_removes_root_node();
- Services.prefs.setBoolPref(kHtmlPrefKey, false);
- subtest_removing_filelinks_removes_root_node();
- Services.prefs.setBoolPref(kHtmlPrefKey, true);
+ try_with_plaintext_and_html_mail(subtest_removing_filelinks_removes_root_node);
}
/**
* Test for test_removing_filelinks_removes_root_node - can be executed
* on both plaintext and HTML compose windows.
*/
function subtest_removing_filelinks_removes_root_node() {
let cw = prepare_some_attachments_and_reply([], kFiles);
@@ -664,20 +670,17 @@ function subtest_adding_filelinks_to_for
}
/**
* Test that if we convert a Filelink from one provider to another, that the
* old Filelink is removed, and a new Filelink is added for the new provider.
* We test this on both HTML and plaintext mail.
*/
function test_converting_filelink_updates_urls() {
- subtest_converting_filelink_updates_urls();
- Services.prefs.setBoolPref(kHtmlPrefKey, false);
- subtest_converting_filelink_updates_urls();
- Services.prefs.setBoolPref(kHtmlPrefKey, true);
+ try_with_plaintext_and_html_mail(subtest_converting_filelink_updates_urls);
}
/**
* Subtest for test_converting_filelink_updates_urls that creates two
* storage provider accounts, uploads files to one, converts them to the
* other, and ensures that the attachment links in the message body get
* get updated.
*/
@@ -707,20 +710,18 @@ function subtest_converting_filelink_upd
}
}
/**
* Test that if we convert a Filelink to a normal attachment that the
* Filelink is removed from the message body.
*/
function test_converting_filelink_to_normal_removes_url() {
- subtest_converting_filelink_to_normal_removes_url();
- Services.prefs.setBoolPref(kHtmlPrefKey, false);
- subtest_converting_filelink_to_normal_removes_url();
- Services.prefs.setBoolPref(kHtmlPrefKey, true);
+ try_with_plaintext_and_html_mail(
+ subtest_converting_filelink_to_normal_removes_url);
}
/**
* Subtest for test_converting_filelink_to_normal_removes_url that adds
* some Filelinks to an email, and then converts those Filelinks back into
* normal attachments, checking to ensure that the links are removed from
* the body of the email.
*/
@@ -743,8 +744,42 @@ function subtest_converting_filelink_to_
}
// At this point, the root should also have been removed.
let mailBody = get_compose_body(cw);
root = mailBody.querySelector("#cloudAttachmentListRoot");
if (root)
throw new Error("Should not have found the cloudAttachmentListRoot");
}
+
+/**
+ * Tests that if the user manually removes the Filelinks from the message body
+ * that it doesn't break future Filelink insertions.
+ */
+function test_filelinks_work_after_manual_removal() {
+ try_with_plaintext_and_html_mail(subtest_filelinks_work_after_manual_removal);
+}
+
+/**
+ * Subtest that first adds some Filelinks to the message body, removes them,
+ * and then adds another Filelink ensuring that the new URL is successfully
+ * inserted.
+ */
+function subtest_filelinks_work_after_manual_removal() {
+ // Insert some Filelinks...
+ gMockFilePicker.returnFiles = collectFiles(kFiles, __file__);
+ let provider = new MockCloudfileAccount();
+ provider.init("someKey");
+ let cw = open_compose_new_mail();
+ cw.window.attachToCloud(provider);
+
+ let [root, list, urls] = wait_for_attachment_urls(cw, kFiles.length);
+
+ // Now remove the root node from the document body
+ let mailBody = get_compose_body(cw);
+ mailBody.removeChild(root);
+
+ gMockFilePicker.returnFiles = collectFiles(["./data/testFile3"], __file__);
+ cw.window.attachToCloud(provider);
+ [root, list, urls] = wait_for_attachment_urls(cw, 1);
+}
+
+