Allow a cleaned comment node to remove "children" fixes #1740

knockout · Nov 26, 2017 · 7c2a339 · 7c2a339
1 parent 34b4123
commit 7c2a339
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 5 deletions.
diff --git a/spec/domNodeDisposalBehaviors.js b/spec/domNodeDisposalBehaviors.js
@@ -67,6 +67,30 @@ describe('DOM node disposal', function() {
         expect(grandChildSpy).not.toHaveBeenCalled();
     });
 
+    it('Should not clean nodes that are removed by a comment dispose handler', function() {
+        var childNode = document.createComment("ko comment");
+        var grandChildNode = document.createElement("DIV");
+        var childNode2 = document.createComment("ko comment");
+        var childSpy = jasmine.createSpy('childSpy')
+            .andCallFake(function() {
+                testNode.removeChild(grandChildNode);
+            });
+        var grandChildSpy = jasmine.createSpy('grandChildSpy');
+        var child2Spy = jasmine.createSpy('child2Spy');
+
+        testNode.appendChild(childNode);
+        testNode.appendChild(grandChildNode);
+        testNode.appendChild(childNode2);
+        ko.utils.domNodeDisposal.addDisposeCallback(childNode, childSpy);
+        ko.utils.domNodeDisposal.addDisposeCallback(grandChildNode, grandChildSpy);
+        ko.utils.domNodeDisposal.addDisposeCallback(childNode2, child2Spy);
+
+        ko.cleanNode(testNode);
+        expect(childSpy).toHaveBeenCalledWith(childNode);
+        expect(grandChildSpy).not.toHaveBeenCalled();
+        expect(child2Spy).toHaveBeenCalledWith(childNode2);
+    });
+
     it('Should throw an error if a cleaned node is removed in a handler', function() {
         // Test by removing the node itself
         var childNode = document.createElement("DIV");
@@ -84,6 +108,14 @@ describe('DOM node disposal', function() {
             testNode.removeChild(childNode);
         });
         expect(function() { ko.cleanNode(testNode); }).toThrowContaining("cleaned node was removed");
+
+        // Test by removing a comment node
+        var childNode = document.createComment("ko comment");
+        testNode.appendChild(childNode);
+        ko.utils.domNodeDisposal.addDisposeCallback(childNode, function() {
+            testNode.removeChild(childNode);
+        });
+        expect(function() { ko.cleanNode(testNode); }).toThrowContaining("cleaned node was removed");
     });
 
     it('Should be able to attach disposal callback to a node that has been cloned', function() {

diff --git a/src/utils.domNodeDisposal.js b/src/utils.domNodeDisposal.js
@@ -38,11 +38,15 @@ ko.utils.domNodeDisposal = new (function () {
     }
 
     function cleanImmediateCommentTypeChildren(nodeWithChildren) {
-        var child, nextChild = nodeWithChildren.firstChild;
-        while (child = nextChild) {
-            nextChild = child.nextSibling;
-            if (child.nodeType === 8)
-                cleanSingleNode(child);
+        var children = nodeWithChildren.childNodes;
+        var cleanedNode;
+        for (var i = 0; i < children.length; i++) {
+            if (children[i].nodeType === 8) {
+                cleanSingleNode(cleanedNode = children[i]);
+                if (children[i] !== cleanedNode) {
+                    throw Error("ko.cleanNode: An already cleaned node was removed from the document");
+                }
+            }
         }
     }