Should there be an opt-out for declarative shadow roots having `clonable=true`? about html HOT 27 OPEN

I think it would be better if:

1. By default they do not clone, if that is indeed the desired behavior.
2. Adding a shadowrootclonable attribute makes them clonable.

So if you use them inside template you'd have to add that attribute.

from html.

I think it would be better if:

1. By default they do not clone, if that is indeed the desired behavior.
2. Adding a shadowrootclonable attribute makes them clonable.

So if you use them inside template you'd have to add that attribute.

I think I like this proposal. It's a bit of a change from the existing shipped behavior, so there might be compat issues. But it feels like there's a very straightforward migration path for that breakage, to simply add shadowrootclonable. And it feels a lot less risky than the currently-spec'd behavior.

I'm willing to give this a try to see what the compat implications are, if there's consensus enough to get that change landed in the spec first?

from html.

That sounds reasonable to me, and makes clonable consistent with all other attributes, and with itself in the scripting and non-scripting case.

from html.

Sorry, I somehow missed the clonable aspect of DSD entirely.

I think it's very much not true that just because a shadow root was created via DSD that it would be valid to clone it later.

For Lit, there are at least two major problems that make cloning problematic:

1. For client side rendering we don't emit the necessary extra DOM structure in order to rehydrate. We don't emit template IDs to be able to re-associate DOM to the template that rendered it, and we don't emit marker comments for attribute bindings. The code to do this isn't even part of the client-side packages.
2. A shadow root could easily go through state-driven changes before being cloned where that state is not available to the clone. Without something like cloneCallback() we don't even have a spot to clone that state. This will make the cloned shadow root useless, as we'd have to clear it out and re-render to be in sync with the clone's actual state.

So I'm pretty confident that right now many, if not most, cloned SSR'ed Lit components would fail, and our only fix would be to detect this case (we would need to differentiate from an actual DSD somehow) and clear the shadow root, completely defeating the purpose of cloning in the first place.

I would rather see cloneable be an opt-in.

from html.

cloneable being opt in makes more sense for whatwg/dom#1246 and co, actually. Otherwise any new non-declarative shadow DOM that you attach is likely to throw...

from html.

cloneable being opt in makes more sense for whatwg/dom#1246 and co, actually. Otherwise any new non-declarative shadow DOM that you attach is likely to throw...

Gah, that's a very good point. That would completely break the use case for attachShadow() emptying and returning the declarative shadow root - to avoid breaking old components that don't know about SSR.

I'm starting to wonder about the rest of the behavior in whatwg/dom#1246 actually. I wonder if the behavior should be that it throws if the call to attachShadow() explicitly passes an argument that disagrees with the existing shadow root, but doesn't throw if that parameter is not passed. E.g.

<div><template shadowrootmode=open></template></div>
<script>
  div.attachShadow({mode: open, clonable: false}); // Throws - clonable mismatch
  div.attachShadow({mode: open}); // Don't throw because `clonable` not passed explicitly
</script>

from html.

Why did we make declarative shadow roots clonable by default? whatwg/dom#831 and whatwg/dom#1137 are not very clear on this.

Also, hasn't this already shipped as part of declarative shadow trees?

from html.

Why did we make declarative shadow roots clonable by default? whatwg/dom#831 and whatwg/dom#1137 are not very clear on this.

Chrome's originally shipped behavior (and initial spec PR) only cloned declarative shadow roots inside template elements. It modified only the template cloning steps. As part of the review, that was changed to the currently spec'd behavior with the clonable flag. At the time that change was made, I didn't realize the implications of that for DSD roots outside templates. (I should have.)

Having said all of that, the use case was, and still is, to allow declarative shadow DOM to be used in a <template> that gets stamped out and still contains the shadow roots. Cloning general shadow roots is perhaps not a real world use case, I'm not sure. I'd be happy if we went back to the original proposal of just cloning shadow roots within templates, but I'm open to suggestions. Perhaps clonable can be made to be true by default only for DSD within a <template>? That would at least be web compatible for sure.

Also, hasn't this already shipped as part of declarative shadow trees?

The behavior that already shipped (in 2020 originally, and no change was made to this part in the re-shipment with shadowrootmode in 2023) is to clone shadow roots found in <template>s.

from html.

Alright, I wrote that up in #10117. It'll have a merge conflict with #10069 once that lands, but I can fix it up after that. The DOM PR (whatwg/dom#1246) doesn't need changes to accommodate this, since it already checks clonable and that now seems ok since clonable is opt-in everywhere.

If we're good with this, let's land the spec changes, and I'll try shipping this new behavior in Chrome to make sure it's web compatible.

from html.

