Knockout: Strange error with custom bindingHandler in Knockout 3.5.0 (virtual foreach) in IE11 causing children to stop evaluating

[Zallist]

I have a custom bindingHandler which implements a wrapper around the foreach binding, in essence making it only render a subset of the items in the array, and showing the additional items as the scroll position of the parent is moved. This has worked fine since Knockout 3.0.0.
After upgrading to Knockout 3.5.0, the binding now throws an error when the computed it reads from is notified of a change...sometimes. In IE11 only.
The exact replication seems to be loading the page, waiting a second, and then typing a number "1" into the textbox. Once the error's happened, the binding stops evaluating anything inside of it. So scrolling down no longer appends new items to the list.

Here is a JSFiddle of the binding in action with Knockout 3.5.0: https://jsfiddle.net/hts79y4p/3/
Here is the same JSFiddle with Knockout 3.4.2: https://jsfiddle.net/hts79y4p/4/

The error thrown is incredibly useful. Literally "Unspecified Error".

The line of code it's thrown on is to do with inserting a node in a virtualElement.

This seems to have been caused by commit https://github.com/knockout/knockout/commit/e0b07f6e17b44f3df44fee384a6065eae15af216.

I have found a fix (since I need this to work) which is to adopt the node before inserting it. appendChild on the containerNode.parentNode also works, but adoptNode should be more performent.

ko.utils.insertAfter beforefix:

insertAfter: function(containerNode, nodeToInsert, insertAfterNode) {
    if (!insertAfterNode) {
        ko.virtualElements.prepend(containerNode, nodeToInsert);
    } else if (!isStartComment(containerNode)) {
        // Insert after insertion point
        if (insertAfterNode.nextSibling)
            containerNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);
        else
            containerNode.appendChild(nodeToInsert);
    } else {
        // Children of start comments must always have a parent and at least one following sibling (the end comment)
        containerNode.parentNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);
    }
}

ko.utils.insertAfter afterfix:

insertAfter: function(containerNode, nodeToInsert, insertAfterNode) {
    if (containerNode.ownerDocument && containerNode.ownerDocument.adoptNode)
        containerNode.ownerDocument.adoptNode(nodeToInsert);

    if (!insertAfterNode) {
        ko.virtualElements.prepend(containerNode, nodeToInsert);
    } else if (!isStartComment(containerNode)) {
        // Insert after insertion point
        if (insertAfterNode.nextSibling)
            containerNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);
        else
            containerNode.appendChild(nodeToInsert);
    } else {
        // Children of start comments must always have a parent and at least one following sibling (the end comment)
        containerNode.parentNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);
    }
}

I'm not raising this as a pull request since I have no idea why IE11 is throwing this error, or what the performance implications of adopting every node to the current document are.
I can confirm that, according to the browser, nodeToInsert already shares the same ownerDocument as containerNode.parentNode, which removes the possibility of just performing an if check for when the owner is different.

If my virtualForeach bindingHandler is doing the wrong thing somehow, or there's a better way of doing it, then that'd be an alternative fix too.

[Zallist]

Seems this error is caused by Internet Explorer just throwing an error when insertBefore is called on an element that already has a parent. So a cleaner solution is to just detach the element before it's inserted.

JSFiddle with code changed: https://jsfiddle.net/Le09rxfh/

Exact code change:

        prepend: function(containerNode, nodeToPrepend) {
        if (nodeToPrepend.parentNode)
    nodeToPrepend.parentNode.removeChild(nodeToPrepend);

            if (!isStartComment(containerNode)) {
                if (containerNode.firstChild)
                    containerNode.insertBefore(nodeToPrepend, containerNode.firstChild);
                else
                    containerNode.appendChild(nodeToPrepend);
            } else {
                // Start comments must always have a parent and at least one following sibling (the end comment)
                containerNode.parentNode.insertBefore(nodeToPrepend, containerNode.nextSibling);
            }
        },

        insertAfter: function(containerNode, nodeToInsert, insertAfterNode) {
        if (nodeToInsert.parentNode)
    nodeToInsert.parentNode.removeChild(nodeToInsert);

            if (!insertAfterNode) {
                ko.virtualElements.prepend(containerNode, nodeToInsert);
            } else if (!isStartComment(containerNode)) {
                // Insert after insertion point
                if (insertAfterNode.nextSibling)
                    containerNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);
                else
                    containerNode.appendChild(nodeToInsert);
            } else {
                // Children of start comments must always have a parent and at least one following sibling (the end comment)
                containerNode.parentNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);
            }
        },

This could potentially be a problem in other parts of the codebase,where elements are moved rather than created.

[chrisknoll]

I'm reading up on what insertBefore does: https://www.w3schools.com/jsref/met_node_insertbefore.asp

And I don't understand IE11's behavior:

Seems this error is caused by Internet Explorer just throwing an error when insertBefore is called on an element that already has a parent.

So you do this:

if (nodeToPrepend.parentNode)
    nodeToPrepend.parentNode.removeChild(nodeToPrepend);

To ensure the node doesn't have a parent. But, how can this work per the documentation:
https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_node_insertbefore2

Does this example fail on IE11?

If so, report the bug to IE team (which I think they abandoned IE11). I agree with you that dom elements being moved around (jquery does this alot for their jqueryui and I think bootstrap would do this as well) is something that is a typical usecase, so removing the node from it's parent could be a breaking change.

[Zallist]

Works perfectly fine in ie11. Bizarre.
Bit more in investigation shows that nodeToInsert doesn't actually have its parentNode changed by the operation in Knockout.
The operation also works 100% of the time if stepping through line by line and checking the parentNode at each step. Which sounds exactly like this is a bug in ie11.

[fastfasterfastest]

I think the problem is that ko.virtualElements.insertAfter does not handle the case when
nodeToInsert and insertAfterNode.nextSibling is the same node, or perhaps more likely, that ko.virtualElements.insertAfter gets called with nodeToInsert and insertAfterNode.nextSibling being the same node,
This happens in your sample when more than one element remains in the array (at a different position.) This can be replicated without your custom binding as well (added: https://jsfiddle.net/fastfasterfastest/6p30be5m), so I think it might be a genuine bug.

insertAfter: function(containerNode, nodeToInsert, insertAfterNode) {
    if (!insertAfterNode) {
        ko.virtualElements.prepend(containerNode, nodeToInsert);
    } else if (!isStartComment(containerNode)) {
        // Insert after insertion point
        if (insertAfterNode.nextSibling)
            containerNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);
        else
            containerNode.appendChild(nodeToInsert);
    } else {
        // Children of start comments must always have a parent and at least one following sibling (the end comment)
        containerNode.parentNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);
    }
},

If you change

        // Children of start comments must always have a parent and at least one following sibling (the end comment)
        containerNode.parentNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);

to

        // Children of start comments must always have a parent and at least one following sibling (the end comment)
        if (nodeToInsert !== insertAfterNode.nextSibling) containerNode.parentNode.insertBefore(nodeToInsert, insertAfterNode.nextSibling);

then your sample appear to work in IE11 as well.
I can imagine this is not the real/final fix, e.g. the test should probably be applied in case containerNode is not a comment node. Or perhaps more likely, ko.virtualElements.insertAfter shouldn't be called in the first place - ko.utils.setDomNodeChildrenFromArrayMapping might need some adjustment.

[fastfasterfastest]

Here is an ultra-simple sample that demonstrates the bug: https://jsfiddle.net/fastfasterfastest/h4vu3w8b/

[mbest]

Thanks for the work narrowing this down. I'll look into it soon.

[mbest]

Unfortunately, this bug is hard to reproduce. I tried @fastfasterfastest's sample in IE and saw the problem once. Now I can't get it to fail, or any of the other samples.

[Zallist]

I'm able to replicate it consistently with @fastfasterfastest 's example.
If you click Trigger Bug within a couple seconds, it won't error. If you wait around 3 seconds, then it errors every single time.

[mbest]

Thanks. Waiting 3 seconds seems to do the trick.

[fastfasterfastest]

