Why does #3814 fail on master?

When transitioning from the tree

<div>
  <Fragment>
    <span>0</span>
    <span>1</span>
  </Fragment>
  <input />
</div>

to the tree (note <span>1</span> was unmounted)

<div>
  <Fragment>
    <span>0</span>
  </Fragment>
  <input />
</div>

the master branch has a bug where it will re-append all the children after the Fragment. This re-appending of the <input> causes it to lose focus. The reason for this re-appending is that when exiting diffChildren for the Fragment, it's _nextDom pointer is set to <span>1</span>. Since <span>1</span> is unmounted it's parentNode is null. When diffing the input element, we hit the oldDom.parentNode !== parentDom condition in placeChild which re-appends the <input> tag and sets oldDom to null causing all siblings of <input> to re-append.

When diffing components/Fragments, the _nextDom pointer should point to where in the DOM tree the diff should continue. So when unmounting <span>1</span>, the Fragment's _nextDom should point to the <input> tag. The previous code in diffChildren removed in #3738 was intended to fix this by detecting when _nextDom pointed to a node that we were unmounting and reset it to it's sibling. However, that code (copied below) had a correctness bug prompting its removal (#3737). Let's look at this bug.

// Remove remaining oldChildren if there are any.
for (i = oldChildrenLength; i--; ) {
	if (oldChildren[i] != null) {
		if (
			typeof newParentVNode.type == 'function' &&
			oldChildren[i]._dom != null &&
			oldChildren[i]._dom == newParentVNode._nextDom
		) {
			// If the newParentVNode.__nextDom points to a dom node that is about to
			// be unmounted, then get the next sibling of that vnode and set
			// _nextDom to it
			newParentVNode._nextDom = getDomSibling(oldParentVNode, i + 1);
		}

		unmount(oldChildren[i], oldChildren[i]);
	}
}

What caused #3737?

Here is a simplified repro of the issue from #3737:

function App() {
	const [condition] = useState(true);
	return this.state.condition ? (
		// Initial render
		<>
			<div>1</div>
			<Fragment>
				<div>A</div>
				<div>B</div>
			</Fragment>
			<div>2</div>
		</>
	) : (
		// Second render: unmount <div>B and move Fragment up
		<>
			<Fragment>
				<div>A</div>
			</Fragment>
			<div>1</div>
			<div>2</div>
		</>
	);
}

The first render creates the DOM 1 A B 2 (each wrapped in divs) and when changing the condition state, it should rerender to produce A 1 2 but instead we get 1 A 2. Why?

When rerendering App we unmount <div>B</div>, which is a child of a Fragment. This unmounting triggers the call to getDomSibling in the code above. getDomSibling has a line of code in it that looks like vnode._parent._children.indexOf(vnode). This line of code doesn't work in this situation because when rerendering a component, we only make a shallow copy of the old VNode tree. So when a child of oldParentVNode (the App component in this case) tries to access it's parent through the _parent pointer (e.g. the Fragment parent of <div>A</div>), it's _parent pointer points to the new VNode tree which we are in progress of diffing and modifying. Ultimately, this tree mismatch (trying to access the old VNode tree but getting the in-progress new tree) causes us to get the wrong DOM pointer and leads to incorrect DOM ordering.

In summary, when unmounting <div>B</div>, we need getDomSibling to return <div>2</div> since that is the next DOM sibling in the old VNode tree. Instead, because our VNode pointers are mixed at this stage of diffing, getDomSibling doesn't work correctly and we get back the wrong DOM element.

Why didn't other tests catch this?

Other tests only do top-level render calls (e.g. render(<App condition={true} />) then render(<App condition={false} />)) which generate brand-new VNode trees with no shared pointers. They did not test renders originating from setState calls which go through a different code path and reuse VNode trees which share pointers across the old and new trees.

The fix

The initial fix for this is to replace getDomSibling with a call to dom.nextSibling to get the actual next DOM sibling. (I found a situation in which this doesn't work optimally. I'll open a separate PR for that.)

Final thoughts

One additional thought I have here is that walking through this has given me more confidence in our approach for v11. First, we do unmounts before insertions so we don't have to do this additional DOM pointer checking. Also, by diffing backwards, we ensure that our _next pointers are correct when we go to search what DOM element to insert an element before.

Fixes #3814, #3737