Merge branch 'fix-ui-hooks-nested-domranges' into release-0.8.2

packages/spacebars-tests/template_tests.html

@@ -779,6 +779,20 @@ Hi there!
   </div>
 </template>
 
+<template name="spacebars_test_ui_hooks_nested">
+  {{#if foo}}
+    {{> spacebars_test_ui_hooks_nested_sub}}
+  {{/if}}
+</template>
+
+<template name="spacebars_test_ui_hooks_nested_sub">
+  <div>
+    {{#with true}}
+      <p>hello</p>
+    {{/with}}
+  </div>
+</template>
+
 <template name="spacebars_test_template_instance_helper">
   {{#with true}}{{foo}}{{/with}}
 </template>

packages/spacebars-tests/template_tests.js

@@ -2093,6 +2093,40 @@ Tinytest.add(
   }
 );
 
+Tinytest.add(
+  "spacebars - ui hooks - nested domranges",
+  function (test) {
+    var tmpl = Template.spacebars_test_ui_hooks_nested;
+    var rv = new ReactiveVar(true);
+
+    tmpl.foo = function () {
+      return rv.get();
+    };
+
+    var subtmpl = Template.spacebars_test_ui_hooks_nested_sub;
+    var uiHookCalled = false;
+    subtmpl.rendered = function () {
+      this.firstNode.parentNode._uihooks = {
+        removeElement: function (node) {
+          uiHookCalled = true;
+        }
+      };
+    };
+
+    var div = renderToDiv(tmpl);
+    document.body.appendChild(div);
+    Deps.flush();
+
+    var htmlBeforeRemove = canonicalizeHtml(div.innerHTML);
+    rv.set(false);
+    Deps.flush();
+    test.isTrue(uiHookCalled);
+    var htmlAfterRemove = canonicalizeHtml(div.innerHTML);
+    test.equal(htmlBeforeRemove, htmlAfterRemove);
+    document.body.removeChild(div);
+  }
+);
+
 Tinytest.add(
   "spacebars - access template instance from helper",
   function (test) {

packages/ui/dombackend.js

@@ -28,7 +28,7 @@ if (Meteor.isClient) {
 
   // Causes `elem` (a DOM element) to be detached from its parent, if any.
   // Whether or not `elem` was detached, causes any callbacks registered
-  // with `onRemoveElement` on `elem` and its descendants to fire.
+  // with `onElementTeardown` on `elem` and its descendants to fire.
   // Not for use on non-element nodes.
   //
   // This method is modeled after the behavior of jQuery's `$(elem).remove()`,
@@ -37,11 +37,17 @@ if (Meteor.isClient) {
     $jq(elem).remove();
   };
 
+  DomBackend.tearDownElement = function (elem) {
+    var elems = Array.prototype.slice.call(elem.getElementsByTagName('*'));
+    elems.push(elem);
+    $jq.cleanData(elems);
+  };
+
   // Registers a callback function to be called when the given element or
   // one of its ancestors is removed from the DOM via the backend library.
   // The callback function is called at most once, and it receives the element
   // in question as an argument.
-  DomBackend.onRemoveElement = function (elem, func) {
+  DomBackend.onElementTeardown = function (elem, func) {
     if (! elem[REMOVAL_CALLBACKS_PROPERTY_NAME]) {
       elem[REMOVAL_CALLBACKS_PROPERTY_NAME] = [];
 

packages/ui/dombackend_tests.js

@@ -35,16 +35,16 @@ var isDetachedSingleNode = function (test, node) {
 };
 
 Tinytest.add("ui - DomBackend - element removal", function (test) {
-  // Test that calling removeElement on a detached element calls onRemoveElement
+  // Test that calling removeElement on a detached element calls onElementTeardown
   // on it and its descendents. For jQuery, `removeElement` runs `$(elem).remove()`,
   // so it tests detecting a jQuery removal, as well as the stronger condition
   // that clean-up still happens on the DOM tree in the detached case.
   runDivSpanBTest(function (div, span, b, buf, func1, func2, func3, func4) {
-    DomBackend.onRemoveElement(div, func1);
-    DomBackend.onRemoveElement(span, func2);
-    DomBackend.onRemoveElement(b, func3);
+    DomBackend.onElementTeardown(div, func1);
+    DomBackend.onElementTeardown(span, func2);
+    DomBackend.onElementTeardown(b, func3);
     // test second callback on same element
-    DomBackend.onRemoveElement(div, func4);
+    DomBackend.onElementTeardown(div, func4);
 
     DomBackend.removeElement(div); // "remove" the (parentless) DIV
 
@@ -59,10 +59,10 @@ Tinytest.add("ui - DomBackend - element removal", function (test) {
   // Test that `removeElement` actually removes the element
   // (and fires appropriate callbacks).
   runDivSpanBTest(function (div, span, b, buf, func1, func2, func3, func4) {
-    DomBackend.onRemoveElement(div, func1);
-    DomBackend.onRemoveElement(span, func2);
-    DomBackend.onRemoveElement(b, func3);
-    DomBackend.onRemoveElement(div, func4);
+    DomBackend.onElementTeardown(div, func1);
+    DomBackend.onElementTeardown(span, func2);
+    DomBackend.onElementTeardown(b, func3);
+    DomBackend.onElementTeardown(div, func4);
 
     DomBackend.removeElement(span); // remove the SPAN
 
@@ -83,10 +83,10 @@ Tinytest.add("ui - DomBackend - element removal (jQuery)", function (test) {
 
   // Test with `$(elem).remove()`.
   runDivSpanBTest(function (div, span, b, buf, func1, func2, func3, func4) {
-    DomBackend.onRemoveElement(div, func1);
-    DomBackend.onRemoveElement(span, func2);
-    DomBackend.onRemoveElement(b, func3);
-    DomBackend.onRemoveElement(div, func4);
+    DomBackend.onElementTeardown(div, func1);
+    DomBackend.onElementTeardown(span, func2);
+    DomBackend.onElementTeardown(b, func3);
+    DomBackend.onElementTeardown(div, func4);
 
     $(span).remove(); // remove the SPAN
 
@@ -103,10 +103,10 @@ Tinytest.add("ui - DomBackend - element removal (jQuery)", function (test) {
 
   // Test that `$(elem).detach()` is NOT considered a removal.
   runDivSpanBTest(function (div, span, b, buf, func1, func2, func3, func4) {
-    DomBackend.onRemoveElement(div, func1);
-    DomBackend.onRemoveElement(span, func2);
-    DomBackend.onRemoveElement(b, func3);
-    DomBackend.onRemoveElement(div, func4);
+    DomBackend.onElementTeardown(div, func1);
+    DomBackend.onElementTeardown(span, func2);
+    DomBackend.onElementTeardown(b, func3);
+    DomBackend.onElementTeardown(div, func4);
 
     $(span).detach(); // detach the SPAN
 

packages/ui/domrange.js

@@ -110,8 +110,8 @@ var rangeParented = function (range) {
       range._rangeDict = rangeDict;
 
       // get jQuery to tell us when this node is removed
-      DomBackend.onRemoveElement(parentNode, function () {
-        rangeRemoved(range, true /* elementsAlreadyRemoved */);
+      DomBackend.onElementTeardown(parentNode, function () {
+        rangeRemoved(range, true /* alreadyTornDown */);
       });
     }
 
@@ -128,7 +128,7 @@ var rangeParented = function (range) {
   }
 };
 
-var rangeRemoved = function (range, elementsAlreadyRemoved) {
+var rangeRemoved = function (range, alreadyTornDown) {
   if (! range.isRemoved) {
     range.isRemoved = true;
 
@@ -146,29 +146,39 @@ var rangeRemoved = function (range, elementsAlreadyRemoved) {
     if (range.removed)
       range.removed();
 
-    membersRemoved(range, elementsAlreadyRemoved);
+    membersRemoved(range, alreadyTornDown);
   }
 };
 
-var nodeRemoved = function (node, elementsAlreadyRemoved) {
+var nodeRemoved = function (node, alreadyTornDown) {
   if (node.nodeType === 1) { // ELEMENT
     var comps = DomRange.getComponents(node);
     for (var i = 0, N = comps.length; i < N; i++)
-      rangeRemoved(comps[i]);
+      rangeRemoved(comps[i], true /* alreadyTornDown */);
 
-    if (! elementsAlreadyRemoved)
-      DomBackend.removeElement(node);
+    // `alreadyTornDown` is an optimization so that we don't
+    // tear down the same elements multiple times when tearing
+    // down a tree of DomRanges and elements, leading to asymptotic
+    // inefficiency.
+    //
+    // When jQuery removes an element or DomBackend.tearDownElement
+    // is called, the DOM is "cleaned" recursively, calling all
+    // onElementTearDown handlers on the entire DOM subtree.
+    // Since the entire subtree is already walked, we don't want to
+    // also walk the subtrees of each DomRange for teardown purposes.
+    if (! alreadyTornDown)
+      DomBackend.tearDownElement(node);
   }
 };
 
-var membersRemoved = function (range, elementsAlreadyRemoved) {
+var membersRemoved = function (range, alreadyTornDown) {
   var members = range.members;
   for (var k in members) {
     var mem = members[k];
     if (mem instanceof DomRange)
-      rangeRemoved(mem, elementsAlreadyRemoved);
+      rangeRemoved(mem, alreadyTornDown);
     else
-      nodeRemoved(mem, elementsAlreadyRemoved);
+      nodeRemoved(mem, alreadyTornDown);
   }
 };
 
@@ -230,7 +240,7 @@ _extend(DomRange.prototype, {
     for (var i = 0, N = nodes.length; i < N; i++)
       removeNode(nodes[i]);
 
-    membersRemoved(this, true /* elementsAlreadyRemoved */);
+    membersRemoved(this);
 
     this.members = {};
   },
@@ -341,7 +351,7 @@ _extend(DomRange.prototype, {
       removeNode(this.start);
       removeNode(this.end);
       this.owner = null;
-      rangeRemoved(this, true /* elementsAlreadyRemoved */);
+      rangeRemoved(this);
       return;
     }
 

packages/ui/domrange_tests.js

@@ -25,7 +25,8 @@ var inDocument = function (range, func) {
   try {
     func(range);
   } finally {
-    document.body.removeChild(onscreen);
+    if (onscreen.parentNode === document.body)
+      document.body.removeChild(onscreen);
   }
 };
 
@@ -862,7 +863,7 @@ Tinytest.add("ui - DomRange - structural removal", function (test) {
     test.isTrue(e.isRemoved);
 
 
-    for (var scenario = 0; scenario < 2; scenario++) {
+    for (var scenario = 0; scenario < 3; scenario++) {
       var f = new DomRange;
       var g = document.createElement("DIV");
       var h = new DomRange;
@@ -882,6 +883,8 @@ Tinytest.add("ui - DomRange - structural removal", function (test) {
         r.removeAll();
       else if (scenario === 1)
         r.remove('f');
+      else if (scenario === 2)
+        $(r.parentNode()).remove();
       test.isTrue(f.isRemoved);
       test.isTrue(h.isRemoved);
       test.isTrue(k.isRemoved);

packages/ui/render.js

@@ -382,7 +382,7 @@ var materialize = function (node, parent, before, parentComponent) {
           reportUIException(e);
         }
       });
-      UI.DomBackend.onRemoveElement(elem, function () {
+      UI.DomBackend.onElementTeardown(elem, function () {
         attrComp.stop();
       });
     }