Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Templating fixes #144

Merged
merged 8 commits into from

4 participants

@seanami

It turns out that the behavior of the dummy templating engine in the templating tests hides some issues with memoization and templating output.

The main issue is that the default templating engine returns an array of node objects at the base level of the template, while the dummy templating engine wraps the template output in a single div. This means that (unlike with the default engine) there are never any data-bind memo comments at the base level of the template. This situation causes problems, so it shouldn't be suppressed in the tests.

When the memos are unmemoized, they aren't removed from the array of node objects when they're removed from their parent, so empty comment nodes end up in the DOM. This can cause nasty issues when combined with other features of Knockout:

  • When you use the beforeRemove hook for the template binding and remove the DOM node yourself, the empty comment nodes start to build up in the container, never getting removed.
  • When the first node is an empty comment node, it gets used as the node that triggers disposal of the dependent observable for the template when it's removed. This means that if you try to clean up the empty comment nodes yourself at a later time, your rendered templates stop updating.

This pull request makes a few changes:

  • Changes the templating tests to not wrap their output in a div and return it as an array, just like the default templating engine does. This reveals the empty comment node issues in the tests.
  • Fixes the issue for most templating by shifting the unmemoize step to be after the rendered nodes are added to the DOM.
  • Adds a test to catch empty comments that are still present when using the template binding with foreach (since the "ignoreTargetNode" rendering mode is used here).
  • Fixes the issue for this case by adding a memoize method that removes empty memos, and uses it after the foreach templates are all done rendering.
@SteveSanderson

Thanks very much for this. I definitely want to apply these changes, but I'll be applying them to the 1.3 branch where there have been various changes to templating and binding logic already. So, it might take a little while before I can migrate your changes over to the new 1.3 world. Hope that's OK!

@seanami

Not a problem. I have a workaround already in place (adding an empty memo cleaning method to the afterRender callback). I just wanted to get this in so you could see it asap.

I think the two most important things to get over into 1.3 are in the test changes:

  • Altering the dummy templating engine to not wrap output so it doesn't hide empty memo issues.
  • Adding a test for the foreach with data-bind on root template elements case, to ensure empty memos are removed there too.

Thanks for Knockout! I'm really enjoying using it. :)

@SteveSanderson

Scheduling for consideration for v1.3.1 (as it's important, but not urgent enough to block 1.3.0)

@mbest
Collaborator

It appears that this bug, in conjunction with other changes in 2.1, causes the problem demonstrated here: http://jsfiddle.net/madcapnmckay/radm8/ I think we should fix this in 2.1 before it's released.

@mbest
Collaborator

Never mind. The problem in that fiddle is related, but not the same as this issue. Actually the main problem described here is already fixed in 2.0, which runs the unmemoize function after the nodes are added to the document. The problem demonstrated in the fiddle is that the memo comment is still in the array of nodes held by setDomNodeChildrenFromArrayMapping and if that comment is the first item in the array, it messes things up.

@SteveSanderson

After seeing issue #440, which may not have occurred (or could have been detected by specs already) if this pull request had been merged already, I'm especially keen to include this in v2.2.

Also, reading through it again, it's an extremely well written pull request with the changes clearly delineated and justified - so thanks again Seanami!

Until we do merge this pull request in, it's not possible to write a spec to demonstrate the fix for #440 (because the dummy template engine doesn't experience the same issue).

@seanami

You're welcome! :)

@mbest
Collaborator

To make sure arrayToDomNodeChildren doesn't try to track the removed comment, add this to the top of fixUpVirtualElements:

// Remove any initial nodes that aren't in the document
while (contiguousNodeArray.length && !ko.utils.domNodeIsAttachedToDocument(contiguousNodeArray[0]))
    contiguousNodeArray.splice(0, 1);
Sean McBride and others added some commits
Sean McBride Alter templating tests to behave like real templating engines
The default templating engine returns an array of nodes that's not wrapped in
a container, and isn't a nodelist. The dummy templating engine in the test
always wraps the returned markup in a div, which masks some issues with the
templating code with empty memo comments that stick around in the markup.

