Bug 1231902 - Fix nsMsgCompose::SetIdentity() to replace signature better. Test signature switch in paragraph format. r=aceman.
authorJorg K
Sun, 14 Feb 2016 23:03:52 +0100
changeset 18796 bd1684a179caaf03f218e5ecb5aaec6ac27bb5af
parent 18795 4ca276dff98aaaa672dc8c76baeb1c2b27e02158
child 18797 f46004b3e9a6b6ac9f73a3a9897b277caff6ef29
push id1623
push userclokep@gmail.com
push dateMon, 25 Apr 2016 14:53:28 +0000
treeherdercomm-esr52@2b8c5494c89f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaceman
bugs1231902
Bug 1231902 - Fix nsMsgCompose::SetIdentity() to replace signature better. Test signature switch in paragraph format. r=aceman.
mail/test/mozmill/composition/test-signature-updating.js
mailnews/compose/src/nsMsgCompose.cpp
mailnews/compose/src/nsMsgCompose.h
--- a/mail/test/mozmill/composition/test-signature-updating.js
+++ b/mail/test/mozmill/composition/test-signature-updating.js
@@ -35,44 +35,42 @@ var setupModule = function (module) {
   let wh = collector.getModule("window-helpers");
   wh.installInto(module);
 
   // Ensure we're in the tinderbox account as that has the right identities set
   // up for this test.
   let server = MailServices.accounts.FindServer("tinderbox", FAKE_SERVER_HOSTNAME, "pop3");
   let inbox = server.rootFolder.getChildNamed("Inbox");
   be_in_folder(inbox);
-
-  // Don't create paragraphs in the test.
-  // The test checks for the first DOM node and expects a text and not
-  // a paragraph.
-  Services.prefs.setBoolPref("editor.CR_creates_new_p", false);
 };
 
 function teardownModule(module) {
   Services.prefs.clearUserPref("editor.CR_creates_new_p");
+  Services.prefs.clearUserPref("mail.identity.id1.compose_html");
+  Services.prefs.clearUserPref("mail.identity.id1.suppress_signature_separator");
+  Services.prefs.clearUserPref("mail.identity.id2.suppress_signature_separator");
 }
 
 function setupComposeWin(toAddr, subj, body) {
   cwc.type(null, toAddr);
   cwc.type(cwc.eid("msgSubject"), subj);
   cwc.type(cwc.eid("content-frame"), body);
 }
 
 /**
  * Test that the plaintext compose window has a signature initially,
  * and has the correct signature after switching to another identity.
  */
 function plaintextComposeWindowSwitchSignatures(suppressSigSep) {
   Services.prefs.setBoolPref("mail.identity.id1.compose_html", false);
-  cwc = composeHelper.open_compose_new_mail();
   Services.prefs.setBoolPref("mail.identity.id1.suppress_signature_separator",
                              suppressSigSep);
   Services.prefs.setBoolPref("mail.identity.id2.suppress_signature_separator",
                              suppressSigSep);
+  cwc = composeHelper.open_compose_new_mail();
 
   let contentFrame = cwc.e("content-frame");
   let mailBody = contentFrame.contentDocument.body;
 
   // The first node in the body should be a BR node, which allows the user
   // to insert text before / outside of the signature.
   assert_equals(mailBody.firstChild.localName, "br");
 
@@ -146,26 +144,26 @@ function plaintextComposeWindowSwitchSig
 
   composeHelper.close_compose_window(cwc);
 }
 
 function testPlaintextComposeWindowSwitchSignatures() {
   plaintextComposeWindowSwitchSignatures(false);
 }
 
-// XXX Disabled due to not correctly switching signatures with no separator
-// See bug TBD
-//function testPlaintextComposeWindowSwitchSignaturesWithSuppressedSeparator() {
-//  plaintextComposeWindowSwitchSignatures(true);
-//}
+function testPlaintextComposeWindowSwitchSignaturesWithSuppressedSeparator() {
+  plaintextComposeWindowSwitchSignatures(true);
+}
 
 /**
  * Same test, but with an HTML compose window
  */
