fix: ensure correct each block element is moved during reconcilation

[trueadm]

Fixes #12177 and fixes #12198

[changeset-bot]

🦋 Changeset detected

Latest commit: 1488aea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Rich-Harris]

This definitely doesn't look right to me. The parent/child relationships aren't relevant here — in the test, if the {#if ...} block is outside the <li>, things remain fucked up.

Ultimately I think what we need here is some version of #11690. Here's an idea that I haven't had the opportunity to test yet: each effect's dom property (or d or whatever) represents the beginning of its DOM range and is either a text/element node, or null if that doesn't exist (because the first child is an {#if ...} or whatever). So get_first_node would look something like this:

function get_first_node(effect) {
  return effect.d ?? effect.first && get_first_node(effect.first);
}

There's no property for the end of its DOM range because that's just get_first_node(effect.next).

If an effect has a template like this...

var root = $.template(`<span></span> <!>`);

...then we call $.append with a flag that causes the current effect's d property to be set to the <span> if it hasn't been set already...

-$.append($$anchor, fragment);
+$.append($$anchor, fragment, true);

...but if it's like this...

var root = $.template(`<!> <span></span>`);

...then we do the opposite:

$.append($$anchor, fragment, false);

I'm sure I'm missing something here but it feels like this should work.

[trueadm]

This definitely doesn't look right to me. The parent/child relationships aren't relevant here — in the test, if the {#if ...} block is outside the <li>, things remain fucked up.

Ultimately I think what we need here is some version of #11690. Here's an idea that I haven't had the opportunity to test yet: each effect's dom property (or d or whatever) represents the beginning of its DOM range and is either a text/element node, or null if that doesn't exist (because the first child is an {#if ...} or whatever). So get_first_node would look something like this:

function get_first_node(effect) {
  return effect.d ?? effect.first && get_first_node(effect.first);
}

There's no property for the end of its DOM range because that's just get_first_node(effect.next).

If an effect has a template like this...

var root = $.template(`<span></span> <!>`);

...then we call $.append with a flag that causes the current effect's d property to be set to the <span> if it hasn't been set already...

-$.append($$anchor, fragment);
+$.append($$anchor, fragment, true);

...but if it's like this...

var root = $.template(`<!> <span></span>`);

...then we do the opposite:

$.append($$anchor, fragment, false);

I'm sure I'm missing something here but it feels like this should work.

I was playing around with this idea. However, by not putting in the anchor nodes into the effect.dom, it means that when we unmount the effects, we are leaving lots of dangling empty text nodes in the document. So I think we still need to include them still, just maybe we can be smarter.

I really wish we could reliably use a element container here instead with display contents.

Update: I found the culprit here, and it seems we can apply a much simpler fix. The root cause was that when we put things in the effect.dom we should be inserting it before the anchor – as that's how we append it into the document. If we make a change to ensure effect.dom reflects this behaviour too then everything should just work.

I did notice that, even before this PR, that we insert hydrated nodes in effect.dom multiple times. It shouldn't cause any runtime errors, but it does seem wasteful. I'm hopeful that your other PR that refactors all this will also solve that and simplify a lot of this logic. However, in the meantime this seems like an appropriate bug fix.

[Rich-Harris]

Merged #12215 so will close this