Bug 746737 - Filelink URL insertion should not move the users selection point. r=bienvenu.
authorMike Conley <mconley@mozilla.com>
Fri, 20 Apr 2012 16:51:54 -0400
changeset 9966 a964c299bcec
parent 9965 b8a4823c1ca3
child 9967 ecdc43b5164e
push id7588
push usermconley@mozilla.com
push dateFri, 20 Apr 2012 20:52:19 +0000
treeherdercomm-central@a964c299bcec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbienvenu
bugs746737
Bug 746737 - Filelink URL insertion should not move the users selection point. r=bienvenu.
mail/components/compose/content/cloudAttachmentLinkManager.js
mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
mail/test/mozmill/shared-modules/test-compose-helpers.js
--- a/mail/components/compose/content/cloudAttachmentLinkManager.js
+++ b/mail/components/compose/content/cloudAttachmentLinkManager.js
@@ -282,16 +282,18 @@ var gCloudAttachmentLinkManager = {
     // 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;
+    let originalAnchor = selection.anchorNode;
+    let originalOffset = selection.anchorOffset;
 
     // 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));
 
     this._findInsertionPoint(aDocument);
 
@@ -348,16 +350,18 @@ var gCloudAttachmentLinkManager = {
 
       let list = editor.createElementWithDefaults("span");
       list.id = "cloudAttachmentList";
       root.appendChild(list);
 
       editor.insertElementAtSelection(root, false);
     }
 
+    selection.collapse(originalAnchor, originalOffset);
+
     // Restore the selection ranges.
     for (let [,range] in Iterator(ranges))
       selection.addRange(range);
   },
 
   /**
    * Updates the count of how many attachments have been added
    * in HTML emails.
--- a/mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
+++ b/mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
@@ -747,17 +747,18 @@ function subtest_converting_filelink_to_
   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.
+ * that it doesn't break future Filelink insertions. Tests both HTML and
+ * plaintext composers.
  */
 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
@@ -777,9 +778,48 @@ function subtest_filelinks_work_after_ma
   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);
 }
 
+/**
+ * Test that if the users selection caret is on a newline when the URL
+ * insertion occurs, that the caret does not move when the insertion is
+ * complete. Tests both HTML and plaintext composers.
+ */
+function test_insertion_restores_caret_point() {
+  try_with_plaintext_and_html_mail(subtest_insertion_restores_caret_point);
+}
 
+/**
+ * Subtest that types some things into the composer, finishes on two
+ * linebreaks, inserts some Filelink URLs, and then types some more,
+ * ensuring that the selection is where we expect it to be.
+ */
+function subtest_insertion_restores_caret_point() {
+  // Insert some Filelinks...
+  gMockFilePicker.returnFiles = collectFiles(kFiles, __file__);
+  let provider = new MockCloudfileAccount();
+  provider.init("someKey");
+
+  let cw = open_compose_new_mail();
+
+  // Put the selection at the beginning of the document...
+  let editor = cw.window.GetCurrentEditor();
+  editor.beginningOfDocument();
+
+  // Do any necessary typing, ending with two linebreaks.
+  type_in_composer(cw, ["Line 1", "Line 2", "", ""]);
+
+  // Attach some Filelinks.
+  cw.window.attachToCloud(provider);
+  let [root, list, urls] = wait_for_attachment_urls(cw, kFiles.length);
+
+  // Type some text.
+  const kTypedIn = "Test";
+  type_in_composer(cw, [kTypedIn]);
+
+  // That text should be inserted just above the root attachment URL node.
+  let textNode = assert_previous_text(root.previousSibling, [kTypedIn]);
+}
--- a/mail/test/mozmill/shared-modules/test-compose-helpers.js
+++ b/mail/test/mozmill/shared-modules/test-compose-helpers.js
@@ -428,20 +428,30 @@ function type_in_composer(aController, a
  * @param aStart the first node to check
  * @param aText an array of strings that should be checked for in reverse
  *              order (so the last element of the array should be the first
  *              text node encountered, the second last element of the array
  *              should be the next text node encountered, etc).
  */
 function assert_previous_text(aStart, aText) {
   let textNode = aStart;
-  for (let i = aText.length - 1; i > 0; --i) {
+  for (let i = aText.length - 1; i >= 0; --i) {
     if (textNode.nodeType != kTextNodeType)
-      throw new Error("Expected a text node");
+      throw new Error("Expected a text node! Node type was: " + textNode.nodeType);
 
     if (textNode.nodeValue != aText[i])
       throw new Error("Unexpected inequality - " + textNode.nodeValue + " != " +
                       + aText[i]);
-    let br = domHelper.assert_previous_nodes("br", textNode, 1);
-    textNode = br.previousSibling;
+
+    // We expect a BR preceding each text node automatically, except
+    // for the last one that we reach.
+    if (i > 0) {
+      let br = textNode.previousSibling;
+
+      if (br.localName != "br")
+        throw new Error("Expected a BR node - got a " + br.localName +
+                        "instead.");
+
+      textNode = br.previousSibling;
+    }
   }
   return textNode;
 }