Refactor EditorDom Renderer to ensure renderNodes are not leaked

fixes #132

src/js/editor/editor.js

@@ -88,7 +88,7 @@ class Editor {
     this._renderer = new Renderer(this, this.cards, this.unknownCardHandler, this.cardOptions);
 
     this.post = this.loadPost();
-    this._renderTree = this.prepareRenderTree(this.post);
+    this._renderTree = new RenderTree(this.post);
   }
 
   addView(view) {
@@ -100,13 +100,6 @@ class Editor {
     return this._builder;
   }
 
-  prepareRenderTree(post) {
-    let renderTree = new RenderTree();
-    let node = renderTree.buildRenderNode(post);
-    renderTree.node = node;
-    return renderTree;
-  }
-
   loadPost() {
     if (this.mobiledoc) {
       return new MobiledocParser(this.builder).parse(this.mobiledoc);

src/js/models/render-node.js

@@ -3,50 +3,61 @@ import LinkedList from 'content-kit-editor/utils/linked-list';
 import { containsNode } from 'content-kit-editor/utils/dom-utils';
 
 export default class RenderNode extends LinkedItem {
-  constructor(postNode) {
+  constructor(postNode, renderTree) {
     super();
     this.parent = null;
     this.isDirty = true;
     this.isRemoved = false;
     this.postNode = postNode;
     this._childNodes = null;
-    this.element = null;
+    this._element = null;
+    this.renderTree = renderTree;
   }
   isAttached() {
-    const rootElement = this.renderTree.node.element;
     if (!this.element) {
       throw new Error('Cannot check if a renderNode is attached without an element.');
     }
-    return containsNode(rootElement, this.element);
+    return containsNode(this.renderTree.rootElement, this.element);
   }
   get childNodes() {
     if (!this._childNodes) {
       this._childNodes = new LinkedList({
-        adoptItem: item => {
-          item.parent = this;
-          item.renderTree = this.renderTree;
-        },
-        freeItem: item => {
-          item.parent = null;
-          item.renderTree = null;
-        }
+        adoptItem: item => item.parent = this,
+        freeItem: item => item.destroy()
       });
     }
     return this._childNodes;
   }
   scheduleForRemoval() {
     this.isRemoved = true;
-    if (this.parent) {
-      this.parent.markDirty();
-    }
+    if (this.parent) { this.parent.markDirty(); }
   }
   markDirty() {
     this.isDirty = true;
-    if (this.parent) {
-      this.parent.markDirty();
-    }
+    if (this.parent) { this.parent.markDirty(); }
   }
   markClean() {
     this.isDirty = false;
   }
+  set element(element) {
+    const currentElement = this._element;
+    this._element = element;
+
+    if (currentElement) {
+      this.renderTree.removeElementRenderNode(currentElement);
+    }
+
+    if (element) {
+      this.renderTree.setElementRenderNode(element, this);
+    }
+  }
+  get element() {
+    return this._element;
+  }
+  destroy() {
+    this.element = null;
+    this.parent = null;
+    this.postNode = null;
+    this.renderTree = null;
+  }
 }

src/js/models/render-tree.js

@@ -2,20 +2,38 @@ import RenderNode from 'content-kit-editor/models/render-node';
 import ElementMap from "../utils/element-map";
 
 export default class RenderTree {
-  constructor(node) {
-    this.node = node;
-    this.elements = new ElementMap();
+  constructor(rootPostNode) {
+    this._rootNode = this.buildRenderNode(rootPostNode);
+    this._elements = new ElementMap();
   }
+  /*
+   * @return {RenderNode} The root render node in this tree
+   */
+  get rootNode() {
+    return this._rootNode;
+  }
+  /*
+   * @return {DOMNode} The root DOM element in this tree
+   */
   get rootElement() {
-    return this.node.element;
+    return this.rootNode.element;
   }
+  /*
+   * @param {DOMNode} element
+   * @return {RenderNode} The renderNode for this element, if any
+   */
   getElementRenderNode(element) {
-    return this.elements.get(element);
+    return this._elements.get(element);
+  }
+  setElementRenderNode(element, renderNode) {
+    this._elements.set(element, renderNode);
+  }
+  removeElementRenderNode(element) {
+    this._elements.remove(element);
   }
-  buildRenderNode(section) {
-    let renderNode = new RenderNode(section);
-    renderNode.renderTree = this;
-    section.renderNode = renderNode;
+  buildRenderNode(postNode) {
+    const renderNode = new RenderNode(postNode, this);
+    postNode.renderNode = renderNode;
     return renderNode;
   }
 }

src/js/parsers/post.js

@@ -110,7 +110,6 @@ export default class PostParser {
 
         renderNode = renderTree.buildRenderNode(marker);
         renderNode.element = textNode;
-        renderNode.renderTree.elements.set(textNode, renderNode);
         renderNode.markClean();
 
         let previousRenderNode = previousMarker && previousMarker.renderNode;

src/js/renderers/editor-dom.js

@@ -1,4 +1,3 @@
-import RenderNode from 'content-kit-editor/models/render-node';
 import CardNode from 'content-kit-editor/models/card-node';
 import { detect } from 'content-kit-editor/utils/array-utils';
 import {
@@ -109,7 +108,8 @@ function renderMarker(marker, element, previousRenderNode) {
   return textNode;
 }
 
-function attachRenderNodeElementToDOM(renderNode, element, originalElement) {
+function attachRenderNodeElementToDOM(renderNode, originalElement) {
+  const element = renderNode.element;
   const hasRendered = !!originalElement;
 
   if (hasRendered) {
@@ -150,8 +150,7 @@ class Visitor {
 
   [POST_TYPE](renderNode, post, visit) {
     if (!renderNode.element) {
-      const element = document.createElement('div');
-      renderNode.element = element;
+      renderNode.element = document.createElement('div');
     }
     visit(renderNode, post.sections);
   }
@@ -161,11 +160,8 @@ class Visitor {
 
     // Always rerender the section -- its tag name or attributes may have changed.
     // TODO make this smarter, only rerendering and replacing the element when necessary
-    const element = renderMarkupSection(section);
-    renderNode.element = element;
-
-    attachRenderNodeElementToDOM(renderNode, element, originalElement);
-    renderNode.renderTree.elements.set(element, renderNode);
+    renderNode.element = renderMarkupSection(section);
+    attachRenderNodeElementToDOM(renderNode, originalElement);
 
     if (section.isBlank) {
       renderNode.element.appendChild(renderCursorPlaceholder());
@@ -177,22 +173,18 @@ class Visitor {
 
   [LIST_SECTION_TYPE](renderNode, section, visit) {
     const originalElement = renderNode.element;
-    const element = renderListSection(section);
-    renderNode.element = element;
 
-    attachRenderNodeElementToDOM(renderNode, element, originalElement);
+    renderNode.element = renderListSection(section);
+    attachRenderNodeElementToDOM(renderNode, originalElement);
 
     const visitAll = true;
     visit(renderNode, section.items, visitAll);
   }
 
   [LIST_ITEM_TYPE](renderNode, item, visit) {
     // FIXME do we need to do anything special for rerenders?
-    const element = renderListItem();
-    renderNode.element = element;
-
-    attachRenderNodeElementToDOM(renderNode, element, null);
-    renderNode.renderTree.elements.set(element, renderNode);
+    renderNode.element = renderListItem();
+    attachRenderNodeElementToDOM(renderNode, null);
 
     if (item.isBlank) {
       renderNode.element.appendChild(renderCursorPlaceholder());
@@ -211,10 +203,7 @@ class Visitor {
       parentElement = renderNode.parent.element;
     }
 
-    const element = renderMarker(marker, parentElement, renderNode.prev);
-    renderNode.element = element;
-
-    renderNode.renderTree.elements.set(element, renderNode);
+    renderNode.element = renderMarker(marker, parentElement, renderNode.prev);
   }
 
   [IMAGE_SECTION_TYPE](renderNode, section) {
@@ -243,19 +232,19 @@ class Visitor {
     const originalElement = renderNode.element;
     const {editor, options} = this;
     const card = detect(this.cards, card => card.name === section.name);
-    const element = renderCard();
-    renderNode.element = element;
 
-    attachRenderNodeElementToDOM(renderNode, element, originalElement);
-    renderNode.renderTree.elements.set(element, renderNode); 
+    renderNode.element = renderCard();
+    attachRenderNodeElementToDOM(renderNode, originalElement);
 
     if (card) {
-      const cardNode = new CardNode(editor, card, section, element, options);
+      const cardNode = new CardNode(
+        editor, card, section, renderNode.element, options);
       renderNode.cardNode = cardNode;
       cardNode.display();
     } else {
       const env = { name: section.name };
-      this.unknownCardHandler(element, options, env, section.payload);
+      this.unknownCardHandler(
+        renderNode.element, options, env, section.payload);
     }
   }
 }
@@ -314,13 +303,14 @@ let destroyHooks = {
   }
 };
 
-// removes children from parentNode that are scheduled for removal
-function removeChildren(parentNode) {
+// removes children from parentNode (a RenderNode) that are scheduled for removal
+function removeDestroyedChildren(parentNode, forceRemoval=false) {
   let child = parentNode.childNodes.head;
   let nextChild, method;
   while (child) {
     nextChild = child.next;
-    if (child.isRemoved) {
+    if (child.isRemoved || forceRemoval) {
+      removeDestroyedChildren(child, true);
       method = child.postNode.type;
       if (!destroyHooks[method]) {
         throw new Error(`editor-dom cannot destroy "${method}"`);
@@ -338,9 +328,8 @@ function lookupNode(renderTree, parentNode, postNode, previousNode) {
   if (postNode.renderNode) {
     return postNode.renderNode;
   } else {
-    let renderNode = new RenderNode(postNode);
+    const renderNode = renderTree.buildRenderNode(postNode);
     parentNode.childNodes.insertAfter(renderNode, previousNode);
-    postNode.renderNode = renderNode;
     return renderNode;
   }
 }
@@ -364,20 +353,21 @@ export default class Renderer {
   }
 
   render(renderTree) {
-    let node = renderTree.node;
+    let renderNode = renderTree.rootNode;
     let method, postNode;
 
-    while (node) {
-      removeChildren(node);
-      postNode = node.postNode;
+    while (renderNode) {
+      removeDestroyedChildren(renderNode);
+      postNode = renderNode.postNode;
 
       method = postNode.type;
       if (!this.visitor[method]) {
         throw new Error(`EditorDom visitor cannot handle type ${method}`);
       }
-      this.visitor[node.postNode.type](node, postNode, (...args) => this.visit(renderTree, ...args));
-      node.markClean();
-      node = this.nodes.shift();
+      this.visitor[method](renderNode, postNode,
+                           (...args) => this.visit(renderTree, ...args));
+      renderNode.markClean();
+      renderNode = this.nodes.shift();
     }
   }
 }

tests/unit/editor/post-test.js

@@ -31,17 +31,10 @@ function postEditorWithMobiledoc(treeFn) {
   return new PostEditor(editor);
 }
 
-function prepareRenderTree(post) {
-  let renderTree = new RenderTree();
-  let node = renderTree.buildRenderNode(post);
-  renderTree.node = node;
-  return renderTree;
-}
-
 function renderBuiltAbstract(post) {
   mockEditor.post = post;
   let renderer = new EditorDomRenderer(mockEditor, [], () => {}, {});
-  let renderTree = prepareRenderTree(post);
+  let renderTree = new RenderTree(post);
   renderer.render(renderTree);
 }