We chatted about this on the Lit team yesterday, and I think we'd like a bit more discussion on the intention and use cases for this feature if possible.

I'm somewhat sympathetic to the idea that trees with declarative shadow roots should be clonable with the idea that one would expect a deep clone to produce the same tree and that it functions the same. Was that the intention behind this feature?

The problems arise after the yet-to-be-cloned shadow root is modified. One way around those problems would be making the shadow root non-clonable during hydration, so it could still be cloned when we knew it had all the correct structure to be hydrated but not clone when it may be out-of-sync.

So, could clonable be mutable?

from html.

So, could clonable be mutable?

I was previously reluctant to do this, but now, having implemented the rest of this feature, I think that'd be trivial. The clonable flag is just checked at clone time, so there's no harm (and nothing to be done) when flipping it between false/true.

@annevk @emilio any objections to that small change? If not I'll incorporate it.

from html.

Any objections to that small change? If not I'll incorporate it.

I think that same simplicity argument could apply as well to other members (delegatesFocus at least).

I'm not sure I object but then throwing for those in attachShadow seems silly (could be better to "override only if present" or so). So maybe worth doing / discussing in a separate issue.

from html.

I think that same simplicity argument could apply as well to other members (delegatesFocus at least).

Probably a good point, and I think I agree. But can we tackle non-clonable things as a follow up? There are already a lot of moving parts.

I'm not sure I object but then throwing for those in attachShadow seems silly (could be better to "override only if present" or so). So maybe worth doing / discussing in a separate issue.

I agree with that point, but then I wasn't a fan of throwing in the first place. I assume you're saying that for things that are read/write about the shadow root, make attachShadow() just mutate those bits when it "re-purposes" an existing declarative shadow, and don't throw? If so, @annevk are you ok with that change?

from html.

Thoughts:

1. Making shadow root members mutable is an interesting proposition that needs wider consideration. I'd rather not couple it with the changes we're making now, but make it a proposal on its own that people can reflect on for a while.
2. I think it would be reasonable that if we had mutable members, their setter would set declarative to false. As you're outside the declarative realm at that point. And that would neatly resolve the attachShadow() issue.

from html.

Thoughts:

1. Making shadow root members mutable is an interesting proposition that needs wider consideration. I'd rather not couple it with the changes we're making now, but make it a proposal on its own that people can reflect on for a while.

Makes sense. I opened whatwg/dom#1252 for this.

2. I think it would be reasonable that if we had mutable members, their setter would set declarative to false. As you're outside the declarative realm at that point.

Yeah, that makes some sense to me also. Once you start touching it with JS, it feels like it's no longer purely declarative.

And that would neatly resolve the attachShadow() issue.

I'm not sure - I think there's maybe still an issue with clonable? Since we're proposing a change to the shipped behavior, specifically that declarative shadow roots aren't clonable by default, it might be the case that some SSR needs to change to add shadowrootclonable. Which would then break legacy code that just calls attachShadow() without passing the clonable flag.

Would it be acceptable to not throw for mismatches in clonable, serializable, and maybe delegatesFocus? Given that once those are mutable, attachShadow() could just mutate them?

from html.

Hmm, I think we don't want attachShadow() to mutate them though.

If the case you mention is important I think the status quo design for attachShadow() is the most reasonable. And we should just not fix that part of whatwg/dom#1235 (the other parts identified there still seem good to fix to be clear). I.e., calling attachShadow() on a shadow host that has an existing declarative shadow root will empty that shadow root and return it, and any passed argument will be ignored.

from html.

@annevk I'm confused, so you don't want attachShadow() with a mismatched mode to report any error, instead eating and potentially returning a completely different ShadowRoot than what attachShadow requested? That seems pretty terrible to me...

from html.

@annevk I'm confused, so you don't want attachShadow() with a mismatched mode to report any error, instead eating and potentially returning a completely different ShadowRoot than what attachShadow requested? That seems pretty terrible to me...

This was why I suggested throwing for the key mismatch cases, like mode and slotAssignment. I'm happy throwing for those, I'm just concerned that we're rushing into throwing for the new (maybe) mutable attributes, and it seems like we might not want to throw for those just yet, since maybe we (maybe) want to mutate them instead.

from html.

@emilio yes, I rather have it consistently ignore arguments for the existing shadow host path than only sometimes. That seems a lot easier to explain and maintain.

from html.

@annevk My understanding is that @mfreed7's proposal wouldn't ignore arguments (not even just sometimes), and would either throw (for immutable arguments like mode) or override (for mutable arguments like clonable).

from html.

I think the problem is that we don't know which ones end up being mutable. It also seems weird to me for attachShadow() to do any kind of mutating.

from html.