Skip to content
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

Preact adding elements out of order when replacing elements in DOM #3737

Closed
1 task done
jaredkrinke opened this issue Sep 19, 2022 · 1 comment · Fixed by #3738
Closed
1 task done

Preact adding elements out of order when replacing elements in DOM #3737

jaredkrinke opened this issue Sep 19, 2022 · 1 comment · Fixed by #3738

Comments

@jaredkrinke
Copy link

jaredkrinke commented Sep 19, 2022

  • Check if updating to the latest Preact version resolves the issue (No, it doesn't)

Describe the bug
Preact seems to be adding HTML elements out of order when updating the DOM, given the right set of before/after elements. In my case, I'm swapping between two pieces of content.

This happens on 10.11.0 and 10.5.8, but does not happen on 10.5.7--so it seems like a regression in 10.5.8.

Tested on 10.11.0 in MS Edge Version 105.0.1343.42 on Windows (both 64-bit)

To Reproduce

Start with the sandbox:
https://codesandbox.io/s/preact-x-preact-cli-3-starter-vj285y2rn3

IMPORTANT: Upgrade to latest preact version! (Note: the dropdown is very finicky...)

Paste in this code:

import { Component, Fragment, render } from "preact";

class App extends Component {
  constructor(props) {
      super(props);
      this.state = { first: true };
  }

  render() {
      if (this.state.first) {
          return <Fragment>
              {<><p>1. Original item first paragraph</p></>}
              <p>2. Original item second paragraph</p>
              <button onClick={() => this.setState({ first: false })}>Click me</button>
          </Fragment>;
      } else {
          return <Fragment>
              <p>1. Second item first paragraph</p>
              {<Fragment>
                  <p>2. Second item second paragraph</p>
                  <div></div>
              </Fragment>}
          <button onClick={() => this.setState({ first: true })}>Click me</button>
          </Fragment>;
      }
  }
}

// Codesandbox workaround.
render(<App />, window.root);

Steps to reproduce the behavior:

  1. Observe first set of elements in expected order
  2. Click the button
  3. Observe the second set of elements in expected order
  4. Click the button

Expected behavior
The first set of elements should be shown in the same order as in the first step.

Actual behavior
The first set of elements is shown in the wrong order!

@jaredkrinke
Copy link
Author

For the record: in my case (which is fairly convoluted, but from which the repro above was derived), I was able to avoid this issue by adding a key attribute to distinguish different pieces of content that I swap between in my container.

andrewiggins added a commit that referenced this issue Feb 2, 2023
…#3875)

## Why does #3814 fail on master?

When transitioning from the tree

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

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

```jsx
<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.

```js
// 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:

```jsx
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant