How should `clonable` work in `cloneNode(false)`?

[mfreed7]

This issue was raised as part of the Chromium review of the implementation of clonable. The question is what should happen here:

const shadow = host.attachShadow({mode: 'open', clonable: true});
shadow.innerHTML = '<div><span>what about me?</span></div>`;
const clone = host.cloneNode(false);

What should happen to the shadow root? Since it is clonable, I'd expect clone to have a shadow root, with parameters that match shadow. But should the content of that shadow root be deep-cloned from host? Or should clone.shadowRoot be empty? Both of those feel slightly odd. I think I slightly prefer deep-cloning the shadow content, but I'm not sure.

@dbaron @annevk @saschanaz

[saschanaz]

cc @avandolder

[saschanaz]

We should probably follow the expectation when cloning UA-provided elements with shadow roots, which would be the option 1; deep cloning the shadow root. (If I'm not misunderstanding how they work, cc @emilio)

[emilio]

They do not get cloned in Gecko (except for printing as a special case). A new shadow root is attached when the clone gets inserted into the document.

[annevk]

Cloning the shadow tree completely makes sense to me. It does make me wonder if we at some point need to provide more cloning options.

cc @rniwa

[mfreed7]

Sounds like 3 votes (including myself) for deep-cloning the shadow tree, and one for not cloning it. @emilio would you be ok changing to a deep clone behavior?

[emilio]

Yeah, I don't mind either way, I'm just saying that that's not the built-in shadow tree behavior at least in Gecko.

Might be worth checking with @justinfagnani about the ergonomics of this for authors too.

E.g., code that does:

connectedCallback() {
  this.attachShadow({ ... });
}

Or so will throw after cloning if we clone the shadow tree. Maybe fine? You kinda need to deal with that for moves inside the document.

So over-all I think given ^ I'm fine with cloning the shadow tree. But hopefully I'm not missing something.

[saschanaz]

Yeah, I don't mind either way, I'm just saying that that's not the built-in shadow tree behavior at least in Gecko.

Does anyone know what Blink and WebKit do?

[mfreed7]

Does anyone know what Blink and WebKit do?

Blink does not clone the shadow root at all for cloneNode(false), at least currently.

Or so will throw after cloning if we clone the shadow tree. Maybe fine? You kinda need to deal with that for moves inside the document.

This is a good point. I'm really starting to think general clonable shadow roots in the main page might cause more problems than they solve. At the very least we need https://github.com/whatwg/html/issues/10107 resolved.

But it sounds like we have rough consensus that modulo the other clonable issues, cloneNode(false) should deep-clone the shadow root. That's nice because that's actually already what the spec says:

7.3 If the clone children flag is set, then for each child child of node’s shadow root, in tree order: append the result of cloning child with document and the clone children flag set, to copy’s shadow root.

[justinfagnani]

Yeah, I don't mind either way, I'm just saying that that's not the built-in shadow tree behavior at least in Gecko.

Might be worth checking with @justinfagnani about the ergonomics of this for authors too.

E.g., code that does:

connectedCallback() {
  this.attachShadow({ ... });
}

Or so will throw after cloning if we clone the shadow tree. Maybe fine? You kinda need to deal with that for moves inside the document.

Yes, you would never write code like that because moves are very common. You either call attachShadow() in the constructor or gate it and call it on the first connection only.

So over-all I think given ^ I'm fine with cloning the shadow tree. But hopefully I'm not missing something.

I actually don't see how cloning the shadow tree is that useful at all in the general case. It's so common that the shadow root depends on non-clonable state, that the only components this would work for would have to be completely static.

[annevk]

I guess the main issue here is that we need better test coverage for this scenario?

[annevk]

It seems this was tested, but also required a change to the specification and that was not proposed. I created #1272 for that.