I feel left out... I cannot trigger the bug anymore, my own sample does not reproduce the issue now for me, even after waiting a few seconds. Wacky - what has changed?? I don't think Edge has been updated.
(Added: well, I was using the wrong browser, I was using Edge and couldn't replicate the issue, but in Edge the issue never caused an error. Using IE11 I can still replicate it.)

Regardless, it seems that the following is still true:

ko.virtualElements.insertAfter gets called with nodeToInsert and insertAfterNode.nextSibling being the same node

And that, I think, is what caused/causes the issue.

[mbest]

Thanks for all the help tracking this down. I've uploaded a fix.

[mKlus]

@mbest We're hitting the issue as well quite often in our webapp. Any chance you could release 3.5.1-rc with the fix any time soon please?

[fastfasterfastest]

@mKlus In the meantime you should be able to avoid the error in IE11 by doing the following after knockout has been loaded:

(function(){
   var origInsertAfter = ko.virtualElements.insertAfter;
   ko.virtualElements.insertAfter = function(containerNode, nodeToInsert, insertAfterNode){
      if (insertAfterNode && nodeToInsert === insertAfterNode.nextSibling) return;
      origInsertAfter.call(this, containerNode, nodeToInsert, insertAfterNode);
   };
})();

[mbest]

@fastfasterfastest @mKlus Here's a more complete workaround that updates the minified function name and also fixes prepend:

if (ko.version === "3.5.0") {
    (function(){
        var origInsertAfter = ko.virtualElements.insertAfter;
        ko.virtualElements.Vb = ko.virtualElements.insertAfter = function(containerNode, nodeToInsert, insertAfterNode) {
            if (insertAfterNode && nodeToInsert === insertAfterNode.nextSibling) return;
            origInsertAfter(containerNode, nodeToInsert, insertAfterNode);
        };
        var origPrepend = ko.virtualElements.prepend;
        ko.virtualElements.Uc = ko.virtualElements.prepend = function(containerNode, nodeToPrepend) {
            if (nodeToPrepend === ko.virtualElements.firstChild(containerNode)) return;
            origPrepend(containerNode, nodeToPrepend);
        };
    })();
}

[mKlus]

Thanks @fastfasterfastest @mbest for the workaround. I've added @mbest 's workaround in and that fixed the issue.
Hopefully 3.5.1 will be released soon as well :)

[KingR1]

Hi. I have a similar error for IE 11, but it seems like in another place.

setDomNodeChildren: function (domNode, childNodes) {
     ko.utils.emptyDomNode(domNode);
     if (childNodes) {
         for (var i = 0, j = childNodes.length; i < j; i++)
             domNode.appendChild(childNodes[i]); //here is Error WrongDocumentError
    }
}

So basically my components not working. I've updated knockout to version 3.5.1 - this not fixed.

[mbest]

@KingR1 Is this something that worked in a previous version of Knockout?

[KingR1]

@mbest so basically yes, it was working before. But I cannot find reason for that. Maybe there were some updates to IE11 and now it is not working properly. I'm using version 3.4.1. I tried to update to 3.5.1, but issue didn't go.

I've checked ownerDocument for domNode and childNodes[i] - not sure, but seems like it is different documents (at least different objects). Maybe that why IE cannot add child items. But if I'm correct, why ownerDocuments are different.

[KingR1]

Ok, it's kinda fixed when adopting these child elements into the document, cause child elements aren't attached to the DOM. Checked with util function ko.utils.domNodeIsAttachedToDocument

setDomNodeChildren: function (domNode, childNodes) {
    ko.utils.emptyDomNode(domNode);
    if (childNodes) {
        for (var i = 0, j = childNodes.length; i < j; i++)
            if(ko.utils.domNodeIsAttachedToDocument(childNodes[i]))
                domNode.appendChild(childNodes[i]);
        else
        domNode.appendChild(document.adoptNode(childNodes[i]));
    }
}

But don't know if it has some weak places...

---

P.S. Still cannot reproduce that issue out of my project...

[KingR1]

And similar update can be done here

setDomNodeChildren: function(node, childNodes) {
            if (!isStartComment(node))
                ko.utils.setDomNodeChildren(node, childNodes);
            else {
                ko.virtualElements.emptyNode(node);
                var endCommentNode = node.nextSibling; // Must be the next sibling, as we just emptied the children
                for (var i = 0, j = childNodes.length; i < j; i++)
                    endCommentNode.parentNode.insertBefore(childNodes[i], endCommentNode); //here is thrown error
            }
        }

also adopt child items

[KingR1]

@mbest Hi! Can my above comments be somehow included into some version? Cause I don't know how to workaround it... and cannot reproduce issue out of my project :(

[KingR1]

@mbest Any updates? Or I shouldn't expect it at all?

[mbest]

@KingR1 If you can provide a way for me to reproduce the problem, I can start looking at it.

[Jay-Ben]

I'm using latest Knockout.js version (3.5.1)

Still getting following error on reloading the toolbox after removing one of the tool Item :