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

hx-* attributes ignored when swapping in different type of element #124

Closed
timomihaljov-yle opened this issue Feb 26, 2025 · 9 comments · Fixed by #129
Closed

hx-* attributes ignored when swapping in different type of element #124

timomihaljov-yle opened this issue Feb 26, 2025 · 9 comments · Fixed by #129

Comments

@timomihaljov-yle
Copy link

When using hx-swap="morph" to swap in a different type of element (e.g. a <div> to replace a <form>), the hx-* attributes of the new element are ignored and the element becomes inert.

The following examples demonstrate the problem:

@botandrose
Copy link
Collaborator

Amazingly helpful reproduction, thank you! I'll take a look at this today.

@botandrose
Copy link
Collaborator

@MichaelWest22 Would you mind taking a look at this? The htmx->idiomorph extension isn't getting triggered the second time, but I don't yet know why. I tried digging into the htmx source to find the hx-get implementation, specifically how the event listeners are set up, but couldn't find it. Trying to dig through 5261 lines of code that I don't use or understand is a bit intimidating! I think you'll be able to figure this out much quicker than me, if you have the time.

@MichaelWest22
Copy link
Collaborator

MichaelWest22 commented Feb 27, 2025

Ok had a look at debugging this. Found the cause of the issue is the duct tape parent seems to be causing our issue here.

edit ignore this following bit I was totatally wrong:
The duck tape parent returns [newContent] when you ask for childNodes and the outerHTML routine uses this to find the on page content children nodes but this is actually the old source data from the morph and not the data that hit the DOM. So in the outerHTML morph default case it will now return the temporary source nodes and not the nodes that have been morphed on the DOM. HTMX extension setup uses the returned nodes and for each one it performs htmx process/init functions to enable any hx attributes on the newly added nodes and this is currently running on the orphaned temporary source nodes instead.

Actually I was totally wrong above and it is mostly fine as it is not newContent but old content in the duct tape parent. But the actual issue is that the duck tape parent is getting created at the start with the original oldNode as its only children and then the morph happens and often this node is now orphaned if it gets replaced with a new real node. the duck tape parent does not update to the real new children after the morph has happened as it is not a real parent node that updates. In the example provided the morph is changing a div node into a button node which can not be morphed in place as they do not softmatch so there is actually no morphing possible in this situation and it just adds the new button node and deletes the div. situations where an id'ed div moves down levels will also cause problems. in the end the node being returned to htmx is now the orphaned removed node and htmx never gets passed back the button so it does not call init routines on it for hx parameters

I think we could maybe remove the duck tape parent to resolve this.

Also we could streamline the morphOuterHTML solution like this:

  function morphOuterHTML(ctx, oldNode, newNode) {
    const oldParent = normalizeParent(oldNode);

    // store start and end points so we can find inbetween nodes to return later
    // as we can avoid returning before or after siblings
    const beforeStartPoint = oldNode.previousSibling;
    const endPoint = oldNode.nextSibling;

    morphChildren(
      ctx,
      oldParent,
      newNode,
      // these two optional params are the secret sauce
      oldNode, // start point for iteration
      endPoint, // end point for iteration
    );

    const nodes = []
    // return array from the first node added to before the last node 
    let cursor = beforeStartPoint?.nextSibling || oldParent.firstChild;
    while (cursor && cursor != endPoint) {
      nodes.push(cursor);
      cursor = cursor.nextSibling;
    }
    return nodes;
  }

But I found this does not solve the limitations of the duck tape as there is just no way I can find for it to store updated node children list after the morph. The only way I can think to keep the duck tape would be to get morphChildren to create a running array of all its processed nodes and return these out which can then be just returned back easily. But this means we need to store and calculate this children nodes array in all morphChildren nested calls which is a perf hit.

@MichaelWest22
Copy link
Collaborator

https://codepen.io/MichaelWest22/pen/LEYbdNz example code pen with duck tape removed seems to work fine using the code from this branch:
main...MichaelWest22:fix-outer-normalization

While I think this change would be fine and do everything we need it to it does allow the mutation of the source new content in some situations as seen in one of the tests that I had to update. I don't think this is a real world concern but it is possible to use cloning to solve this unlikely edge case if required. But this did need some rework of the id-set generation code as the with the optimization to not clone and reparent the new content if it is a single node with no siblings can cause another test to fail which I added to detect if new content's parent id has a node.
Here is the possible change we could make here:
main...MichaelWest22:fix-outer-normalization2

@MichaelWest22
Copy link
Collaborator

main...MichaelWest22:realParent another example of a possible way to fix it if we had to keep duck tape old parent.

@botandrose
Copy link
Collaborator

@MichaelWest22 Thank you very much for taking a look at this!

Honestly, I'd really, really love to remove that ugly duck-type-parent hack, if we can do it without reparenting (losing state, maybe loss of perf?) or cloning (probably loss of perf for larger DOM trees). Or if it turns out the perf hit is negligable.

Related to #103, I think we can explicitly commit to not supporting hidden state in newContent, so reparenting that should be okay as long as there are no perf implications. I really have no idea if reparenting connected DOM nodes to e.g. a document-fragment is expensive, TBH!

I think there are clearly still many unknowns regarding all of that above, so right now I'm favoring your last solution in the realParent branch. I'll pull it down and play with it a bit.

@botandrose
Copy link
Collaborator

botandrose commented Mar 3, 2025

@MichaelWest22 I just had a brainwave. I think we can resolve all these recent issues by changing how the duck-type-parentNode works. Rather than holding onto the node so that it can return in an array as its childNodes, it holds on to the next and previous siblings, and calculates childNodes in real-time when needed, using your code above in #124 (comment) . This way it essentially works as a "live slice," if that terminology makes sense.

That said, I still really really do want to remove the duck-type-parentNode altogether. But at least for the purposes of a v0.7.3, I think this should at least resolve the issues we've been seeing. I'll open a separate PR.

@botandrose
Copy link
Collaborator

@timomihaljov-yle Idiomorph v0.7.3 should fix this. Reopen if not!

@timomihaljov-yle
Copy link
Author

Works great! Thanks so much!

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.

3 participants