Keep track and use markupElement for marker when rendering/destroying

Fixes #306
Fixes #299

src/js/models/render-node.js

@@ -13,6 +13,9 @@ export default class RenderNode extends LinkedItem {
     this._childNodes = null;
     this._element = null;
     this.renderTree = renderTree;
+
+    // RenderNodes for Markers keep track of their markupElement
+    this.markupElement = null;
   }
   isAttached() {
     assert('Cannot check if a renderNode is attached without an element.',

src/js/renderers/editor-dom.js

@@ -22,7 +22,7 @@ export const SPACE = ' ';
 export const ZWNJ = '\u200c';
 
 function createElementFromMarkup(doc, markup) {
-  var element = doc.createElement(markup.tagName);
+  let element = doc.createElement(markup.tagName);
   Object.keys(markup.attributes).forEach(k => {
     element.setAttribute(k, markup.attributes[k]);
   });
@@ -120,30 +120,44 @@ function getNextMarkerElement(renderNode) {
   return element;
 }
 
-function renderMarker(marker, element, previousRenderNode) {
+/**
+ * Render the marker
+ * @param {Marker} marker the marker to render
+ * @param {DOMNode} element the element to attach the rendered marker to
+ * @param {RenderNode} [previousRenderNode] The render node before this one, which
+ *        affects the determination of where to insert this rendered marker.
+ * @return {element, markupElement} The element (textNode) that has the text for
+ *         this marker, and the outermost rendered element. If the marker has no
+ *         markups, element and markupElement will be the same textNode
+ */
+function renderMarker(marker, parentElement, previousRenderNode) {
   let text = renderHTMLText(marker);
 
-  let textNode = document.createTextNode(text);
-  let currentElement = textNode;
+  let element = document.createTextNode(text);
+  let markupElement = element;
   let markup;
 
   const openTypes = marker.openedMarkups;
   for (let j=openTypes.length-1;j>=0;j--) {
     markup = openTypes[j];
     let openedElement = createElementFromMarkup(document, markup);
-    openedElement.appendChild(currentElement);
-    currentElement = openedElement;
+    openedElement.appendChild(markupElement);
+    markupElement = openedElement;
   }
 
+  let referenceElement;
+
   if (previousRenderNode) {
     let previousSibling = previousRenderNode.element;
-    let previousSiblingPenultimate = penultimateParentOf(previousSibling, element);
-    element.insertBefore(currentElement, previousSiblingPenultimate.nextSibling);
+    let previousSiblingPenultimate = penultimateParentOf(previousSibling, parentElement);
+    referenceElement = previousSiblingPenultimate.nextSibling;
   } else {
-    element.insertBefore(currentElement, element.firstChild);
+    referenceElement = parentElement.firstChild;
   }
 
-  return textNode;
+  parentElement.insertBefore(markupElement, referenceElement);
+
+  return { element, markupElement };
 }
 
 function attachRenderNodeElementToDOM(renderNode, originalElement) {
@@ -274,7 +288,11 @@ class Visitor {
       parentElement = renderNode.parent.element;
     }
 
-    renderNode.element = renderMarker(marker, parentElement, renderNode.prev);
+    let { element, markupElement } =
+      renderMarker(marker, parentElement, renderNode.prev);
+
+    renderNode.element = element;
+    renderNode.markupElement = markupElement;
   }
 
   [IMAGE_SECTION_TYPE](renderNode, section) {
@@ -342,19 +360,15 @@ let destroyHooks = {
     // FIXME before we render marker, should delete previous renderNode's element
     // and up until the next marker element
 
-    let element = renderNode.element;
-    let nextMarkerElement = getNextMarkerElement(renderNode);
-    while (element.parentNode && element.parentNode !== nextMarkerElement) {
-      element = element.parentNode;
-    }
+    let { markupElement } = renderNode;
 
     if (marker.section) {
       marker.section.markers.remove(marker);
     }
 
-    if (element.parentNode) {
+    if (markupElement.parentNode) {
       // if no parentNode, the browser already removed this element
-      element.parentNode.removeChild(element);
+      markupElement.parentNode.removeChild(markupElement);
     }
   },
 

tests/acceptance/basic-editor-test.js

@@ -236,3 +236,34 @@ test('typing enter splits lines, sets cursor', (assert) => {
     done();
   }, 0);
 });
+
+// see https://github.com/bustlelabs/mobiledoc-kit/issues/306
+test('adding/removing bold text between two bold markers works', (assert) => {
+  editor = Helpers.mobiledoc.renderInto(editorElement, ({post, markupSection, marker, markup}) => {
+    return post([
+      markupSection('p', [
+        marker('abc', [markup('b')]),
+        marker('123', []),
+        marker('def', [markup('b')])
+      ])
+    ]);
+  });
+
+  // preconditions
+  assert.hasElement('#editor b:contains(abc)');
+  assert.hasElement('#editor b:contains(def)');
+  assert.hasNoElement('#editor b:contains(123)');
+
+  Helpers.dom.selectText('123', editorElement);
+  editor.run(postEditor => postEditor.toggleMarkup('b'));
+
+  assert.hasElement('#editor b:contains(abc123def)', 'adds B to selection');
+
+  assert.equal(Helpers.dom.getSelectedText(), '123', '123 still selected');
+
+  editor.run(postEditor => postEditor.toggleMarkup('b'));
+
+  assert.hasElement('#editor b:contains(abc)', 'removes B from middle, leaves abc');
+  assert.hasElement('#editor b:contains(def)', 'removes B from middle, leaves def');
+  assert.hasNoElement('#editor b:contains(123)', 'removes B from middle');
+});

tests/unit/renderers/editor-dom-test.js

@@ -5,6 +5,7 @@ import Helpers from '../../test-helpers';
 import { NO_BREAK_SPACE, ZWNJ } from 'mobiledoc-kit/renderers/editor-dom';
 import { TAB } from 'mobiledoc-kit/utils/characters';
 const { module, test } = Helpers;
+import Range from 'mobiledoc-kit/utils/cursor/range';
 
 import placeholderImageSrc from 'mobiledoc-kit/utils/placeholder-image-src';
 let builder;
@@ -16,15 +17,20 @@ function render(renderTree, cards=[]) {
   return renderer.render(renderTree);
 }
 
+let editor, editorElement;
 module('Unit: Renderer: Editor-Dom', {
   beforeEach() {
     builder = new PostNodeBuilder();
+    editorElement = $('#editor')[0];
   },
   afterEach() {
     if (renderer) {
       renderer.destroy();
       renderer = null;
     }
+    if (editor) {
+      editor.destroy();
+    }
   }
 });
 
@@ -669,6 +675,34 @@ test('#destroy is safe to call if renderer has not rendered', (assert) => {
   assert.ok(true, 'ok to destroy');
 });
 
+test('rerender after adding markup to a marker when the marker siblings have that markup', (assert) => {
+  let strong, midMarker;
+
+  editor = Helpers.mobiledoc.renderInto(editorElement, ({post, markupSection, marker, markup}) => {
+    strong = markup('strong');
+    midMarker = marker('X');
+    return post([
+      markupSection('p', [
+        marker('a', [strong]),
+        midMarker,
+        marker('c', [strong])
+      ])
+    ]);
+  });
+
+  assert.hasElement('#editor strong:contains(a)');
+  assert.hasElement('#editor strong:contains(c)');
+  assert.hasNoElement('#editor strong:contains(X)');
+
+  editor.run(postEditor => {
+    let range = Range.create(editor.post.sections.head, 1,
+                             editor.post.sections.head, 2);
+    postEditor.toggleMarkup('strong', range);
+  });
+
+  assert.hasElement('#editor strong:contains(aXc)');
+});
+
 /*
 test("It renders a renderTree with rendered dirty section", (assert) => {
   /*