fix(quoted-text): div vs blockquote, signature cleanup #1746

Summary:
Previously we always created <blockquote class="gmail_quote"> to wrap quoted text. This is not correct.
Gmail uses blockquotes only when it wants visual indentation, and <div>s to wrap other quoted text, like forwarded
messages which are not displayed indented.

This diff updates N1 to match Gmail exactly. Note that for replies, Gmail actually nests a blockquote.gmail_quote
inside a div.gmail_quote.

I also updated signature handling because it turns out the regexp that was removing existing signatures would blow
away any and all divs until it reached a <blockquote> tag.

Test Plan: See updated specs. Manually tested by creating a thread in Google Inbox and then performing fwd and reply in both N1 and Inbox. Results match.

Reviewers: juan, evan

Reviewed By: evan

Differential Revision: https://phab.nylas.com/D2750

internal_packages/composer-signature/lib/signature-composer-extension.es6

@@ -11,4 +11,14 @@ export default class SignatureComposerExtension extends ComposerExtension {
     }
     draft.body = SignatureUtils.applySignature(draft.body, signature);
   }
+
+  static finalizeSessionBeforeSending = ({session}) => {
+    // remove the <signature> element from the DOM,
+    // essentially unwraps the signature
+    const body = session.draft().body;
+    const changed = body.replace(/<\/?signature>/g, '');
+    if (body !== changed) {
+      session.changes.add({body: changed})
+    }
+  }
 }

internal_packages/composer-signature/lib/signature-utils.es6