-function HTMLComposeWindowSwitchSignatures(suppressSigSep) {
+function HTMLComposeWindowSwitchSignatures(suppressSigSep, paragraphFormat) {
+  Services.prefs.setBoolPref("editor.CR_creates_new_p", paragraphFormat);
+
   Services.prefs.setBoolPref("mail.identity.id1.compose_html", true);
   Services.prefs.setBoolPref("mail.identity.id1.suppress_signature_separator",
                              suppressSigSep);
   Services.prefs.setBoolPref("mail.identity.id2.suppress_signature_separator",
                              suppressSigSep);
   cwc = composeHelper.open_compose_new_mail();
 
   setupComposeWin("", "HTML compose window", "Body, first line.");
@@ -201,27 +199,41 @@ function HTMLComposeWindowSwitchSignatur
   node = node.nextSibling;
   assert_equals(node.localName, "b");
   node = node.firstChild;
   assert_equals(node.nodeValue, "hotness!");
 
   // Now check that the original signature has been removed,
   // and no blank lines got added!
   node = contentFrame.contentDocument.body.firstChild;
-  assert_equals(node.nodeValue, "Body, first line.");
+  let textNode;
+  if (paragraphFormat) {
+    textNode = node.firstChild;
+  } else {
+    textNode = node;
+  }
+  assert_equals(textNode.nodeValue, "Body, first line.");
   node = node.nextSibling;
   assert_equals(node.localName, "br");
   node = node.nextSibling;
   // check that the signature is immediately after the message text.
   assert_equals(node.className, "moz-signature");
   // check that that the signature is the last node.
   assert_equals(node, contentFrame.contentDocument.body.lastChild);
 
   composeHelper.close_compose_window(cwc);
 }
 
 function testHTMLComposeWindowSwitchSignatures() {
-  HTMLComposeWindowSwitchSignatures(false);
+  HTMLComposeWindowSwitchSignatures(false, false);
 }
 
 function testHTMLComposeWindowSwitchSignaturesWithSuppressedSeparator() {
-  HTMLComposeWindowSwitchSignatures(true);
+  HTMLComposeWindowSwitchSignatures(true, false);
 }
+
+function testHTMLComposeWindowSwitchSignaturesParagraphFormat() {
+  HTMLComposeWindowSwitchSignatures(false, true);
+}
+
+function testHTMLComposeWindowSwitchSignaturesWithSuppressedSeparatorParagraphFormat() {
+  HTMLComposeWindowSwitchSignatures(true, true);
+}
--- a/mailnews/compose/src/nsMsgCompose.cpp
+++ b/mailnews/compose/src/nsMsgCompose.cpp
@@ -523,17 +523,16 @@ nsMsgCompose::InsertDivWrappedTextAtSele
                                               const nsAString &classStr)
 {
   NS_ASSERTION(m_editor, "InsertDivWrappedTextAtSelection called, but no editor exists\n");
   if (!m_editor)
     return;
 
   nsCOMPtr<nsIDOMElement> divElem;
   nsCOMPtr<nsIHTMLEditor> htmlEditor(do_QueryInterface(m_editor));
-  nsCOMPtr<nsIPlaintextEditor> textEditor(do_QueryInterface(m_editor));
 
   nsresult rv = htmlEditor->CreateElementWithDefaults(NS_LITERAL_STRING("div"),
                                                       getter_AddRefs(divElem));
 
   NS_ENSURE_SUCCESS_VOID(rv);
 
   nsCOMPtr<nsIDOMNode> divNode (do_QueryInterface(divElem));
 
@@ -4423,22 +4422,22 @@ nsMsgCompose::BuildBodyMessageAndSignatu
   // Now, we have the body so we can just blast it into the
   // composition editor window.
   //
   nsAutoString   body;
   m_compFields->GetBody(body);
 
   /* Some time we want to add a signature and sometime we wont. Let's figure that now...*/
   bool addSignature;
-  bool addDashes = false;
+  bool isQuoted = false;
   switch (mType)
   {
     case nsIMsgCompType::ForwardInline :
       addSignature = true;
-      addDashes = true;
+      isQuoted = true;
       break;
     case nsIMsgCompType::New :
     case nsIMsgCompType::MailToUrl :    /* same as New */
     case nsIMsgCompType::Reply :        /* should not happen! but just in case */
     case nsIMsgCompType::ReplyAll :       /* should not happen! but just in case */
     case nsIMsgCompType::ReplyToList :    /* should not happen! but just in case */
     case nsIMsgCompType::ForwardAsAttachment :  /* should not happen! but just in case */
     case nsIMsgCompType::NewsPost :
@@ -4456,17 +4455,17 @@ nsMsgCompose::BuildBodyMessageAndSignatu
 
     default :
       addSignature = false;
       break;
   }
 
   nsAutoString tSignature;
   if (addSignature)
-    ProcessSignature(m_identity, addDashes, &tSignature);
+    ProcessSignature(m_identity, isQuoted, &tSignature);
 
   // if type is new, but we have body, this is probably a mapi send, so we need to
   // replace '\n' with <br> so that the line breaks won't be lost by html.
   // if mailtourl, do the same.
   if (m_composeHTML && (mType == nsIMsgCompType::New || mType == nsIMsgCompType::MailToUrl))
     MsgReplaceSubstring(body, NS_LITERAL_STRING("\n"), NS_LITERAL_STRING("<br>"));
 
   // Restore flowed text wrapping for Drafts/Templates.
@@ -5376,41 +5375,149 @@ nsMsgCompose::BodyConvertible(int32_t *_
 NS_IMETHODIMP
 nsMsgCompose::GetIdentity(nsIMsgIdentity **aIdentity)
 {
   NS_ENSURE_ARG_POINTER(aIdentity);
   NS_IF_ADDREF(*aIdentity = m_identity);
   return NS_OK;
 }
 
+/**
+ * Position above the quote, that is either <blockquote> or
+ * <div class="moz-cite-prefix"> or <div class="moz-forward-container">
+ * in an inline-forwarded message.
+ */
+nsresult
+nsMsgCompose::MoveToAboveQuote(void)
+{
+  nsCOMPtr<nsIDOMElement> rootElement;
+  nsresult rv = m_editor->GetRootElement(getter_AddRefs(rootElement));
+  if (NS_FAILED(rv) || !rootElement) {
+    return rv;
+  }
+
+  nsCOMPtr<nsIDOMNode> node;
+  nsAutoString attributeName;
+  nsAutoString attributeValue;
+  nsAutoString tagLocalName;
+  attributeName.AssignLiteral("class");
+
+  rv = rootElement->GetFirstChild(getter_AddRefs(node));
+  while (NS_SUCCEEDED(rv) && node) {
+    nsCOMPtr<nsIDOMElement> element = do_QueryInterface(node);
+    if (element) {
+      // First check for <blockquote>. This will most likely not trigger
+      // since well-behaved quotes are preceded by a cite prefix.
+      node->GetLocalName(tagLocalName);
+      if (tagLocalName.EqualsLiteral("blockquote")) {
+        break;
+      }
+
+      // Get the class value.
+      element->GetAttribute(attributeName, attributeValue);
+
+      // Now check for the cite prefix, so an element with
+      // class="moz-cite-prefix".
+      if (attributeValue.Find("moz-cite-prefix", true) != kNotFound) {
+        break;
+      }
+
+      // Next check for forwarded content.
+      // The forwarded part is inside an element with
+      // class="moz-forward-container".
+      if (attributeValue.Find("moz-forward-container", true) != kNotFound) {
+        break;
+      }
+    }
+
+    rv = node->GetNextSibling(getter_AddRefs(node));
+    if (NS_FAILED(rv) || !node) {
+      // No further siblings found, so we didn't find what we were looking for.
+      rv = NS_OK;
+      node = nullptr;
+      break;
+    }
+  }
+
+  // Now position. If no quote was found, we position to the very front.
+  int32_t offset = 0;
+  if (node) {
+    rv = GetChildOffset(node, rootElement, offset);
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
+  }
+  nsCOMPtr<nsISelection> selection;
+  m_editor->GetSelection(getter_AddRefs(selection));
+  if (selection)
+    rv = selection->Collapse(rootElement, offset);
+
+  return rv;
+}
+
+/**
+ * M-C's nsEditor::EndOfDocument() will position to the end of the document
+ * but it will position into a container. We really need to position
+ * after the last container so we don't accidentally position into a
+ * <blockquote>. That's why we use our own function.
+ */
+nsresult
+nsMsgCompose::MoveToEndOfDocument(void)
+{
+  int32_t offset;
+  nsCOMPtr<nsIDOMElement> rootElement;
+  nsCOMPtr<nsIDOMNode> lastNode;
+  nsresult rv = m_editor->GetRootElement(getter_AddRefs(rootElement));
+  if (NS_FAILED(rv) || !rootElement) {
+    return rv;
+  }
+
+  rv = rootElement->GetLastChild(getter_AddRefs(lastNode));
+  if (NS_FAILED(rv) || !lastNode) {
+    return rv;
+  }
+
+  rv = GetChildOffset(lastNode, rootElement, offset);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  nsCOMPtr<nsISelection> selection;
+  m_editor->GetSelection(getter_AddRefs(selection));
+  if (selection)
+    rv = selection->Collapse(rootElement, offset + 1);
+
+  return rv;
+}
+
 NS_IMETHODIMP
 nsMsgCompose::SetIdentity(nsIMsgIdentity *aIdentity)
 {
   NS_ENSURE_ARG_POINTER(aIdentity);
 
   m_identity = aIdentity;
 
   nsresult rv;
 
   if (! m_editor)
     return NS_ERROR_FAILURE;
 
   nsCOMPtr<nsIDOMElement> rootElement;
   rv = m_editor->GetRootElement(getter_AddRefs(rootElement));
-  if (NS_FAILED(rv) || nullptr == rootElement)
+  if (NS_FAILED(rv) || !rootElement)
     return rv;
 
   //First look for the current signature, if we have one
   nsCOMPtr<nsIDOMNode> lastNode;
   nsCOMPtr<nsIDOMNode> node;
   nsCOMPtr<nsIDOMNode> tempNode;
   nsAutoString tagLocalName;
 
   rv = rootElement->GetLastChild(getter_AddRefs(lastNode));
-  if (NS_SUCCEEDED(rv) && nullptr != lastNode)
+  if (NS_SUCCEEDED(rv) && lastNode)
   {
     node = lastNode;
     // In html, the signature is inside an element with
     // class="moz-signature"
     bool signatureFound = false;
     nsAutoString attributeName;
     attributeName.AssignLiteral("class");
 
@@ -5438,17 +5545,17 @@ nsMsgCompose::SetIdentity(nsIMsgIdentity
       node->GetPreviousSibling(getter_AddRefs(tempNode));
       rv = m_editor->DeleteNode(node);
       if (NS_FAILED(rv))
       {
         m_editor->EndTransaction();
         return rv;
       }
 
-      //Also, remove the <br> right before the signature.
+      // Also, remove the <br> right before the signature.
       if (tempNode)
       {
         tempNode->GetLocalName(tagLocalName);
         if (tagLocalName.EqualsLiteral("br"))
           m_editor->DeleteNode(tempNode);
       }
       m_editor->EndTransaction();
     }
@@ -5456,59 +5563,59 @@ nsMsgCompose::SetIdentity(nsIMsgIdentity
 
   if (!CheckIncludeSignaturePrefs(aIdentity))
     return NS_OK;
 
   // Then add the new one if needed
   nsAutoString aSignature;
 
   // No delimiter needed if not a compose window
-  bool noDelimiter;
+  bool isQuoted;
   switch (mType)
   {
     case nsIMsgCompType::New :
     case nsIMsgCompType::NewsPost :
     case nsIMsgCompType::MailToUrl :
     case nsIMsgCompType::ForwardAsAttachment :
-      noDelimiter = false;
+      isQuoted = false;
       break;
     default :
-      noDelimiter = true;
+      isQuoted = true;
       break;
   }
 
-  ProcessSignature(aIdentity, noDelimiter, &aSignature);
+  ProcessSignature(aIdentity, isQuoted, &aSignature);
 
   if (!aSignature.IsEmpty())
   {
     TranslateLineEnding(aSignature);
 
     m_editor->BeginTransaction();
     int32_t reply_on_top = 0;
     bool sig_bottom = true;
     aIdentity->GetReplyOnTop(&reply_on_top);
     aIdentity->GetSigBottom(&sig_bottom);
     bool sigOnTop = (reply_on_top == 1 && !sig_bottom);
-    if (sigOnTop && noDelimiter)
-      m_editor->BeginningOfDocument();
-    else
-      m_editor->EndOfDocument();
-
-    nsCOMPtr<nsIHTMLEditor> htmlEditor (do_QueryInterface(m_editor));
-    nsCOMPtr<nsIPlaintextEditor> textEditor (do_QueryInterface(m_editor));
-
-    if (m_composeHTML)
-      rv = htmlEditor->InsertHTML(aSignature);
-    else {
-      rv = textEditor->InsertLineBreak();
-      InsertDivWrappedTextAtSelection(aSignature, NS_LITERAL_STRING("moz-signature"));
+    if (sigOnTop && isQuoted) {
+      rv = MoveToAboveQuote();
+    } else {
+      // Note: New messages aren't quoted so we always move to the end.
+      rv = MoveToEndOfDocument();
     }
 
-    if (sigOnTop && noDelimiter)
-      m_editor->EndOfDocument();
+    if (NS_SUCCEEDED(rv)) {
+      if (m_composeHTML) {
+        nsCOMPtr<nsIHTMLEditor> htmlEditor (do_QueryInterface(m_editor));
+        rv = htmlEditor->InsertHTML(aSignature);
+      } else {
+        nsCOMPtr<nsIPlaintextEditor> textEditor (do_QueryInterface(m_editor));
+        rv = textEditor->InsertLineBreak();
+        InsertDivWrappedTextAtSelection(aSignature, NS_LITERAL_STRING("moz-signature"));
+      }
+    }
     m_editor->EndTransaction();
   }
 
   return rv;
 }
 
 NS_IMETHODIMP nsMsgCompose::CheckCharsetConversion(nsIMsgIdentity *identity, char **fallbackCharset, bool *_retval)
 {
--- a/mailnews/compose/src/nsMsgCompose.h
+++ b/mailnews/compose/src/nsMsgCompose.h
@@ -81,16 +81,18 @@ protected:
   nsresult CreateMessage(const char * originalMsgURI, MSG_ComposeType type, nsIMsgCompFields* compFields);
   void CleanUpRecipients(nsString& recipients);
   nsresult GetABDirectories(const nsACString& aDirUri,
                             nsCOMArray<nsIAbDirectory> &aDirArray);
   nsresult BuildMailListArray(nsIAbDirectory* parentDir,
                               nsTArray<nsMsgMailList>& array);
   nsresult TagConvertible(nsIDOMElement *node,  int32_t *_retval);
   nsresult _NodeTreeConvertible(nsIDOMElement *node, int32_t *_retval);
+  nsresult MoveToAboveQuote(void);
+  nsresult MoveToEndOfDocument(void);
 
 // 3 = To, Cc, Bcc
 #define MAX_OF_RECIPIENT_ARRAY 3
   typedef nsTArray<nsMsgRecipient> RecipientsArray[MAX_OF_RECIPIENT_ARRAY];
   /**
    * This method parses the compose fields and associates email addresses with
    * the relevant cards from the address books.
    */