After this change, all of the tests that are failing fail because of these
empty memo comments that stick around with the real templating engine due to
the templating result being returned as a bare array of nodes (which the memo
cleanup can't remove a node from).
ed307bc
Sean McBride Use a better util function in the templating tests a6a411d
@mbest mbest Fix small issues with merging template spec fixes by @seanami
* change makeArray to arrayPushAll (makeArray isn't exported)
* remove `div` wrapper from new tests (that were added since the original commit)
* fix other tests that were changed since the original commit
76ab9ea
@mbest
Collaborator

I've updated the template spec portion of this pull request to the latest version. I will add commits to include a new test and updated fix for #440.

@mbest mbest #440 and #144 (foreach, re-written template, data-bind)
Add spec to show that foreach correctly handles templates where first node has a binding.
Add secondary fix for #440, which will solve the original bug in a different way and possibly prevent other problems down the road.
98ffbd0
@mbest mbest referenced this pull request
Closed

2.2 release discussion #479

@SteveSanderson SteveSanderson Fix spec failure that occurred only when *all* specs are run (not whe…
…n just running template specs).

Really, we should stop using `testNode` as a global variable, as it introduces order dependencies between specs.
c666cb5
@SteveSanderson

Added a fix for a spec issue.

I'm a bit unsure about the following change:

// Remove any initial nodes that aren't in the document
while (contiguousNodeArray.length && !ko.utils.domNodeIsAttachedToDocument(contiguousNodeArray[0]))
    contiguousNodeArray.splice(0, 1);

Could you clarify why this is the right thing to do? It looks very special-casey, but the comment doesn't say what special case it addresses. Why are we only removing a continuous sequence of leading nodes, and not all nodes that aren't in the document? And if the fix to #440 already provides the desired behaviour, what extra behavior or safety does this subsequent addition provide?

Hope that doesn't sound too demanding - I just want to be sure all the code in KO will make sense to anyone who tries to understand it, and right now, I genuinely don't know what scenario this bit uniquely handles.

@SteveSanderson

Also merged master into this branch so that Travis will verify it properly.

@mbest
Collaborator

Hope that doesn't sound too demanding.

Not at all. I'll work on it.

@mbest
Collaborator

Here's the explanation. If the first node in a string-based template has a binding, a comment is inserted before it during template rendering and then removed during un-memoization. But arrayToDomNodeChildren still includes that node in its mappedNodes array. When it needs to remove an array entry, it calls fixUpVirtualElements, but it doesn't do anything because the first node's nextSibling is null. Thus it fails to remove nodes that were added during binding or unmemoization.

I've created a fiddle to demonstrate: http://jsfiddle.net/mbest/KDGP9/ If you remove an item from the array, the x's will not be removed.

Now here's the same example with some text added before: http://jsfiddle.net/mbest/v8Ls6/ Now you'll see it working because the first node is okay.

@mbest
Collaborator

Why are we only removing a continuous sequence of leading nodes, and not all nodes that aren't in the document?

To answer this question, once we have an initial node in the document, the remainder will be have to be in the document since they are loaded using nextSibling. So we only need to make sure we have a valid initial node.

With this change we could revert the change made in #440 if we want.

@mbest
Collaborator

I've created a fiddle to demonstrate:

I first tried demonstrate the problem using an inner template with virtual elements (as that's what fixUpVirtualElements was designed to deal with), but I ran into a different bug that prevented it from working. Here's the fiddle and the issue: #494.

@mbest mbest #144 - arrayToDomNodeChildren/fixUpVirtualElements updates and specs
Add spec and expand comments on earlier change.
Fix up node list for any array with 2 or more items (rather than 3 or more).
Add spec for above.
3ec0059
@mbest
Collaborator

I added two new specs related to fixUpVirtualElements. One was fixed in 98ffbd0 and the other I've fixed in 3ec0059.

@mbest mbest was assigned
@SteveSanderson

Here's the explanation. If the first node in a string-based template has a binding, a comment is inserted before it during template rendering and then removed during un-memoization.

Thanks. I was aware of that, but my concern was that the code was diverging from its stated design and was introducing implicit dependencies that no longer quite made sense. Having the function fixUpVirtualElements become entangled with the implementation details of string-based templating, a totally separate feature, was an alarm for me.

In the end, I can accept that fixUpVirtualElements just has to make a bunch of guesses about what DOM nodes you are trying to remove/replace, but we should be explicit about this. So, I've renamed it to fixUpNodesToBeRemoved and updated the comments to clarify why we're doing this heuristic kind of thing.

Thanks very much for adding the extra specs, and for the extra bugfix. This is ready to merge now if you're happy with my function rename and the new comments. Or I'll merge it myself if you prefer - just let me know if so!

@mbest
Collaborator

This is good. Thanks for making the code more informative.

I think we may want to change the name again soon, though, as it will be used for more than determining nodes to remove (see #259, #484).

@mbest mbest merged commit c558d08 into master
@domenic

Agh, why is the Knockout release cycle so slow. I've been hitting my head against this problem for a few days before finally tracking it down to this line. I wish it were released already :(

@smerchek smerchek referenced this pull request from a commit in softek/knockout
@smerchek smerchek Fix error in fixUpContinuousNodeArray when using jquery.tmpl
The `shift` method does not exist on jQuery objects. However, the `splice` method does exist. Previously, `splice(0,1)` was used as of #144 but was replaced with `shift()` in [this commit](knockout@7c99a94#diff-2b4ca49d4bb0a774c4d4c1672d7aa781R235).

A workaround can be found [on StackOverflow:](http://stackoverflow.com/questions/6515544/how-to-pop-or-shift-from-jquery-set)

```
(function( $ ) {
    $.fn.pop = function() {
        var top = this.get(-1);
        this.splice(this.length-1,1);
        return top;
    };

    $.fn.shift = function() {
        var bottom = this.get(0);
        this.splice(0,1);
        return bottom;
    };
})( jQuery );
```
8b3c402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 12, 2012
  1. @mbest

    Alter templating tests to behave like real templating engines

    Sean McBride authored mbest committed
    The default templating engine returns an array of nodes that's not wrapped in
    a container, and isn't a nodelist. The dummy templating engine in the test
    always wraps the returned markup in a div, which masks some issues with the
    templating code with empty memo comments that stick around in the markup.
    
    After this change, all of the tests that are failing fail because of these
    empty memo comments that stick around with the real templating engine due to
    the templating result being returned as a bare array of nodes (which the memo
    cleanup can't remove a node from).
  2. @mbest

    Use a better util function in the templating tests

    Sean McBride authored mbest committed
  3. @mbest

    Fix small issues with merging template spec fixes by @seanami

    mbest authored
    * change makeArray to arrayPushAll (makeArray isn't exported)
    * remove `div` wrapper from new tests (that were added since the original commit)
    * fix other tests that were changed since the original commit
  4. @mbest

    #440 and #144 (foreach, re-written template, data-bind)

    mbest authored
    Add spec to show that foreach correctly handles templates where first node has a binding.
    Add secondary fix for #440, which will solve the original bug in a different way and possibly prevent other problems down the road.
Commits on May 18, 2012
  1. @SteveSanderson

    Fix spec failure that occurred only when *all* specs are run (not whe…

    SteveSanderson authored
    …n just running template specs).
    
    Really, we should stop using `testNode` as a global variable, as it introduces order dependencies between specs.
  2. @SteveSanderson
Commits on May 19, 2012
  1. @mbest

    #144 - arrayToDomNodeChildren/fixUpVirtualElements updates and specs

    mbest authored
    Add spec and expand comments on earlier change.
    Fix up node list for any array with 2 or more items (rather than 3 or more).
    Add spec for above.
Commits on Jun 6, 2012
  1. @SteveSanderson

    Rename fixUpVirtualElements to fixUpNodesToBeRemoved and amend commen…

    SteveSanderson authored
    …ts, since the function now has a different role
This page is out of date. Refresh to see the latest.
View
19 spec/defaultBindingsBehaviors.js
@@ -1450,6 +1450,25 @@ describe('Binding: Foreach', {
value_of(testNode).should_contain_html('<div data-bind="foreach: someitems">a<!-- ko if:true -->b<!-- /ko --></div>');
},
+ 'Should remove all nodes corresponding to a removed array item, even if they were added via containerless syntax and there are no other nodes': function() {
+ ko.bindingHandlers.test = {
+ init: function (element, valueAccessor) {
+ var value = valueAccessor();
+ ko.virtualElements.prepend(element, document.createTextNode(value));
+ }
+ };
+ ko.virtualElements.allowedBindings['test'] = true;
+
+ testNode.innerHTML = "x-<!--ko foreach: someitems--><!--ko test:$data--><!--/ko--><!--/ko-->";
+ var someitems = ko.observableArray(["aaa","bbb"]);
+ ko.applyBindings({ someitems: someitems }, testNode);
+ value_of(testNode).should_contain_text('x-aaabbb');
+
+ // Now remove items, and check the corresponding child nodes vanished
+ someitems.splice(1, 1);
+ value_of(testNode).should_contain_text('x-aaa');
+ },
+
'Should update all nodes corresponding to a changed array item, even if they were generated via containerless templates': function() {
testNode.innerHTML = "<div data-bind='foreach: someitems'><!-- ko if:true --><span data-bind='text: $data'></span><!-- /ko --></div>";
var someitems = [ ko.observable('A'), ko.observable('B') ];
View
135 spec/templatingBehaviors.js
@@ -66,7 +66,7 @@ var dummyTemplateEngine = function (templates) {
}
}
- if (options.bypassDomNodeWrap)
+ if (options.bypassDomNodeTransform)
return ko.utils.parseHtmlFragment(result);
else {
var node = document.createElement("div");
@@ -76,7 +76,10 @@ var dummyTemplateEngine = function (templates) {
node.removeChild(node.firstChild);
node.removeChild(node.lastChild);
- return [node];
+ // Convert the nodelist to an array to mimic what the default templating engine does, so we see the effects of not being able to remove dead memo comment nodes.
+ var renderedNodesArray = ko.utils.arrayPushAll([], node.childNodes);
+
+ return renderedNodesArray;
}
};
@@ -93,17 +96,17 @@ dummyTemplateEngine.prototype = new ko.templateEngine();
describe('Templating', {
before_each: function () {
ko.setTemplateEngine(new ko.nativeTemplateEngine());
- var existingNode = document.getElementById("templatingTarget");
+ var existingNode = document.getElementById("testNode");
if (existingNode != null)
existingNode.parentNode.removeChild(existingNode);
testNode = document.createElement("div");
- testNode.id = "templatingTarget";
+ testNode.id = "testNode";
document.body.appendChild(testNode);
},
'Template engines can return an array of DOM nodes': function () {
ko.setTemplateEngine(new dummyTemplateEngine({ x: [document.createElement("div"), document.createElement("span")] }));
- ko.renderTemplate("x", null, { bypassDomNodeWrap: true });
+ ko.renderTemplate("x", null, { bypassDomNodeTransform: true });
},
'Should not be able to render a template until a template engine is provided': function () {
@@ -118,7 +121,7 @@ describe('Templating', {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "ABC" }));
ko.renderTemplate("someTemplate", null, null, testNode);
value_of(testNode.childNodes.length).should_be(1);
- value_of(testNode.childNodes[0].innerHTML).should_be("ABC");
+ value_of(testNode.innerHTML).should_be("ABC");
},
'Should be able to access newly rendered/inserted elements in \'afterRender\' callaback': function () {
@@ -131,7 +134,7 @@ describe('Templating', {
var myModel = {};
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "ABC" }));
ko.renderTemplate("someTemplate", myModel, { afterRender: myCallback }, testNode);
- value_of(passedElement.innerHTML).should_be("ABC");
+ value_of(passedElement.nodeValue).should_be("ABC");
value_of(passedDataItem).should_be(myModel);
},
@@ -144,11 +147,11 @@ describe('Templating', {
ko.renderTemplate("someTemplate", null, null, testNode);
value_of(testNode.childNodes.length).should_be(1);
- value_of(testNode.childNodes[0].innerHTML).should_be("Value = A");
+ value_of(testNode.innerHTML).should_be("Value = A");
dependency("B");
value_of(testNode.childNodes.length).should_be(1);
- value_of(testNode.childNodes[0].innerHTML).should_be("Value = B");
+ value_of(testNode.innerHTML).should_be("Value = B");
},
'If the supplied data item is observable, evaluates it and has subscription on it': function () {
@@ -158,10 +161,10 @@ describe('Templating', {
}
}));
ko.renderTemplate("someTemplate", observable, null, testNode);
- value_of(testNode.childNodes[0].innerHTML).should_be("Value = A");
+ value_of(testNode.innerHTML).should_be("Value = A");
observable("B");
- value_of(testNode.childNodes[0].innerHTML).should_be("Value = B");
+ value_of(testNode.innerHTML).should_be("Value = B");
},
'Should stop updating DOM nodes when the dependency next changes if the DOM node has been removed from the document': function () {
@@ -171,26 +174,26 @@ describe('Templating', {
ko.renderTemplate("someTemplate", null, null, testNode);
value_of(testNode.childNodes.length).should_be(1);
- value_of(testNode.childNodes[0].innerHTML).should_be("Value = A");
+ value_of(testNode.innerHTML).should_be("Value = A");
testNode.parentNode.removeChild(testNode);
dependency("B");
value_of(testNode.childNodes.length).should_be(1);
- value_of(testNode.childNodes[0].innerHTML).should_be("Value = A");
+ value_of(testNode.innerHTML).should_be("Value = A");
},
'Should be able to render a template using data-bind syntax': function () {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "template output" }));
testNode.innerHTML = "<div data-bind='template:\"someTemplate\"'></div>";
ko.applyBindings(null, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>template output</div>");
+ value_of(testNode.childNodes[0].innerHTML).should_be("template output");
},
'Should be able to tell data-bind syntax which object to pass as data for the template (otherwise, uses viewModel)': function () {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "result = [js: childProp]" }));
testNode.innerHTML = "<div data-bind='template: { name: \"someTemplate\", data: someProp }'></div>";
ko.applyBindings({ someProp: { childProp: 123} }, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>result = 123</div>");
+ value_of(testNode.childNodes[0].innerHTML).should_be("result = 123");
},
'Should stop tracking inner observables immediately when the container node is removed from the document': function() {
@@ -211,7 +214,7 @@ describe('Templating', {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "result = [js: childProp]" }));
testNode.innerHTML = "<div data-bind='template: { name: templateSelectorFunction, data: someProp }'></div>";
ko.applyBindings({ someProp: { childProp: 123, myTemplate: "someTemplate" }, templateSelectorFunction: templatePicker }, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>result = 123</div>");
+ value_of(testNode.childNodes[0].innerHTML).should_be("result = 123");
},
'Should be able to chain templates, rendering one from inside another': function () {
@@ -221,7 +224,7 @@ describe('Templating', {
}));
testNode.innerHTML = "<div data-bind='template:\"outerTemplate\"'></div>";
ko.applyBindings(null, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>outer template output, <div>inner template output <span>123</span></div></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("outer template output, inner template output <span>123</span>");
},
'Should rerender chained templates when their dependencies change, without rerendering parent templates': function () {
@@ -233,12 +236,12 @@ describe('Templating', {
}));
testNode.innerHTML = "<div data-bind='template:\"outerTemplate\"'></div>";
ko.applyBindings(null, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>outer template output, <div>abc</div></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("outer template output, abc");
value_of(timesRenderedOuter).should_be(1);
value_of(timesRenderedInner).should_be(1);
observable("DEF");
- value_of(testNode.childNodes[0]).should_contain_html("<div>outer template output, <div>def</div></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("outer template output, def");
value_of(timesRenderedOuter).should_be(1);
value_of(timesRenderedInner).should_be(2);
},
@@ -260,19 +263,19 @@ describe('Templating', {
'Should handle data-bind attributes from inside templates, regardless of element and attribute casing': function () {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "<INPUT Data-Bind='value:\"Hi\"' />" }));
ko.renderTemplate("someTemplate", null, null, testNode);
- value_of(testNode.childNodes[0].childNodes[0].value).should_be("Hi");
+ value_of(testNode.childNodes[0].value).should_be("Hi");
},
'Should handle data-bind attributes that include newlines from inside templates': function () {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "<input data-bind='value:\n\"Hi\"' />" }));
ko.renderTemplate("someTemplate", null, null, testNode);
- value_of(testNode.childNodes[0].childNodes[0].value).should_be("Hi");
+ value_of(testNode.childNodes[0].value).should_be("Hi");
},
'Data binding syntax should be able to reference variables put into scope by the template engine': function () {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "<input data-bind='value:message' />" }));
ko.renderTemplate("someTemplate", null, { templateRenderingVariablesInScope: { message: "hello"} }, testNode);
- value_of(testNode.childNodes[0].childNodes[0].value).should_be("hello");
+ value_of(testNode.childNodes[0].value).should_be("hello");
},
'Data binding syntax should defer evaluation of variables until the end of template rendering (so bindings can take independent subscriptions to them)': function () {
@@ -280,7 +283,7 @@ describe('Templating', {
someTemplate: "<input data-bind='value:message' />[js: message = 'goodbye'; undefined; ]"
}));
ko.renderTemplate("someTemplate", null, { templateRenderingVariablesInScope: { message: "hello"} }, testNode);
- value_of(testNode.childNodes[0].childNodes[0].value).should_be("goodbye");
+ value_of(testNode.childNodes[0].value).should_be("goodbye");
},
'Data binding syntax should use the template\'s \'data\' object as the viewModel value (so \'this\' is set correctly when calling click handlers etc.)': function() {
@@ -292,7 +295,7 @@ describe('Templating', {
someFunctionOnModel : function() { this.didCallMyFunction = true }
};
ko.renderTemplate("someTemplate", viewModel, null, testNode);
- var buttonNode = testNode.childNodes[0].childNodes[0];
+ var buttonNode = testNode.childNodes[0];
value_of(buttonNode.tagName).should_be("BUTTON"); // Be sure we're clicking the right thing
buttonNode.click();
value_of(viewModel.didCallMyFunction).should_be(true);
@@ -329,7 +332,7 @@ describe('Templating', {
'Data binding syntax should support \'foreach\' option, whereby it renders for each item in an array but doesn\'t rerender everything if you push or splice': function () {
var myArray = new ko.observableArray([{ personName: "Bob" }, { personName: "Frank"}]);
- ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "The item is [js: personName]" }));
+ ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "<div>The item is [js: personName]</div>" }));
testNode.innerHTML = "<div data-bind='template: { name: \"itemTemplate\", foreach: myCollection }'></div>";
ko.applyBindings({ myCollection: myArray }, testNode);
@@ -349,7 +352,7 @@ describe('Templating', {
testNode.innerHTML = "<div data-bind='template: { name: \"itemTemplate\", foreach: myCollection }'></div>";
ko.applyBindings({ myCollection: myArray }, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>the item is <span>bob</span></div><div>the item is <span>frank</span></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("the item is <span>bob</span>the item is <span>frank</span>");
},
'Data binding \'foreach\' options should only bind each group of output nodes once': function() {
@@ -362,13 +365,49 @@ describe('Templating', {
value_of(initCalls).should_be(3); // 3 because there were 3 items in myCollection
},
+ 'Data binding \'foreach\' should handle templates in which the very first node has a binding': function() {
+ // Represents https://github.com/SteveSanderson/knockout/pull/440
+ // Previously, the rewriting (which introduces a comment node before the bound node) was interfering
+ // with the array-to-DOM-node mapping state tracking
+ ko.setTemplateEngine(new dummyTemplateEngine({ mytemplate: "<div data-bind='text: $data'></div>" }));
+ testNode.innerHTML = "<div data-bind=\"template: { name: 'mytemplate', foreach: items }\"></div>";
+
+ // Bind against initial array containing one entry. UI just shows "original"
+ var myArray = ko.observableArray(["original"]);
+ ko.applyBindings({ items: myArray });
+ value_of(testNode.childNodes[0]).should_contain_html("<div>original</div>");
+
+ // Now replace the entire array contents with one different entry.
+ // UI just shows "new" (previously with bug, showed "original" AND "new")
+ myArray(["new"]);
+ value_of(testNode.childNodes[0]).should_contain_html("<div>new</div>");
+ },
+
+ 'Data binding \'foreach\' should handle chained templates in which the very first node has a binding': function() {
+ // See https://github.com/SteveSanderson/knockout/pull/440 and https://github.com/SteveSanderson/knockout/pull/144
+ ko.setTemplateEngine(new dummyTemplateEngine({
+ outerTemplate: "<div data-bind='text: $data'></div>[renderTemplate:innerTemplate]x", // [renderTemplate:...] is special syntax supported by dummy template engine
+ innerTemplate: "inner <span data-bind='text: 123'></span>"
+ }));
+ testNode.innerHTML = "<div data-bind=\"template: { name: 'outerTemplate', foreach: items }\"></div>";
+
+ // Bind against initial array containing one entry.
+ var myArray = ko.observableArray(["original"]);
+ ko.applyBindings({ items: myArray });
+ value_of(testNode.childNodes[0]).should_contain_html("<div>original</div>inner <span>123</span>x");
+
+ // Now replace the entire array contents with one different entry.
+ myArray(["new"]);
+ value_of(testNode.childNodes[0]).should_contain_html("<div>new</div>inner <span>123</span>x");
+ },
+
'Data binding \'foreach\' option should apply bindings with an $index in the context': function () {
var myArray = new ko.observableArray([{ personName: "Bob" }, { personName: "Frank"}]);
ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "The item # is <span data-bind='text: $index'></span>" }));
testNode.innerHTML = "<div data-bind='template: { name: \"itemTemplate\", foreach: myCollection }'></div>";
ko.applyBindings({ myCollection: myArray }, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>the item # is <span>0</span></div><div>the item # is <span>1</span></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("the item # is <span>0</span>the item # is <span>1</span>");
},
'Data binding \'foreach\' option should update bindings that reference an $index if the list changes': function () {
@@ -377,13 +416,13 @@ describe('Templating', {
testNode.innerHTML = "<div data-bind='template: { name: \"itemTemplate\", foreach: myCollection }'></div>";
ko.applyBindings({ myCollection: myArray }, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>the item <span>bob</span>is <span>0</span></div><div>the item <span>frank</span>is <span>1</span></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("the item <span>bob</span>is <span>0</span>the item <span>frank</span>is <span>1</span>");
var frank = myArray.pop(); // remove frank
- value_of(testNode.childNodes[0]).should_contain_html("<div>the item <span>bob</span>is <span>0</span></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("the item <span>bob</span>is <span>0</span>");
myArray.unshift(frank); // put frank in the front
- value_of(testNode.childNodes[0]).should_contain_html("<div>the item <span>frank</span>is <span>0</span></div><div>the item <span>bob</span>is <span>1</span></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("the item <span>frank</span>is <span>0</span>the item <span>bob</span>is <span>1</span>");
},
@@ -393,13 +432,13 @@ describe('Templating', {
testNode.innerHTML = "<div data-bind='template: { name: \"itemTemplate\", foreach: myCollection }'></div>";
ko.applyBindings({ myCollection: myArray }, testNode);
- value_of(testNode.childNodes[0]).should_contain_html("<div>the item is <span>undefined</span></div><div>the item is <span>null</span></div>");
+ value_of(testNode.childNodes[0]).should_contain_html("the item is <span>undefined</span>the item is <span>null</span>");
},
'Data binding \'foreach\' option should update DOM nodes when a dependency of their mapping function changes': function() {
var myObservable = new ko.observable("Steve");
var myArray = new ko.observableArray([{ personName: "Bob" }, { personName: myObservable }, { personName: "Another" }]);
- ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "The item is [js: ko.utils.unwrapObservable(personName)]" }));
+ ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "<div>The item is [js: ko.utils.unwrapObservable(personName)]</div>" }));
testNode.innerHTML = "<div data-bind='template: { name: \"itemTemplate\", foreach: myCollection }'></div>";
ko.applyBindings({ myCollection: myArray }, testNode);
@@ -462,7 +501,7 @@ describe('Templating', {
'Data binding syntax should omit any items whose \'_destroy\' flag is set (unwrapping the flag if it is observable)' : function() {
var myArray = new ko.observableArray([{ someProp: 1 }, { someProp: 2, _destroy: 'evals to true' }, { someProp : 3 }, { someProp: 4, _destroy: ko.observable(false) }]);
- ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "someProp=[js: someProp]" }));
+ ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "<div>someProp=[js: someProp]</div>" }));
testNode.innerHTML = "<div data-bind='template: { name: \"itemTemplate\", foreach: myCollection }'></div>";
ko.applyBindings({ myCollection: myArray }, testNode);
@@ -471,7 +510,7 @@ describe('Templating', {
'Data binding syntax should include any items whose \'_destroy\' flag is set if you use includeDestroyed' : function() {
var myArray = new ko.observableArray([{ someProp: 1 }, { someProp: 2, _destroy: 'evals to true' }, { someProp : 3 }]);
- ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "someProp=[js: someProp]" }));
+ ko.setTemplateEngine(new dummyTemplateEngine({ itemTemplate: "<div>someProp=[js: someProp]</div>" }));
testNode.innerHTML = "<div data-bind='template: { name: \"itemTemplate\", foreach: myCollection, includeDestroyed: true }'></div>";
ko.applyBindings({ myCollection: myArray }, testNode);
@@ -522,9 +561,9 @@ describe('Templating', {
var viewModel = { myProp: ko.observable({ childProp: 'abc' }) };
ko.applyBindings(viewModel, testNode);
- value_of(testNode.childNodes[0].childNodes[0]).should_contain_text("Value: abc");
- value_of(testNode.childNodes[0].childNodes[1]).should_contain_text("Value: abc");
- value_of(testNode.childNodes[0].childNodes[2]).should_contain_text("Value: abc");
+ value_of(testNode.childNodes[0].childNodes[0].nodeValue).should_be("Value: abc");
+ value_of(testNode.childNodes[0].childNodes[1].nodeValue).should_be("Value: abc");
+ value_of(testNode.childNodes[0].childNodes[2].nodeValue).should_be("Value: abc");
// Causing the condition to become false causes the output to be removed
viewModel.myProp(null);
@@ -532,21 +571,21 @@ describe('Templating', {
// Causing the condition to become true causes the output to reappear
viewModel.myProp({ childProp: 'def' });
- value_of(testNode.childNodes[0].childNodes[0]).should_contain_text("Value: def");
- value_of(testNode.childNodes[0].childNodes[1]).should_contain_text("Value: def");
- value_of(testNode.childNodes[0].childNodes[2]).should_contain_text("Value: def");
+ value_of(testNode.childNodes[0].childNodes[0].nodeValue).should_be("Value: def");
+ value_of(testNode.childNodes[0].childNodes[1].nodeValue).should_be("Value: def");
+ value_of(testNode.childNodes[0].childNodes[2].nodeValue).should_be("Value: def");
},
'Should be able to populate checkboxes from inside templates, despite IE6 limitations': function () {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "<input type='checkbox' data-bind='checked:isChecked' />" }));
ko.renderTemplate("someTemplate", null, { templateRenderingVariablesInScope: { isChecked: true } }, testNode);
- value_of(testNode.childNodes[0].childNodes[0].checked).should_be(true);
+ value_of(testNode.childNodes[0].checked).should_be(true);
},
'Should be able to populate radio buttons from inside templates, despite IE6 limitations': function () {
ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: "<input type='radio' name='somename' value='abc' data-bind='checked:someValue' />" }));
ko.renderTemplate("someTemplate", null, { templateRenderingVariablesInScope: { someValue: 'abc' } }, testNode);
- value_of(testNode.childNodes[0].childNodes[0].checked).should_be(true);
+ value_of(testNode.childNodes[0].checked).should_be(true);
},
'Should be able to render a different template for each array entry by passing a function as template name': function() {
@@ -555,8 +594,8 @@ describe('Templating', {
{ preferredTemplate: 2, someProperty: 'secondItemValue' }
]);
ko.setTemplateEngine(new dummyTemplateEngine({
- firstTemplate: "Template1Output, [js:someProperty]",
- secondTemplate: "Template2Output, [js:someProperty]"
+ firstTemplate: "<div>Template1Output, [js:someProperty]</div>",
+ secondTemplate: "<div>Template2Output, [js:someProperty]</div>"
}));
testNode.innerHTML = "<div data-bind='template: {name: getTemplateModelProperty, foreach: myCollection}'></div>";
@@ -575,7 +614,7 @@ describe('Templating', {
{ name: "Beta" }
])
};
- ko.setTemplateEngine(new dummyTemplateEngine({myTemplate: "Person [js:name] has additional property [js:templateOptions.myAdditionalProp]"}));
+ ko.setTemplateEngine(new dummyTemplateEngine({myTemplate: "<div>Person [js:name] has additional property [js:templateOptions.myAdditionalProp]</div>"}));
testNode.innerHTML = "<div data-bind='template: {name: \"myTemplate\", foreach: people, templateOptions: someAdditionalData }'></div>";
ko.applyBindings(myModel, testNode);
@@ -592,7 +631,7 @@ describe('Templating', {
ko.applyBindings(myModel, testNode);
// Right now the template references myObservable, so there should be exactly one subscription on it
- value_of(testNode.childNodes[0]).should_contain_html("<div>the value is some value</div>");
+ value_of(testNode.childNodes[0].innerHTML).should_be("The value is some value");
value_of(myModel.myObservable.getSubscriptionsCount()).should_be(1);
// By changing unrelatedObservable, we force the data-bind value to be re-evaluated, setting up a new template subscription,
@@ -640,7 +679,7 @@ describe('Templating', {
}
}
}, testNode);
- value_of(testNode.childNodes[0].childNodes[0].childNodes[0].childNodes[0].childNodes[0]).should_contain_text("(Data:INNER, Parent:MIDDLE, Grandparent:OUTER, Root:ROOT, Depth:3)");
+ value_of(testNode.childNodes[0].childNodes[0]).should_contain_text("(Data:INNER, Parent:MIDDLE, Grandparent:OUTER, Root:ROOT, Depth:3)");
},
'Should not be allowed to rewrite templates that embed anonymous templates': function() {
@@ -691,7 +730,7 @@ describe('Templating', {
ko.setTemplateEngine(new dummyTemplateEngine());
testNode.innerHTML = "Start <!-- ko template: { data: someData } -->Childprop: [js: childProp]<!-- /ko --> End";
ko.applyBindings({ someData: { childProp: 'abc' } }, testNode);
- value_of(testNode).should_contain_html("start <!-- ko template: { data: somedata } --><div>childprop: abc</div><!-- /ko -->end");
+ value_of(testNode).should_contain_html("start <!-- ko template: { data: somedata } -->childprop: abc<!-- /ko -->end");
},
'Should be able to use anonymous templates that contain first-child comment nodes': function() {
View
31 src/binding/editDetection/arrayToDomNodeChildren.js
@@ -10,13 +10,26 @@
// "callbackAfterAddingNodes" will be invoked after any "mapping"-generated nodes are inserted into the container node
// You can use this, for example, to activate bindings on those nodes.
- function fixUpVirtualElements(contiguousNodeArray) {
- // Ensures that contiguousNodeArray really *is* an array of contiguous siblings, even if some of the interior
- // ones have changed since your array was first built (e.g., because your array contains virtual elements, and
- // their virtual children changed when binding was applied to them).
- // This is needed so that we can reliably remove or update the nodes corresponding to a given array item
-
- if (contiguousNodeArray.length > 2) {
+ function fixUpNodesToBeRemoved(contiguousNodeArray) {
+ // Before deleting or replacing a set of nodes that were previously outputted by the "map" function, we have to reconcile
+ // them against what is in the DOM right now. It may be that some of the nodes have already been removed from the document,
+ // or that new nodes might have been inserted in the middle, for example by a binding. Also, there may previously have been
+ // leading comment nodes (created by rewritten string-based templates) that have since been removed during binding.
+ // So, this function translates the old "map" output array into its best guess of what set of current DOM nodes should be removed.
+ //
+ // Rules:
+ // [A] Any leading nodes that aren't in the document any more should be ignored
+ // These most likely correspond to memoization nodes that were already removed during binding
+ // See https://github.com/SteveSanderson/knockout/pull/440
+ // [B] We want to output a contiguous series of nodes that are still in the document. So, ignore any nodes that
+ // have already been removed, and include any nodes that have been inserted among the previous collection
+
+ // Rule [A]
+ while (contiguousNodeArray.length && !ko.utils.domNodeIsAttachedToDocument(contiguousNodeArray[0]))
+ contiguousNodeArray.splice(0, 1);
+
+ // Rule [B]
+ if (contiguousNodeArray.length > 1) {
// Build up the actual new contiguous node set
var current = contiguousNodeArray[0], last = contiguousNodeArray[contiguousNodeArray.length - 1], newContiguousSet = [current];
while (current !== last) {
@@ -40,7 +53,7 @@
// On subsequent evaluations, just replace the previously-inserted DOM nodes
if (mappedNodes.length > 0) {
- fixUpVirtualElements(mappedNodes);
+ fixUpNodesToBeRemoved(mappedNodes);
ko.utils.replaceDomNodes(mappedNodes, newMappedNodes);
if (callbackAfterAddingNodes)
callbackAfterAddingNodes(valueToMap, newMappedNodes);
@@ -89,7 +102,7 @@
lastMappingResult[lastMappingResultIndex].dependentObservable.dispose();
// Queue these nodes for later removal
- fixUpVirtualElements(lastMappingResult[lastMappingResultIndex].domNodes);
+ fixUpNodesToBeRemoved(lastMappingResult[lastMappingResultIndex].domNodes);
ko.utils.arrayForEach(lastMappingResult[lastMappingResultIndex].domNodes, function (node) {
nodesToDelete.push({
element: node,
Something went wrong with that request. Please try again.