@@ -1,24 +1,28 @@
 export default {
   applySignature(body, signature) {
     // https://regex101.com/r/nC0qL2/1
-    const signatureRegex = /<div class="nylas-n1-signature">[^]*<\/div>/;
+    const signatureRegex = /<signature>[^]*<\/signature>/;
 
-    let signatureHTML = '<div class="nylas-n1-signature">' + signature + '</div>';
-    let insertionPoint = body.search(signatureRegex);
     let newBody = body;
+    let paddingBefore = '';
+    let paddingAfter = '';
 
-    // If there is a signature already present
-    if (insertionPoint !== -1) {
-      // Remove it
-      newBody = newBody.replace(signatureRegex, "");
+    // Remove any existing signature in the body
+    newBody = newBody.replace(signatureRegex, "");
+
+    // http://www.regexpal.com/?fam=94390
+    // prefer to put the signature one <br> before the beginning of the quote,
+    // if possible.
+    let insertionPoint = newBody.search(/<\w+[^>]*gmail_quote/i);
+    if (insertionPoint === -1) {
+      insertionPoint = newBody.length;
+      paddingBefore = '<br/><br/>';
     } else {
-      insertionPoint = newBody.indexOf('<blockquote');
-
-      if (insertionPoint === -1) {
-        insertionPoint = newBody.length;
-        signatureHTML = '<br/><br/>' + signatureHTML;
-      }
+      paddingAfter = '<br/>';
     }
-    return newBody.slice(0, insertionPoint) + signatureHTML + newBody.slice(insertionPoint);
+
+    const contentBefore = newBody.slice(0, insertionPoint);
+    const contentAfter = newBody.slice(insertionPoint);
+    return `${contentBefore}<signature>${paddingBefore}${signature}${paddingAfter}</signature>${contentAfter}`;
   },
 };

internal_packages/composer-signature/spec/signature-composer-extension-spec.es6

@@ -2,19 +2,41 @@ import {Message} from 'nylas-exports';
 import SignatureComposerExtension from '../lib/signature-composer-extension';
 import SignatureStore from '../lib/signature-store';
 
+const TEST_SIGNATURE = '<div class="something">This is my signature.</div>';
+
 describe("SignatureComposerExtension", ()=> {
+  describe("finalizeSessionBeforeSending", ()=> {
+    it("should unwrap the signature and remove the custom DOM element", ()=> {
+      const a = new Message({
+        draft: true,
+        accountId: TEST_ACCOUNT_ID,
+        body: `This is a test! <signature>${TEST_SIGNATURE}<br/></signature><div class="gmail_quote">Hello world</div>`,
+      });
+      const add = jasmine.createSpy('changes.add');
+      const session = {
+        draft: ()=> a,
+        changes: {
+          add: add,
+        },
+      }
+      SignatureComposerExtension.finalizeSessionBeforeSending({session});
+      expect(add).toHaveBeenCalledWith({
+        body: `This is a test! ${TEST_SIGNATURE}<br/><div class="gmail_quote">Hello world</div>`,
+      })
+    });
+  });
+
   describe("prepareNewDraft", ()=> {
     describe("when a signature is defined", ()=> {
       beforeEach(()=> {
-        this.signature = '<div id="signature">This is my signature.</div>';
-        spyOn(NylasEnv.config, 'get').andCallFake(()=> this.signature);
+        spyOn(NylasEnv.config, 'get').andCallFake(()=> TEST_SIGNATURE);
       });
 
-      it("should insert the signature at the end of the message or before the first blockquote and have a newline", ()=> {
+      it("should insert the signature at the end of the message or before the first quoted text block and have a newline", ()=> {
         const a = new Message({
           draft: true,
           accountId: TEST_ACCOUNT_ID,
-          body: 'This is a test! <blockquote>Hello world</blockquote>',
+          body: 'This is a test! <div class="gmail_quote">Hello world</div>',
         });
         const b = new Message({
           draft: true,
@@ -23,36 +45,36 @@ describe("SignatureComposerExtension", ()=> {
         });
 
         SignatureComposerExtension.prepareNewDraft({draft: a});
-        expect(a.body).toEqual('This is a test! <div class="nylas-n1-signature"><div id="signature">This is my signature.</div></div><blockquote>Hello world</blockquote>');
+        expect(a.body).toEqual(`This is a test! <signature>${TEST_SIGNATURE}<br/></signature><div class="gmail_quote">Hello world</div>`);
         SignatureComposerExtension.prepareNewDraft({draft: b});
-        expect(b.body).toEqual('This is a another test.<br/><br/><div class="nylas-n1-signature"><div id="signature">This is my signature.</div></div>');
+        expect(b.body).toEqual(`This is a another test.<signature><br/><br/>${TEST_SIGNATURE}</signature>`);
       });
 
-      it("should replace the signature if a signature is already present", ()=> {
-        const scenarios = [
-          {
-            // With blockquote
-            body: 'This is a test! <div class="nylas-n1-signature"><div>SIG</div></div><blockquote>Hello world</blockquote>',
-            expected: `This is a test! <div class="nylas-n1-signature">${this.signature}</div><blockquote>Hello world</blockquote>`,
-          },
-          {
-            // Populated signature div
-            body: 'This is a test! <div class="nylas-n1-signature"><div>SIG</div></div>',
-            expected: `This is a test! <div class="nylas-n1-signature">${this.signature}</div>`,
-          },
-          {
-            // Empty signature div
-            body: 'This is a test! <div class="nylas-n1-signature"></div>',
-            expected: `This is a test! <div class="nylas-n1-signature">${this.signature}</div>`,
-          },
-          {
-            // With newlines
-            body: 'This is a test! <div class="nylas-n1-signature">\n<br>\n<div>SIG</div>\n</div>',
-            expected: `This is a test! <div class="nylas-n1-signature">${this.signature}</div>`,
-          },
-        ]
+      const scenarios = [
+        {
+          name: 'With blockquote',
+          body: `This is a test! <signature><div>SIG</div></signature><div class="gmail_quote">Hello world</div>`,
+          expected: `This is a test! <signature>${TEST_SIGNATURE}<br/></signature><div class="gmail_quote">Hello world</div>`,
+        },
+        {
+          name: 'Populated signature div',
+          body: `This is a test! <signature><br/><br/><div>SIG</div></signature>`,
+          expected: `This is a test! <signature><br/><br/>${TEST_SIGNATURE}</signature>`,
+        },
+        {
+          name: 'Empty signature div',
+          body: 'This is a test! <signature></signature>',
+          expected: `This is a test! <signature><br/><br/>${TEST_SIGNATURE}</signature>`,
+        },
+        {
+          name: 'With newlines',
+          body: 'This is a test! <signature>\n<br>\n<div>SIG</div>\n</signature>',
+          expected: `This is a test! <signature><br/><br/>${TEST_SIGNATURE}</signature>`,
+        },
+      ]
 
-        scenarios.forEach((scenario)=> {
+      scenarios.forEach((scenario)=> {
+        it(`should replace the signature if a signature is already present (${scenario.name})`, ()=> {
           const message = new Message({
             draft: true,
             body: scenario.body,
@@ -60,7 +82,7 @@ describe("SignatureComposerExtension", ()=> {
           })
           SignatureComposerExtension.prepareNewDraft({draft: message});
           expect(message.body).toEqual(scenario.expected)
-        })
+        });
       });
     });
 
@@ -73,10 +95,10 @@ describe("SignatureComposerExtension", ()=> {
         const a = new Message({
           draft: true,
           accountId: TEST_ACCOUNT_ID,
-          body: 'This is a test! <blockquote>Hello world</blockquote>',
+          body: 'This is a test! <div class="gmail_quote">Hello world</div>',
         });
         SignatureComposerExtension.prepareNewDraft({draft: a});
-        expect(a.body).toEqual(`This is a test! <div class="nylas-n1-signature">${SignatureStore.DefaultSignature}</div><blockquote>Hello world</blockquote>`);
+        expect(a.body).toEqual(`This is a test! <signature>${SignatureStore.DefaultSignature}<br/></signature><div class="gmail_quote">Hello world</div>`);
       });
     });
 
@@ -90,10 +112,10 @@ describe("SignatureComposerExtension", ()=> {
         const a = new Message({
           draft: true,
           accountId: TEST_ACCOUNT_ID,
-          body: 'This is a test! <blockquote>Hello world</blockquote>',
+          body: 'This is a test! <div class="gmail_quote">Hello world</div>',
         });
         SignatureComposerExtension.prepareNewDraft({draft: a});
-        expect(a.body).toEqual(`This is a test! <blockquote>Hello world</blockquote>`);
+        expect(a.body).toEqual(`This is a test! <div class="gmail_quote">Hello world</div>`);
       });
     });
   });

internal_packages/composer-templates/lib/template-store.es6

@@ -108,7 +108,7 @@ class TemplateStore extends NylasStore {
         const draftName = name ? name : draft.subject.replace(TemplateStore.INVALID_TEMPLATE_NAME_REGEX, '');
         let draftContents = contents ? contents : QuotedHTMLTransformer.removeQuotedHTML(draft.body);
 
-        const sigIndex = draftContents.indexOf('<div class="nylas-n1-signature">');
+        const sigIndex = draftContents.indexOf('<signature>');
         draftContents = sigIndex > -1 ? draftContents.slice(0, sigIndex) : draftContents;
         if (!draftName || draftName.length === 0) {
           this._displayError('Give your draft a subject to name your template.');
@@ -259,7 +259,7 @@ class TemplateStore extends NylasStore {
 
         if (proceed) {
           const draftContents = QuotedHTMLTransformer.removeQuotedHTML(session.draft().body);
-          const sigIndex = draftContents.indexOf('<div class="nylas-n1-signature">');
+          const sigIndex = draftContents.indexOf('<signature>');
           const signature = sigIndex > -1 ? draftContents.slice(sigIndex) : '';
 
           const draftHtml = QuotedHTMLTransformer.appendQuotedHTML(templateBody + signature, session.draft().body);

internal_packages/composer/lib/composer-editor.jsx

@@ -144,7 +144,7 @@ class ComposerEditor extends Component {
 
   _findLastNodeBeforeQuoteOrSignature(editor) {
     const walker = document.createTreeWalker(editor.rootNode, NodeFilter.SHOW_TEXT);
-    const nodesBelowUserBody = editor.rootNode.querySelectorAll('.nylas-n1-signature, .gmail_quote, blockquote');
+    const nodesBelowUserBody = editor.rootNode.querySelectorAll('signature, .gmail_quote, blockquote');
 
     let lastNode = null;
     let node = walker.nextNode();

internal_packages/composer/lib/composer-view.cjsx

@@ -732,7 +732,7 @@ class ComposerView extends React.Component
 
   _mentionsAttachment: (body) =>
     body = QuotedHTMLTransformer.removeQuotedHTML(body.toLowerCase().trim())
-    signatureIndex = body.indexOf('<div class="nylas-n1-signature">')
+    signatureIndex = body.indexOf('<signature>')
     body = body[...signatureIndex] if signatureIndex isnt -1
     return body.indexOf("attach") >= 0
 

internal_packages/composer/spec/composer-view-spec.cjsx

@@ -194,7 +194,7 @@ describe "ComposerView", ->
     describe "when sending a forwarded message message", ->
       beforeEach ->
         @fwdBody = """<br><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
-        Begin forwarded message:
+        ---------- Forwarded message ---------
         <br><br>
         From: Evan Morikawa &lt;evan@evanmorikawa.com&gt;<br>Subject: Test Forward Message 1<br>Date: Sep 3 2015, at 12:14 pm<br>To: Evan Morikawa &lt;evan@nylas.com&gt;
         <br><br>

internal_packages/quick-schedule/lib/main.cjsx

@@ -135,7 +135,7 @@ module.exports =
       text = QuotedHTMLTransformer.removeQuotedHTML(draftHtml)
 
       # Check for an N1 signature and split that off
-      sigIndex = text.indexOf('<div class="nylas-n1-signature">')
+      sigIndex = text.indexOf('<signature>')
       beforeSig = if sigIndex>-1 then text.slice(0,sigIndex) else text
       afterSig = text.slice(beforeSig.length)
 

spec/stores/draft-store-spec.coffee

@@ -327,8 +327,9 @@ describe "DraftStore", ->
         runs ->
           @model = DatabaseTransaction.prototype.persistModel.mostRecentCall.args[0]
 
-      it "should include quoted text", ->
-        expect(@model.body.indexOf('blockquote') > 0).toBe(true)
+      it "should include quoted text, but in a div rather than a blockquote", ->
+        expect(@model.body.indexOf('gmail_quote') > 0).toBe(true)
+        expect(@model.body.indexOf('blockquote') > 0).toBe(false)
         expect(@model.body.indexOf(fakeMessage1.body) > 0).toBe(true)
 
       it "should not address the message to anyone", ->
@@ -497,12 +498,12 @@ describe "DraftStore", ->
             expect(model.body.indexOf('gmail_quote') > 0).toBe(true)
             expect(model.body.indexOf('Fake Message 1') > 0).toBe(true)
 
-        it "should include the `Begin forwarded message:` line", ->
+        it "should include the `---------- Forwarded message ---------:` line", ->
           @_callNewMessageWithContext {threadId: fakeThread.id}
           , (thread, message) ->
             forwardMessage: fakeMessage1
           , (model) ->
-            expect(model.body.indexOf('Begin forwarded message') > 0).toBe(true)
+            expect(model.body.indexOf('---------- Forwarded message ---------') > 0).toBe(true)
 
         it "should make the subject the subject of the message, not the thread", ->
           fakeMessage1.subject = "OLD SUBJECT"

src/flux/stores/draft-store.coffee

@@ -337,12 +337,15 @@ class DraftStore
       replyToMessage = attributes.replyToMessage
       @_prepareBodyForQuoting(replyToMessage.body).then (body) ->
         return """
-          <br><br><blockquote class="gmail_quote"
-            style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+          <br><br><div class="gmail_quote">
             #{DOMUtils.escapeHTMLCharacters(replyToMessage.replyAttributionLine())}
             <br>
-            #{body}
-          </blockquote>"""
+            <blockquote class="gmail_quote"
+              style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+              #{body}
+            </blockquote>
+          </div>"""
+
     else if attributes.forwardMessage
       forwardMessage = attributes.forwardMessage
       contactsAsHtml = (cs) ->
@@ -356,14 +359,13 @@ class DraftStore
       fields.push("BCC: #{contactsAsHtml(forwardMessage.bcc)}") if forwardMessage.bcc.length > 0
       @_prepareBodyForQuoting(forwardMessage.body).then (body) ->
         return """
-          <br><br><blockquote class="gmail_quote"
-            style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
-            Begin forwarded message:
+          <br><br><div class="gmail_quote">
+            ---------- Forwarded message ---------
             <br><br>
             #{fields.join('<br>')}
             <br><br>
             #{body}
-          </blockquote>"""
+          </div>"""
     else return Promise.resolve("")
 
   # Eventually we'll want a nicer solution for inline attachments

src/services/quoted-html-transformer.coffee

@@ -181,8 +181,10 @@ class QuotedHTMLTransformer
       el.removeAttribute("data-nylas-quoted-text-original-display")
 
   _findGmailQuotes: (doc) ->
-    # There can sometimes be `div.gmail_quote` that are false positives.
-    return Array::slice.call(doc.querySelectorAll('blockquote.gmail_quote'))
+    # Gmail creates both div.gmail_quote and blockquote.gmail_quote. The div
+    # version marks text but does not cause indentation, but both should be
+    # considered quoted text.
+    return Array::slice.call(doc.querySelectorAll('.gmail_quote'))
 
   _findOffice365Quotes: (doc) ->
     elements = doc.querySelectorAll('#divRplyFwdMsg, #OLK_SRC_BODY_SECTION')