From 03def4bebaaf51115342090361212c80028ca751 Mon Sep 17 00:00:00 2001 From: David Greenspan Date: Thu, 14 Aug 2014 14:17:43 -0700 Subject: [PATCH] Fix DOMRange onAttached; view.{first,last}Node() DOMRange#setMembers was causing DOMRange#onAttached callbacks to get called, masking a bug in onViewReady where DOMRange#attached was misspelled DOMRange#isAttached. --- packages/blaze/domrange.js | 28 ++++++++++++++++------------ packages/blaze/view.js | 16 +++++++++++++++- packages/blaze/view_tests.js | 9 +++++++++ packages/ui/render_tests.js | 4 ++-- 4 files changed, 42 insertions(+), 15 deletions(-) diff --git a/packages/blaze/domrange.js b/packages/blaze/domrange.js index 96ae8bf49d..df9f46903c 100644 --- a/packages/blaze/domrange.js +++ b/packages/blaze/domrange.js @@ -120,17 +120,17 @@ DOMRange.forElement = function (elem) { return range; }; -DOMRange.prototype.attach = function (parentElement, nextNode, _isMove) { +DOMRange.prototype.attach = function (parentElement, nextNode, _isMove, _isReplace) { // This method is called to insert the DOMRange into the DOM for // the first time, but it's also used internally when // updating the DOM. // // If _isMove is true, move this attached range to a different // location under the same parentElement. - if (_isMove) { + if (_isMove || _isReplace) { if (! (this.parentElement === parentElement && this.attached)) - throw new Error("Can only move an attached DOMRange, and only under the same parent element"); + throw new Error("Can only move or replace an attached DOMRange, and only under the same parent element"); } var members = this.members; @@ -150,7 +150,7 @@ DOMRange.prototype.attach = function (parentElement, nextNode, _isMove) { this.attached = true; this.parentElement = parentElement; - if (! _isMove) { + if (! (_isMove || _isReplace)) { for(var i = 0; i < this.attachedCallbacks.length; i++) { var obj = this.attachedCallbacks[i]; obj.attached && obj.attached(this, parentElement); @@ -178,9 +178,10 @@ DOMRange.prototype.setMembers = function (newNodeAndRangeArray) { // detach the old members and insert the new members var nextNode = this.lastNode().nextSibling; var parentElement = this.parentElement; - this.detach(); + // Use detach/attach, but don't fire attached/detached hooks + this.detach(true /*_isReplace*/); this.members = newMembers; - this.attach(parentElement, nextNode); + this.attach(parentElement, nextNode, false, true /*_isReplace*/); } } }; @@ -207,7 +208,7 @@ DOMRange.prototype.lastNode = function () { return (m instanceof DOMRange) ? m.lastNode() : m; }; -DOMRange.prototype.detach = function () { +DOMRange.prototype.detach = function (_isReplace) { if (! this.attached) throw new Error("Must be attached"); @@ -222,12 +223,15 @@ DOMRange.prototype.detach = function () { this.parentElement.removeChild(placeholder); this.emptyRangePlaceholder = null; } - this.attached = false; - this.parentElement = null; - for(var i = 0; i < this.attachedCallbacks.length; i++) { - var obj = this.attachedCallbacks[i]; - obj.detached && obj.detached(this, oldParentElement); + if (! _isReplace) { + this.attached = false; + this.parentElement = null; + + for(var i = 0; i < this.attachedCallbacks.length; i++) { + var obj = this.attachedCallbacks[i]; + obj.detached && obj.detached(this, oldParentElement); + } } }; diff --git a/packages/blaze/view.js b/packages/blaze/view.js index 3492882fb4..aa87e9b8ee 100644 --- a/packages/blaze/view.js +++ b/packages/blaze/view.js @@ -92,7 +92,7 @@ Blaze.View.prototype.onViewReady = function (cb) { self._onViewRendered(function onViewRendered() { if (self.isDestroyed) return; - if (! self._domrange.isAttached) + if (! self._domrange.attached) self._domrange.onAttached(fire); else fire(); @@ -167,6 +167,20 @@ Blaze.View.prototype.autorun = function (f, _inViewScope) { return c; }; +Blaze.View.prototype.firstNode = function () { + if (! this.isAttached) + throw new Error("View must be attached before accessing its DOM"); + + return this._domrange.firstNode(); +}; + +Blaze.View.prototype.lastNode = function () { + if (! this.isAttached) + throw new Error("View must be attached before accessing its DOM"); + + return this._domrange.lastNode(); +}; + Blaze._fireCallbacks = function (view, which) { Blaze._withCurrentView(view, function () { Deps.nonreactive(function fireCallbacks() { diff --git a/packages/blaze/view_tests.js b/packages/blaze/view_tests.js index 10678b680b..ffb0653422 100644 --- a/packages/blaze/view_tests.js +++ b/packages/blaze/view_tests.js @@ -32,7 +32,11 @@ if (Meteor.isClient) { test.isFalse(v.isAttached); test.equal(buf, 'c0r1'); test.equal(canonicalizeHtml(div.innerHTML), ""); + test.throws(function () { v.firstNode(); }, /View must be attached/); + test.throws(function () { v.lastNode(); }, /View must be attached/); Blaze.insert(v, div); + test.equal(typeof (v.firstNode().nodeType), "number"); + test.equal(typeof (v.lastNode().nodeType), "number"); test.isTrue(v.isRendered); test.isTrue(v.isAttached); test.equal(buf, 'c0r1'); @@ -48,6 +52,11 @@ if (Meteor.isClient) { Blaze.remove(v); test.equal(buf, 'c0r1y1r2y2d2'); test.equal(canonicalizeHtml(div.innerHTML), ""); + + buf = ""; + R.set("baz"); + Deps.flush(); + test.equal(buf, ""); }); } diff --git a/packages/ui/render_tests.js b/packages/ui/render_tests.js index d1dbc806a0..674fddca92 100644 --- a/packages/ui/render_tests.js +++ b/packages/ui/render_tests.js @@ -499,8 +499,8 @@ Tinytest.add("ui - render - templates and views", function (test) { }; var view = this.view; - var start = view._domrange.firstNode(); - var end = view._domrange.lastNode(); + var start = view.firstNode(); + var end = view.lastNode(); // skip marker nodes while (start !== end && ! nodeDescr(start)) start = start.nextSibling;