Move matching nodes, rather than deleting up to them

[botandrose]

Hello, thank you so much for Idiomorph! It's a key component in making it possible to make modern-feeling apps with just server-side rendering. What an amazing gift to the world! I hope you will consider merging this PR to improve it further.

Context

I'm currently using Turbo + Idiomorph to build a collaborative project management app in the style of Pivotal Tracker (RIP). A core interaction of this app is reordering items by dragging and dropping. This change gets sent over the wire to other active users, and Idiomorph updates their screen with the new order.

Issue

However, I submit that Idiomorph's core algorithm doesn't handle reordering operations very well at the moment. Imagine the following scenario:

1. Alice and Bob are collaborating on a project with five items: 1, 2, 3, 4, 5
2. Bob is currently in item 3, writing a new comment in a textarea[data-turbo-permanent] (akin to hx-preserve)
3. Alice drags item 5 to the top of the list, to mark it as top priority: 5, 1, 2, 3, 4
4. The new list HTML gets sent over the wire to Bob, and Idiomorph begins to morph the old list into the new
5. Starting with the top item, item 5, it finds a match at the bottom of the old HTML
6. It deletes items 1, 2, 3, and 4 so that item 5 is now in the correct position on top
7. It finds no matches for items 1, 2, 3, and 4, so it adds them from scratch, one by one
8. Bob's screen now looks like Alice's, except it completely blew away the contents of his comment text box, and now he has to start over. Bob is angry.

Proposed solution in this PR

1-5. Same as above
6. It moves item 5 to the current insertion point with Element.before.
7. Items 1, 2, 3, and 4 match perfectly so they are kept in the DOM as-is.
8. Bob's screen now looks like Alice's, and he continues writing his comment without interruption. Bob is wistfully thinking about what he ate for breakfast.

Personally, I can see no downside to this strategy, other than perhaps more time spent in findIdSetMatch since its no longer bailing early to preserve potential future matches. But I expect that would be more than made up for less time spent in unnecessary DOM removal and reinsertion operations. Of course, I haven't done the deep dive into this domain that you have, so I'm sure you have more much insight into this.

So what do you think? Any reason not to do this?

Demo

I've added a video demo in the first commit that demonstrates the issue, and then also demonstrates the fix after the second commit is applied.

Before

After

[botandrose]

A few more follow-up commits:
1 & 4. Running this branch through Turbo's full test suite exposed a few more subtle bugs in Idiomorph, now that we're morphing elements much more often.
2. Move the new before function call into morphOldNodeTo so that it gets called at the right time with regards to the lifecycle hooks.
3. Apply the same movement strategy to soft matches, as well.

Now that all the tests are passing in Idiomorph, Turbo, and my app, I'm going to run this branch in production for a bit to see if anything else shakes out.

[1cg]

Hi @botandrose I'm hesitant to accept this change because idiomorph here is trying not to disconnect nodes from the DOM in order to keep things like the video playing. I'm not sure why the flower video in the example above stopped the flower video from playing, it looks like it should have been preserved properly, as it is very similar to the core idiomorph demo.

Bob's screen now looks like Alice's, except it completely blew away the contents of his comment text box, and now he has to start over. Bob is angry.

Yes, but the best case scenario we have here is that bob's text input looses focus if we insert it into the new UI, moving inputs that have active editing going on will kill focus anyway as far as I understand. We could prioritize morphing around an actively edited element over all other possible morphs, but that's a different change than what I see.

I would suggest that you consider forking and renaming the algorithm to something else if you prefer this behavior instead, I would be absolutely fine with that and I'm happy to link to it from this repo. You may also be interested in the upcoming moveBefore() API that will address these issues:

https://htmx.org/examples/move-before/

I'm planning on integrating that algorithm into idiomorph once it is widely available, but that may be a few years.

Sorry if I am misunderstanding your change, it's always hard to coordinate technical information over github!

[botandrose]

Hi @1cg, thanks for taking the time to look at this and consider it. If this PR isn't something you want to merge into Idiomorph, of course that's your perogative, and I respect that! However, reading what you've written above, it does seem to me like there's room for more discussion here. For example:

I'm not sure why the flower video in the example above stopped the flower video from playing, it looks like it should have been preserved properly, as it is very similar to the core idiomorph demo.

This demo indeed demonstrates the crux of the issue, and if you're surprised by it, then I think its likely worth belaboring a bit more.

The bug from an htmx context

GitHub's contributor list tells me that you're the main force behind htmx (amazing work, btw), so let's ignore the specific changes in this PR for now, and zoom out to the context of that project, and have another look at this bug. I submit that hx-preserve only works in a narrow happy path. If the relationship between the old and new DOM doesn't conform to a specific pattern, then hx-preserve has no effect. This seems to me to be a serious bug! If we were to do nothing to address this, then the following list of caveats would need be added to the documentation for hx-preserve:

• If the preserved element is in any of the following contexts, hx-preserve will have no effect, the element will be deleted, and a new copy will be added to the DOM from scratch.
  1. If any element in the HTML that is lexically after the preserved element gets moved to a position lexically before the preserved element, while maintaining the same parent. I say lexically, because this could be a sibling element to the preserved element, or a completely unrelated element in a totally different section of the document.
  2. If the preserved element (or any of its ancestors) gets moved to a different DOM level. For example, moving a leaf on a tree structure from level 3 to level 2, or vice-versa.
  3. If the preserved element (or any of its ancestors) gets moved to a different container. For example, moving an item from a list of Todo items to a list of Done items.

Assuming for the moment that the above list is an accurate description of the status-quo, would you agree that these are bugs that should be fixed if a satisfactory solution can be found? If we can agree on that, I propose that I'll write tests to demonstrate the veracity of each scenario, and then we can move onto discussing potential strategies for fixing them, whether its the one I propose in this PR, or something else entirely (I have other ideas).

Misc responses

Yes, but the best case scenario we have here is that bob's text input looses focus [snip]

Turbo has solved this by saving and restoring the focus point around the morph operation. But of course this only works if Idiomorph doesn't blow away the element's state.

You may also be interested in the upcoming moveBefore() API [snip]

Yes, while going down this rabbit hole, I came across this proposal, and worried briefly that a solution to this bug was blocked on moveBefore landing in browsers. However, I believe that the demo in this PR demonstrates that it does not!

I'm not sure why the flower video in the example above stopped the flower video from playing [snip]

As an aside, the first commit of this PR only contains the demo pictured above. So if you're so inclined, it'd be easy to check it out and poke at it in the broken state that is status quo, and then check out the next commit to see the fixed state.

Thank you again for your time and consideration, and I look forward to your reply!

[1cg]

hi @botandrose I copied the example html file you have locally and tried it and it does the expected thing: one of the videos continues playing while the other one (which was moved before it) stops playing. That's expected because we have to pick one video element to disconnect from the DOM so the play state for the one we move will get nuked.

This is why the current idiomorph algorithm "gives up": disconnecting a node isn't significantly different than just removing it entirely and adding a new node wherever it ends up. We try to do a decent job of detecting, via ids, which nodes we should "give up" on.

(the eventual moveBefore() API standardization will change this)

Now, I feel like I am probably missing what you are saying and this conversation would be better had in a more interactive channel. Are you OK jumping on the htmx discord to discuss it at some point? I can't promise I'm going to release a new version soon, but I'm definitely willing to talk more in an environment a bit more amenable to active discussion than github issues.

url is https://htmx.org/discord and you can ping me @1cg in the htmx-dev channel