Fix adam-p#297: Unrendering fails when replying to (visible) MDH email

src/common/CHANGES.md

@@ -1,19 +1,22 @@
 Change Log
 ==========
 
-### Voting is complete and the [**new Markdown Here logo**](http:https://markdown-here.com/logo.html) has been chosen by the users!
-
 
 2015-xx-yy: v2.11.10
 --------------------
 
-* [Fixed bug #289](https://github.com/adam-p/markdown-here/issues/289): With forgot-to-render detection enabled, sending a large email could result in MDH causing the mail client to hang. (The HTML-to-text processing code had a bad regex.)
+* [Fixed bug #297](https://github.com/adam-p/markdown-here/issues/297): Unrendering an email that is a reply to an email that was itself rendered with MDH would fail (if the original email were visible, like with Thunderbird or when it's expanded in Gmail).
+ - Thanks to [Dave Tapley](https://github.com/dukedave) for creating a great video that illustrated the problem and how to reproduce it.
+ - Repeatedly trigger this bug could also lead to the next bug...
+
+* [Fixed bug #289](https://github.com/adam-p/markdown-here/issues/289): With forgot-to-render detection enabled, sending a large email could result in MDH causing the mail client to hang. 
  - Thanks to [r2evans](https://github.com/r2evans), [Dave Tapley](https://github.com/dukedave), and [Eugene Fryntov](https://github.com/efryntov) for reporting and helping to diagnose the problem. Also thanks to [georg](https://stackoverflow.com/users/989121/georg) on StackOverflow for helping me to [understand and improve](https://stackoverflow.com/questions/31952381/end-of-string-regex-match-too-slow) the offending regex.
 
 * [Fixed bug #283](https://github.com/adam-p/markdown-here/issues/283): Forgot-to-render detection was broken for Google Inbox. Thanks to [Marvin R.](https://github.com/therealmarv).
  - If you find that the forgot-to-render detection gets broken for the Gmail or Google Inbox web interfaces, please post to the ["markdown-here" Google Group](https://groups.google.com/group/markdown-here) or create [an issue in the Github project](https://github.com/adam-p/markdown-here/issues). The MDH code that hooks into the web UI is brittle and might break when Google changes stuff.
 
-* [Fixed bug #288](https://github.com/adam-p/markdown-here/issues/288): Some character combinations involving a dollar sign in inline code would render incorrectly. Thanks to [rfulkerson](https://github.com/rfulkerson).
+* [Fixed bug #288](https://github.com/adam-p/markdown-here/issues/288): Some character combinations involving a dollar sign in inline code would render incorrectly. 
+ - Thanks to [rfulkerson](https://github.com/rfulkerson) for reporting the problem.
 
 
 2015-05-26: v2.11.9

src/common/markdown-here.js

@@ -329,24 +329,55 @@ function hasParentElementOfTagName(element, tagName) {
 }
 
 
-function isWrapperElem(elem) {
- // Make sure the candidate is an element node
- if (elem.nodeType === elem.ELEMENT_NODE) {
- // A MDH wrapper has a child element with a specially prefixed title.
- var rawHolder = elem.querySelector('[title^="' + WRAPPER_TITLE_PREFIX + '"]');
-
- if (rawHolder &&
- // The above `querySelector` will also look at grandchildren of
+// Look for valid raw-MD-holder element under `elem`. Only MDH wrappers will
+// have such an element.
+// Returns null if no raw-MD-holder element is found; otherwise returns that element.
+function findElemRawHolder(elem) {
+ // A raw-MD-holder element has a specially prefixed title and must be an
+ // immediate child of `elem`.
+ //
+ // To restrict our selector to only immediate children of elem, we would
+ // use `:scope > whatever`, but scope is not supported widely enough yet.
+ // See: https://developer.mozilla.org/en-US/docs/Web/CSS/:scope#Browser_compatibility
+ //
+ // If we just take the first `querySelector` result, we may get the wrong
+ // raw-MD-holder element -- a grandchild -- and incorrectly assume that
+ // `elem` is not a wrapper element. So we'll check all query results and
+ // only return false if none of them are immediate children.
+ // Here's an example of a failure case email if we didn't do that:
+ // New rendered MD in a reply here.
+ // On Thu, Aug 13, 2015 at 9:08 PM, Billy Bob wrote:
+ // | Rendered MD in original email here.
+ // | [invisible raw MD holder elem for original email]
+ // [invisible raw MD holder elem for reply]
+ // `querySelector` would return the holder inside the original email.
+ // This scenario is issue #297 https://github.com/adam-p/markdown-here/issues/297
+
+ var rawHolders = elem.querySelectorAll('[title^="' + WRAPPER_TITLE_PREFIX + '"]');
+
+ for (var i = 0; i < rawHolders.length; i++) {
+ if (// The above `querySelector` will also look at grandchildren of
  // `elem`, which we don't want.
- rawHolder.parentNode === elem &&
+ rawHolders[i].parentNode === elem &&
  // Skip all wrappers that are in a `blockquote`. We don't want to revert
  // Markdown that was sent to us.
  !hasParentElementOfTagName(elem, 'BLOCKQUOTE')) {
- return true;
+ return rawHolders[i];
  }
  }
 
- return false;
+ return null;
+}
+
+// Determine if the given element is a MDH wrapper element.
+function isWrapperElem(elem) {
+ return true &&
+ // Make sure the candidate is an element node
+ elem.nodeType === elem.ELEMENT_NODE &&
+ // And is not a blockquote, so we ignore replies
+ elem.tagName.toUpperCase() !== 'BLOCKQUOTE' &&
+ // And has a raw-MD-holder element
+ findElemRawHolder(elem) !== null;
 }
 
 
@@ -480,7 +511,7 @@ function renderMarkdown(focusedElem, selectedRange, markdownRenderer, renderComp
 
 // Revert the rendered Markdown wrapperElem back to its original form.
 function unrenderMarkdown(wrapperElem) {
- var rawHolder = wrapperElem.querySelector('[title^="' + WRAPPER_TITLE_PREFIX + '"]');
+ var rawHolder = findElemRawHolder(wrapperElem);
  // Not checking for success of that call, since we shouldn't be here if there
  // isn't a wrapper.
 

src/common/test/markdown-here-test.js

@@ -19,7 +19,6 @@ describe('markdownHere', function() {
  expect(window.MutationObserver || window.WebKitMutationObserver).to.be.ok;
  });
 
- /*
  describe('markdownHere', function() {
  var userprefs = {};
  var $testElem = null;
@@ -48,9 +47,13 @@ describe('markdownHere', function() {
  callback(renderedMarkdown, userprefs['main-css'] + userprefs['syntax-css']);
  };
 
- var render = function(mdHTML, renderCompleteCallback) {
+ var renderMD = function(mdHTML, renderCompleteCallback) {
  $testElem.html(mdHTML);
  $testElem.focus();
+ renderFocusedElem(renderCompleteCallback);
+ };
+
+ var renderFocusedElem = function(renderCompleteCallback) {
  markdownHere(
  document,
  markdownRenderHelper,
@@ -60,16 +63,54 @@ describe('markdownHere', function() {
 
  // If there's no error, done has to be called with no argument.
  var doneCaller = function(expectedInnerHtml, done) {
+ expectedInnerHtml = expectedInnerHtml.trim();
  return function(elem) {
- MORE COMPLEX THAN THIS: Wrapper elem has data-md-url and data-md-original
- expect(elem.innerHTML).to.equal(expectedInnerHtml);
+ var renderedHTMLRegex = /^<div class="markdown-here-wrapper" data-md-url="[^"]+">([\s\S]*)<div title="MDH:[\s\S]+">[\s\S]*<\/div><\/div>$/;
+ var renderedHTML = elem.innerHTML.match(renderedHTMLRegex)[1];
+ renderedHTML = renderedHTML.trim();
+ expect(renderedHTML).to.equal(expectedInnerHtml);
  done();
  };
  };
 
  it('should render simple MD', function(done) {
- render('_hi_', doneCaller(done));
+ var md = '_hi_';
+ var html = '<p><em>hi</em></p>';
+ renderMD(md, doneCaller(html, done));
+ });
+
+ it('should unrender simple MD', function(done) {
+ var md = '_hi_';
+
+ // First render
+ renderMD(md, function(elem) {
+ // Then unrender
+ $testElem.focus();
+ renderFocusedElem(
+ function(elem) {
+ expect(elem.innerHTML).to.equal(md);
+ done();
+ });
+ });
+ });
+
+ // Tests fix for https://github.com/adam-p/markdown-here/issues/297
+ // Attempting to unrender an email that was a reply to an email that was
+ // itself MDH-rendered failed.
+ it('should unrender a reply to a rendered email', function(done) {
+ var replyMD = '_bye_';
+ var fullReplyMD = replyMD+'<br><div class="gmail_quote">On Fri, Aug 14, 2015 at 10:34 PM, Billy Bob <span dir="ltr">&lt;<a href="mailto:[email protected]" target="_blank">[email protected]</a>&gt;</span> wrote:<br><blockquote><div class="markdown-here-wrapper" data-md-url="xxx"><p><em>hi</em></p>\n<div title="MDH:X2hpXw==" style="height:0;width:0;max-height:0;max-width:0;overflow:hidden;font-size:0em;padding:0;margin:0;">​</div></div></blockquote></div>';
+ // First render
+ renderMD(fullReplyMD, function(elem) {
+ // Then unrender
+ $testElem.focus();
+ renderFocusedElem(
+ function(elem) {
+ expect(elem.innerHTML.slice(0, replyMD.length)).to.equal(replyMD);
+ done();
+ });
+ });
  });
+
  });
- */
 });