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

@@ -16,15 +16,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 +674,45 @@ test('#destroy is safe to call if renderer has not rendered', (assert) => {
   assert.ok(true, 'ok to destroy');
 });
 
+// see https://github.com/bustlelabs/mobiledoc-kit/issues/306
+test('rerender after adding markup to a marker when the marker siblings have that markup', (assert) => {
+  let strong, expected;
+  let post = Helpers.postAbstract.build(({post, markupSection, marker, markup}) => {
+    strong = markup('strong');
+    expected = post([markupSection('p', [marker('aXc', [strong])])]);
+    return post([markupSection('p', [marker('a', [strong]), marker('X'), marker('c', [strong])])]);
+  });
+
+  let renderTree = new RenderTree(post);
+  render(renderTree);
+
+  let markers = post.sections.head.markers.toArray();
+  assert.equal(markers.length, 3);
+
+  // step 1: add markup to the marker
+  markers[1].addMarkup(strong);
+
+  // step 2, join the markers
+  markers[1].value = 'aX';
+  markers[1].renderNode.markDirty();
+  markers[0].renderNode.scheduleForRemoval();
+  markers[0].section.markers.remove(markers[0]);
+
+  markers[2].value = 'aXc';
+  markers[2].renderNode.markDirty();
+  markers[1].renderNode.scheduleForRemoval();
+  markers[1].section.markers.remove(markers[1]);
+
+  render(renderTree);
+
+  assert.renderTreeIsEqual(renderTree, expected);
+
+  markers = post.sections.head.markers.toArray();
+  assert.equal(markers.length, 1);
+  assert.ok(markers[0].hasMarkup(strong), 'marker has strong');
+  assert.equal(markers[0].value, 'aXc');
+});
+
 /*
 test("It renders a renderTree with rendered dirty section", (assert) => {
   /*