Should there be an opt-out for declarative shadow roots having `clonable=true`?

[mfreed7]

What is the issue with the HTML Standard?

The spec for declarative shadow DOM says:

• Attach a shadow root with declarative shadow host element, declarative shadow mode, true, declarative shadow delegates focus, and "named".

The third true argument there corresponds to the clonable flag, meaning all declarative shadow roots are clonable. It seems like there should be a way to opt out of that behavior somehow, e.g.

<template shadowrootmode=open notclonable>

or something like that. Otherwise, every declarative shadow root becomes clonable. There is some concern from Chromium that shipping the general clonable behavior might be a web-incompatible change, for sites that use SSR/DSD and assume those shadow roots aren't cloned. @justinfagnani raised this concern for Lit users, for example.

@annevk @emilio

[justinfagnani]

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.

[emilio]

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

[mfreed7]

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 https://github.com/whatwg/dom/pull/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>

[annevk]

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

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

[mfreed7]

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.

[annevk]

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.

[mfreed7]

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?

[emilio]

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

[mfreed7]

Alright, I wrote that up in https://github.com/whatwg/html/pull/10117. It'll have a merge conflict with https://github.com/whatwg/html/pull/10069 once that lands, but I can fix it up after that. The DOM PR (https://github.com/whatwg/dom/pull/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.

[justinfagnani]

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?

[mfreed7]

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.

[emilio]

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.

[mfreed7]

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?

[annevk]

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.

[mfreed7]

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 https://github.com/whatwg/dom/issues/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?

[annevk]

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 https://github.com/whatwg/dom/issues/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.

[emilio]

@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...

[mfreed7]

@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.

[annevk]

@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.

[emilio]

@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).

[annevk]

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.

[mfreed7]

So the open questions are:

1. Should some ShadowRoot attributes be mutable? Which ones?
2. Should attachShadow() throw on some/all mismatches in attributes? If some, which ones?

For (1), the current (shipped and spec'd) behavior is that no parameters are mutable on ShadowRoot. It feels "ok" to kick this can down the road a bit, since it'll be web compatible to make them mutable later.

For (2), the current (shipped and spec'd) behavior for attachShadow() is never to throw on a mismatch. That means it'll get increasingly hard to make a change to that over time, since it might become web incompatible to start throwing exceptions. The most likely (and dangerous/concerning) mismatch is on the mode, I would think. As a compromise position, perhaps we just make attachShadow() throw if the mode mismatches? And then leave other attributes alone, even if they mismatch. I think that relieves compat pressure for the other attributes, and we can potentially discuss and make changes later.

WDYT?

[annevk]

That's acceptable to me, but I continue to have a slight preference for ignoring the argument altogether when there's an existing shadow host. @emilio?

[emilio]

I have the opposite (but also not super-strong) preference. mode at least seems worth raising an error for.

[mfreed7]

Perhaps only throwing on mode mismatch is a good compromise, then? It doesn't throw on all mismatches, but it does throw on one bad one.

Let me know if that proposal is acceptable, and I'll update the spec PRs accordingly.

[annevk]

Let's do that. I will attempt to get all the things we agreed on merged next week.

[mfreed7]

Let's do that. I will attempt to get all the things we agreed on merged next week.

Great! Thanks. I just updated https://github.com/whatwg/dom/pull/1246 accordingly. Let me know if I missed something.