htmx make bodyContains() handle possibly nested shadow root's

Description

make bodyContains() handle the case where elt might be inside nested shadow dom's

Testing

Tested manually with and without nested shadow dom's

Checklist

[x] I have read the contribution guidelines
[x] I have targeted this PR against the correct branch (master for website changes, dev for source changes)
[x] This is either a bugfix, a documentation update, or a new feature that has been explicitly approved via an issue
[x] I ran the test suite locally (npm run test) and verified that it succeeded

May 05 '24 20:05 spez

no opinion on this @kgscialdone do you have one?

Definitely needs tests if it is to be accepted though.

May 15 '24 18:05 1cg

This is... acceptable. Recursive checks like this are gross but there isn't really a better way.

May 15 '24 18:05 kgscialdone

@kgscialdone , @1cg - I propose to solve this via htmx extension. That way its up to the user to iterate and fetch the host node.

function getRootNode(elt) {
    // custom logic to iterate nodes and return host
    const node = /* ... */
    return node;
}

htmx.defineExtension("body-contains-elem", {
    bodyContains: function (elt) {
       const node = getRootNode(elt);
       return document.body.contains(node);
    }
})

The revised bodyContains would be:

function bodyContains(elt) {
    let exists = undefined;
    
    // process extension
    withExtensions(asElement(elt), function (extension) {
        exists = extension.bodyContains(elt);
    })

    if (exists === true || exists === false) {
        return exists;
    }

    // original implementation
    const rootNode = elt.getRootNode && elt.getRootNode();
    if (rootNode && rootNode instanceof ShadowRoot) {
        return getDocument().body.contains(rootNode.host);
    } else {
        return getDocument().body.contains(elt);
    }  
}

Let me know if this solution is acceptable, I can raise PR for it.

Jun 04 '24 19:06 innovation-stack

@innovation-stack I'm unclear why solving this with an extension would be preferable to simply solving it, even if the solution is a little gross. Could you elaborate on why?

Jun 05 '24 00:06 kgscialdone

@kgscialdone - For performance reasons. In my case, I am building a query-builder web component, which can be contain deeply nested components. Here's an illustration:

deeply-nested-wcs

Few notes:

Card component can be deeply nested, upto any level of hierarchy
In my query-builder, any deeply nested child component can refer its root via builder property. I want to leverage this fact as part of bodyContains implementation.
As per solution proposed in PR, calling bodyContains on any deeply nested child web component have to iterate over all parent web components, which will incur performance. bodyContains is being called multiple times during render and API calls.

Proposed solution:

Using body-contains-elem extension, I can completely bypass all parent node traversals and simply use the builder property to refer to root component.

Let me know your thoughts on this. Thank you.

Jun 05 '24 06:06 innovation-stack

@innovation-stack I see where you're coming from, thank you for the explanation. However, I don't think that relegating a fix for this to extensions is the right approach from a logistical point of view, as it puts the impetus on developers using HTMX to handle the problem rather than on HTMX itself.

That said, on further inspection of the use cases of bodyContains, it may be worth looking into whether it actually needs to be "does the <body> contain this element", or if all use cases would be fine also including elements in the <head>. I can't determine that offhand at the moment, so will need to inquire of @1cg for more information, but if it's the case that it doesn't matter whether it's in the <body> or the <head> in all use cases, the whole function could be replaced with the following and avoid the whole issue:

function documentContains(elt) {
  return elt.getRootNode({ composed: true }) === getDocument()
}

Jun 05 '24 06:06 kgscialdone

@kgscialdone : Thank you for the explanation. I agree with your solution. Its elegant. Let me know what's the final verdict on the approach.

Jun 05 '24 06:06 innovation-stack

Got Carson's approval and closing this in favor of #3034. Thanks for diving on this everyone.

Nov 24 '24 17:11 alexpetros