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

custom attributes don't work in shadow roots (MO subtree observation doesn't move into shadow roots) #23

Open
trusktr opened this issue Apr 22, 2019 · 7 comments

Comments

@trusktr
Copy link

trusktr commented Apr 22, 2019

(Continuing from #3 (comment))

When we register a custom attribute, it only works in the root light tree, and the functionality doesn't carry over across ShadowDOM boundaries into shadow roots.

After the small change from #3, a user could create new registries for every shadow root in the app (perhaps by monkey patching attachShadow). But then would require forwarding registrations from the document registry to all shadow root registries whenever a custom attribute is defined.

It'd be sweet if Custom Attributes worked like Custom Elements in this regard, where a defined custom element works anywhere in the app, regardless of ShadowDOM boundaries.

@matthewp
Copy link
Owner

That indeed would be sweet but I don't know how to implement it in a non-scary sort of way. The only thing I can think is to override HTMLElement.prototype.attachShadow and force a registry onto each shadow that, like you said, inherits from all parent shadows.

@trusktr
Copy link
Author

trusktr commented Apr 22, 2019

As user of the lib, I can achieve what I want by writing

customAttributes.define( 'has', HasAttribute )

if (Element.prototype.attachShadow) {
    const _attachShadow = Element.prototype.attachShadow
    
    Element.prototype.attachShadow = function(...args) {
        const root = _attachShadow.call(this, ...args)
        const attributes = new CustomAttributeRegistry(root)
        
        attributes.define( 'has', HasAttribute )
        
        return root
    }
}

instead of just

customAttributes.define( 'has', HasAttribute )

and then it works in all shadow roots.

@trusktr
Copy link
Author

trusktr commented Apr 22, 2019

In my above example, it assumes that custom attributes are all defined before any shadow roots are created. That's the simple case which the above technique handles just fine, but the real problem is when/if someone defines custom attributes after having created shadow roots. This would involve needing to keep references to the registries that we've associated with shadow roots (and thus keeping references to shadow roots).

On the library side, we could make and track a new registry on each root (in the monkey patch), keep registries in an Array or Set, and any time a new attribute is registered on the document registry we'd loop through all the shadow root registries and register them there too.

The problem is, if we're using Array or Set, then there will be a leak when the roots aren't used any more because they are still contained in our Array or Set, and we have no idea when they are no longer used. We also can't iterate on a WeakSet.

We'd need a leak-free way to traverse all root, if we want to do that, otherwise we'd require all shadow-containing components to call a cleanup method, which wouldn't be ideal.

One thing we could do is force all roots to be mode: "open", which makes them easy to traverse, but then we are breaking the expectations that users of attachShadow have. That's the only way I can think of so far to be able to iterate on shadow roots without leaking.

Is there some way?

@Elvynia
Copy link

Elvynia commented Aug 11, 2019

@trusktr Thank you ! I've built a temporary solution upon yours using LitElement. Instead of changing the prototype method, I override the createRenderRoot() with the following :

createRenderRoot() {
    let root = super.createRenderRoot();
    const attributes = new CustomAttributeRegistry(root)
    attributes.define('test-select', TestSelectAttribute);
    return root;
}

This allows choosing for each custom element whether it'll register custom attributes or not (maybe even which ones).

@trusktr
Copy link
Author

trusktr commented Dec 28, 2019

@Elvynia Interesting approach.

On the other hand, imagine a custom attribute that is designed to work on any element, anywhere in the DOM, without having any idea which shadow roots an end user is going to create. In this case, attachShadow is the only way to make the custom attribute work everywhere without the user having to make every custom element responsible for defining the custom attribute definition.

@wessberg
Copy link

Hey there,

I realize this is an old issue, but I agree with @trusktr that ideally, Custom Attributes should work on any element, anywhere in the DOM, including in subtrees not necessarily controlled/owned by the end-user. Additionally, as someone who works a lot with Shadow DOM, this is a functional requirement for me. And then ultimately, if something like Custom Attributes is to be standardized at some point (I'm hoping we can work in that direction somehow), it will be something that needs to be addressed at some point.

And as for your, @trusktr, example here, and as you mention yourself in a follow-up comment, it implies that the Custom Attributes are declared prior to the creation of any shadow roots. I think this needs to align with Custom Elements where an element can be upgraded lazily, after DOM insertion, if an element is declared later. A workaround could be to perform a recursive descent from the document root and upgrade all roots immediately after patching Element.prototype.attachShadow.

As for how to proceed, there are at least two things we'll need to consider:

  1. Observing attribute changes on subtrees from any root in the tree
  2. Observing connectedness changes on decorated nodes across any root in the tree. In that regard, I'd like to point you to this issue. It could really use some fresh eyes.

I understand why @matthewp may have reservations with patching Element.prototype.attachShadow, but in considering the alternatives I only see one.

If you're up for going down the standardization path, Custom Attributes could be treated as essentially a polyfill, a proof of concept of such Web API, in which case I find it reasonable that Element.prototype.attachShadow is in fact patched, just like globalThis would be patched with a CustomAttributeRegistry.

@matthewp
Copy link
Owner

@wessberg are you interested in a PR? Ideally it would be nice to experimental with this idea in a way that doesn't force everyone to use it. Perhaps we could have a different entry point for this idea? I'm also a little concerned that this might increase the size quite a bit, but will have to see.

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

No branches or pull requests

4 participants