Fix issue with outerHTML morphing an IDed node that gets moved

[botandrose]

Summary

While fixing another bug, I stumbled upon a corner case issue. Consider the following situation:

1. Root node is IDed
2. Root node gets moved
3. outerHTML morph style
4. Root node is part of the document and gets a fake duck-typed parent node because of 3

When all of the above are true, a type error is raised: TypeError: Cannot read properties of null (reading 'id') at removeElementFromAncestorsIdMaps

Example

<hr id="a"> => <div><hr id="a"></div>
The first commit has this in a failing test.

WIP Analysis

1. The error raises because moveBeforeById fails to consider the case where the ctx.target is the IDed node to move.
2. Fixing that issue (second commit here) results in an incorrect morph, due to the hacky nature of the duck-typed parent node.
3. ???
4. Profit!

[botandrose]

Hmm, looks like this isn't actually as bad as it appears. The ctx.target getting moved in a morph is a bit weird, but I think the resultant HTML actually is correct after the second commit. It was just the way the test helpers were reusing the nodes made it seem otherwise.

@MichaelWest22 would you mind applying your keen perspective to this issue? My last commit marks three tests with it.only. I think the first one is a correct, good test that actually demonstrates the failure. While the last two are actually red herrings. I think at this point the only issue to solve is a wrong return value. What do you think? Does that sound right to you?

[MichaelWest22]

We found it was possible to remove the duck tape solution earlier which I think would likely resolve this issue properly. But the reversion of the duck tape solution i proposed had a drawback that by default it would allow the mutation of the source nodes which was always the way idiomorph 0.4.0 and older worked anyway. But in 0.7.0+ with the duct tape solution it no longer mutates the source nodes as it normally did before. I found a solution to this by always cloning the source in this single node case but this has a small perf loss we would need to spend more time testing. I think the mutation of the source node in situations like this is fine as no one should be using idiomorph like this in real world use case and anyone using it this way in old deployments would have had issues anyway before.

I also think that the test fidelity tests are not designed well to handle the way idiomorph can work to retain things. the fidelity test as it is written right now does not handle the tests you have written and to fix this it would have to insert the initial as a child in the dom and then compare the finial child contents with the expected value and not the initial value that can be moved by moveBeforeId stuff. The current testing just creates an orphaned node and then mutates it to the destination content and this will just never match this test for these edge cases. You can probably just move these tests to use a different manual testing strategy instead of fidelity.js. maybe ops.js but not sure.

[MichaelWest22]

Retested your extra tests in my test branch where i removed duck tape solution and it seems to me the duck tape is not the real issue here. just a simple missed case with the query selector so I think your fix is fine. And those last two tests are just invalid so i would ignore them and do it more the way you have in your first .only test.

[botandrose]

@MichaelWest22 Thank you for your keen perspective, as always! I agree with your assessment, and I'll clean this up accordingly.

[botandrose]

@MichaelWest22 Okay, cleaned up and green, so merging. The return values are still off, but that should be fixed by #129