New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error when inserting node during render: "Failed to execute 'insertBefore' on 'Node'" #2128

Closed
JacksonJN opened this Issue Apr 19, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@JacksonJN
Copy link

JacksonJN commented Apr 19, 2018

Expected Behavior

When rendering there should be no errors and all the elements should be rendered in the correct order.

Current Behavior

When inserting a node during the render process, if nextSibling is not a proper child of parent then the following error occurs:

DOMException: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.

This happens in the following situations that I have found. In all cases nextSibling.parentNode becomes null:

  1. In updateNodes when the keys between the compared nodes match while traversing the nodes backwards.
    • nextSibling is assigned to o.dom of the element that got updated. o.dom can be removed from the dom if the tags are different, causing o.dom.parentNode to be null. nextSibling is set to o.dom causing the error if insertNode is eventually called with the same nextSibling.

      mithril.js/render/render.js

      Lines 314 to 318 in ec43ca0

      else if (o.key === v.key) {
      updateNode(parent, o, v, hooks, getNextSibling(old, oldEnd + 1, nextSibling), ns)
      if (o.dom != null) nextSibling = o.dom
      oldEnd--, end--
      } else {

      mithril.js/render/render.js

      Lines 322 to 328 in ec43ca0

      if (oldIndex != null) {
      o = old[oldIndex]
      updateNode(parent, o, v, hooks, getNextSibling(old, oldEnd + 1, nextSibling), ns)
      insertNode(parent, toFragment(v), nextSibling)
      o.skip = true
      if (o.dom != null) nextSibling = o.dom
      } else {
    • Additionally, this can happen when there are more old nodes than new nodes. In this case the call to updateNode the next sibling is determined by calling getNextSibling with the old nodes. If an old node exists but was removed it could return that node which would be set to nextSibling.

      mithril.js/render/render.js

      Lines 462 to 467 in ec43ca0

      function getNextSibling(vnodes, i, nextSibling) {
      for (; i < vnodes.length; i++) {
      if (vnodes[i] != null && vnodes[i].dom != null) return vnodes[i].dom
      }
      return nextSibling
      }
  2. In updateNodes when keys do not match between compared nodes while traversing backwards:
    • nextSibling is set to the node that gets created.

      mithril.js/render/render.js

      Lines 328 to 330 in ec43ca0

      } else {
      var dom = createNode(parent, v, hooks, ns, nextSibling)
      nextSibling = dom

      But that node can be a fragment and parentNode for a fragment is also null.
      function createFragment(parent, vnode, hooks, ns, nextSibling) {
      var fragment = $doc.createDocumentFragment()
      if (vnode.children != null) {
      var children = vnode.children
      createNodes(fragment, children, 0, children.length, hooks, null, ns)
      }
      vnode.dom = fragment.firstChild
      vnode.domSize = fragment.childNodes.length
      insertNode(parent, fragment, nextSibling)
      return fragment
      }

Those are the only cases where nextSibling is set to a value without a parent as far as I could tell.

This commit: 02aab65 removed the check for nextSibling.parentNode inside insertNode. This is causing the underlying issues to not be hidden. Whereas before the commit the error wasn't present, there still was issues with ordering of the rendered elements since the check caused appendChild to be called instead of insertBefore.

Possible Solution

In the first case above the old element o.dom was removed. Then it's assigned to nextSibling. Instead I think v.dom should be the next sibling since it is the element that is on the dom.

updateNode(parent, o, v, hooks, getNextSibling(old, oldEnd + 1, nextSibling), ns)
nextSibling = v.dom

getNextSibling should be updated to not return a dom element if it doesn't have a parent. That way it doesn't set nextSibling to a removed node.

function getNextSibling(vnodes, i, 
    for (; i < vnodes.length; 
        if (vnodes[i] != null && vnodes[i].dom != null && vnodes[i].dom.parentNode != null) return vnodes[i].dom
    }
    return nextSibling
}

And for the second case the created fragment is set to nextSibling. Instead v.dom should be used since the fragment itself isn't a part of the dom. v.dom is already set to the first child of the fragment.

createNode(parent, v, hooks, ns, nextSibling)
nextSibling = v.dom

I did some testing with the above changes and it appears to fix the error successfully and keep the correct order of elements. But I'm not too familiar with the code so I'm looking for some feedback.

I'd be happy to create the PR with these fixes if they're correct.

Steps to Reproduce (for bugs)

Simple steps to reproduce the first case:

  1. Render node:
    • Tag "div" with key "2".
  2. Render nodes:
    • Tag "span" with key "1".
    • Tag "span" with key "2".

Code snippet: Flems

Your Environment

  • Version used: Branch next. Error doesn't occur in v1.1.6 or below because it doesn't include mentioned commit.
  • Browser Name and version: All
  • Operating System and version (desktop or mobile): Windows 10 desktop
  • Link to your project: NA

@pygy pygy added the bug label Apr 20, 2018

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 20, 2018

Great catch, thanks a lot. I hadn't thought of that scenario.

Your analysis seems pretty good, but it's getting too late here for me to think it through. More tomorrow.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 20, 2018

So:

For 1) I think that we could set the DOM to v.dom instead.

I'm almost certain that 2) is a bug in its own right and that createFragment should actually return vnode.dom instead of fragment (and that we should only conditionally re-assign nextSibling in the case that result is not null).

@JacksonJN

This comment has been minimized.

Copy link

JacksonJN commented Apr 20, 2018

Thanks for the feedback.

Looks like changing createFragment to return vnode.dom causes some issues when creating a component with fragments.

I tried the changes and some of the tests fail, including this one:

o("can return fragments", function() {
var component = createComponent({
view: function() {
return [
{tag: "label"},
{tag: "input"},
]
}
})
render(root, [{tag: component}])
o(root.childNodes.length).equals(2)
o(root.childNodes[0].nodeName).equals("LABEL")
o(root.childNodes[1].nodeName).equals("INPUT")
})

The fragment children are rendering out of order. Code snippet of the test.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 20, 2018

I'm starting to get the hang of it...

There's a similar bug when using the map-based diff part of the loop. PR coming soon.

pygy added a commit to pygy/mithril.js that referenced this issue Apr 20, 2018

pygy added a commit to pygy/mithril.js that referenced this issue Apr 20, 2018

@pygy pygy referenced this issue Apr 20, 2018

Merged

Fix 2128 #2130

5 of 11 tasks complete
@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 20, 2018

Fix proposal in #2130

@JacksonJN

This comment has been minimized.

Copy link

JacksonJN commented Apr 20, 2018

Thanks @pygy! I tested the proposed fixes in my project and everything works so far. Previous errors are gone.

pygy added a commit to pygy/mithril.js that referenced this issue Apr 20, 2018

pygy added a commit to pygy/mithril.js that referenced this issue Apr 20, 2018

pygy added a commit to pygy/mithril.js that referenced this issue Apr 21, 2018

pygy added a commit to pygy/mithril.js that referenced this issue Apr 21, 2018

@pygy pygy closed this in #2130 Apr 23, 2018

pygy added a commit that referenced this issue Apr 23, 2018

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this issue Oct 12, 2018

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this issue Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment