From dc2b03aee634cf5b99cb6023b957707b41ff91d6 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Wed, 18 Jun 2014 16:45:47 -0700 Subject: [PATCH 1/3] Tentative maybe fix for UI hooks on nested domranges. --- packages/spacebars-tests/template_tests.html | 14 +++++++++ packages/spacebars-tests/template_tests.js | 33 ++++++++++++++++++++ packages/ui/domrange.js | 4 +-- 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/packages/spacebars-tests/template_tests.html b/packages/spacebars-tests/template_tests.html index 4477b18c64..0964ba4b9f 100644 --- a/packages/spacebars-tests/template_tests.html +++ b/packages/spacebars-tests/template_tests.html @@ -779,6 +779,20 @@ Hi there! + + + + diff --git a/packages/spacebars-tests/template_tests.js b/packages/spacebars-tests/template_tests.js index e610da4652..599917ca1c 100644 --- a/packages/spacebars-tests/template_tests.js +++ b/packages/spacebars-tests/template_tests.js @@ -2093,6 +2093,39 @@ 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); + } +); + Tinytest.add( "spacebars - access template instance from helper", function (test) { diff --git a/packages/ui/domrange.js b/packages/ui/domrange.js index dc958eaa11..1b46b5cae1 100644 --- a/packages/ui/domrange.js +++ b/packages/ui/domrange.js @@ -11,7 +11,7 @@ var removeNode = function (n) { n.parentNode._uihooks && n.parentNode._uihooks.removeElement) { n.parentNode._uihooks.removeElement(n); } else { - n.parentNode.removeChild(n); + DomBackend.removeElement(n); } }; @@ -154,7 +154,7 @@ var nodeRemoved = function (node, elementsAlreadyRemoved) { 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], elementsAlreadyRemoved); if (! elementsAlreadyRemoved) DomBackend.removeElement(node); From 4ec9cb5847be61226538c066523d42bc2d143fd8 Mon Sep 17 00:00:00 2001 From: David Greenspan Date: Fri, 20 Jun 2014 12:08:08 -0700 Subject: [PATCH 2/3] Prevent a test from leaving a DIV behind --- packages/spacebars-tests/template_tests.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/spacebars-tests/template_tests.js b/packages/spacebars-tests/template_tests.js index 599917ca1c..da45b0bed9 100644 --- a/packages/spacebars-tests/template_tests.js +++ b/packages/spacebars-tests/template_tests.js @@ -2123,6 +2123,7 @@ Tinytest.add( test.isTrue(uiHookCalled); var htmlAfterRemove = canonicalizeHtml(div.innerHTML); test.equal(htmlBeforeRemove, htmlAfterRemove); + document.body.removeChild(div); } ); From 085441524fc3f4badbbee1dfe825fe3128dc95ef Mon Sep 17 00:00:00 2001 From: David Greenspan Date: Fri, 20 Jun 2014 12:34:23 -0700 Subject: [PATCH 3/3] =?UTF-8?q?Separate=20element=20removal=20and=20?= =?UTF-8?q?=E2=80=9Cteardown=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DomRange now never removes elements except through the removeElement UI hook. If you write a hook that prevents removal, teardown still happens (e.g. templates stop updating). This code also provides the basis for stopping updates to part of the DOM by triggering teardown without removal. Before, DomBackend.removeElement would both trigger teardown and actually deparent the element. Now we have DomBackend.tearDownElement, which just triggers the jQuery teardown (which in turn triggers finalization of DomRanges that have been inserted in the DOM tree). The flag to {node,members,range}Removed is now named “alreadyTornDown” and documented. Its purpose is to prevent redundant teardown walks through the DOM. --- packages/ui/dombackend.js | 10 +++++++-- packages/ui/dombackend_tests.js | 34 ++++++++++++++--------------- packages/ui/domrange.js | 38 +++++++++++++++++++++------------ packages/ui/domrange_tests.js | 7 ++++-- packages/ui/render.js | 2 +- 5 files changed, 55 insertions(+), 36 deletions(-) diff --git a/packages/ui/dombackend.js b/packages/ui/dombackend.js index 7800723175..d94d8fbcc0 100644 --- a/packages/ui/dombackend.js +++ b/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] = []; diff --git a/packages/ui/dombackend_tests.js b/packages/ui/dombackend_tests.js index 1b3583b192..5799e3c030 100644 --- a/packages/ui/dombackend_tests.js +++ b/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 diff --git a/packages/ui/domrange.js b/packages/ui/domrange.js index 1b46b5cae1..a875b759de 100644 --- a/packages/ui/domrange.js +++ b/packages/ui/domrange.js @@ -11,7 +11,7 @@ var removeNode = function (n) { n.parentNode._uihooks && n.parentNode._uihooks.removeElement) { n.parentNode._uihooks.removeElement(n); } else { - DomBackend.removeElement(n); + n.parentNode.removeChild(n); } }; @@ -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], elementsAlreadyRemoved); + 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; } diff --git a/packages/ui/domrange_tests.js b/packages/ui/domrange_tests.js index fb042f2dea..2fd7f2b519 100644 --- a/packages/ui/domrange_tests.js +++ b/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); diff --git a/packages/ui/render.js b/packages/ui/render.js index 31da03bda3..58d30787dc 100644 --- a/packages/ui/render.js +++ b/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(); }); }