Should attachShadow throw or override when an existing declarative shadow root doesn't match?

[emilio]

Should attachShadow throw or override when an existing declarative shadow root doesn't match?

[emilio]

(Forgot the ccs, hah)

cc @mfreed7 @rniwa @annevk @avandolder

[mfreed7]

I did think about this one. The important detail (to me at least) is that attachShadow in these cases will clear out the shadow root before returning it. So it's not like this is an end-run around revealing closed shadow root content or anything. It's just a weird way to create a closed shadow root via a call to attachShadow({mode:'open'}).

I do agree that it's perhaps a bit odd that the options provided to attachShadow don't get respected in this case, but the alternative seemed worse to me: changing the flags on an already-existing shadow root as a result of this call. I'm open to suggestions. (Keeping in mind that several implementations have shipped.)

[emilio]

I don't know, that makes code like this potentially fail in a very action-at-a-distance way, and with no pointer to the root cause of the issue (a shadowrootmode=closed template):

host.attachShadow({ mode: "open" });
host.shadowRoot.appendChild(...);

I guess we could add a console warning or so... Maybe it's worth a note in the spec?

[annevk]

Why did we not throw again?

I'm also not sure I understand

This means that if multiple declarative shadow roots are contained within a single shadow host, only the last one will remain.

upon rereading. How could a host ever have multiple shadow roots?

[emilio]

You'd have multiple <template> elements as children.

[annevk]

Okay, but those are not necessarily declarative shadow roots. Only one of them can be. Might just be a matter of using more precise wording then.

[mfreed7]

I don't know, that makes code like this potentially fail in a very action-at-a-distance way, and with no pointer to the root cause of the issue (a shadowrootmode=closed template):

That's true. But typically this (DSD) is already "action at a distance" by definition, since it's rendered on the server.

I guess we could add a console warning or so... Maybe it's worth a note in the spec?

This makes a lot of sense - the console warning. I think I'll add that to Chromium. Do we add notes about console warnings to the spec?

Why did we not throw again?

Not throwing is an important backwards-compatibility behavior, which is the entire point for this whole "empty and return the root" behavior. If a component was built without SSR/DSD knowledge, and then some server code decides to SSR that component, we didn't want to break the component. The only real way to do that is to keep attachShadow() working roughly as intended.

Importantly, if the server is taking a component that calls attachShadow({mode:'open'}) and converting that into a <template shadowrootmode=closed>, then that server is already very broken. So I don't think the "change the parameters" issue is really an issue in practice. Plus, components that understand SSD/DSD will check for a .shadowRoot and won't ever call attachShadow().

Okay, but those are not necessarily declarative shadow roots. Only one of them can be. Might just be a matter of using more precise wording then.

I agree that the wording is kind of hard to understand here. Perhaps instead of "declarative shadow roots" in that sentence, we should just use "<template shadowrootmode>'s" or something?

[emilio]

I think the ideal behaviour here would be to throw only if the init parameters are mismatched. Alternatively current behaviour and console warning, and probably note on the spec. But I find the current behaviour rather unfortunate

[mfreed7]

I think the ideal behaviour here would be to throw only if the init parameters are mismatched. Alternatively current behaviour and console warning, and probably note on the spec. But I find the current behaviour rather unfortunate

I think I could live with that (throw if parameters mismatch). I'm assuming this won't be much of a breaking change.

Are you suggesting we check all parameters, such as delegatesFocus, slotAssignment, etc.?

[annevk]

For the wording issue: yes,

template elements whose shadowrootmode attribute is appropriately set

would work I think.

@emilio having to check for all the parameters and remember to do so when we add new parameters seems like a pain, but I guess we could do it. In retrospect I'm not sure we should have supported this scenario, but it's probably too late to completely change it.

[emilio]

Are you suggesting we check all parameters, such as delegatesFocus, slotAssignment, etc.?

Yes, otherwise they get silently ignored...

[mfreed7]

I was getting started thinking about this change, and another case comes up. What should happen here?

<div>
  <template shadowrootmode=open>Root 1</template>
  <template shadowrootmode=closed>Root 2</template>
</div>

In current behavior and spec, there will be an open shadow root attached which contains "Root 2", because the "attach a shadow root" algorithm will clear out the first (open) root and insert "Root 2". We can't throw in the parser. Should this just be the console warning case?

[rniwa]

It seems like the first template should win in this case.

[annevk]

@rniwa I guess what you are suggesting is that the HTML parser completely ignores subsequent shadowrootmode template elements?

What does the parser do when a script attached a shadow root?

[mfreed7]

It seems like the first template should win in this case.
@rniwa I guess what you are suggesting is that the HTML parser completely ignores subsequent shadowrootmode template elements?

I suppose that could work, presuming you mean always ignore subsequent declarative shadow roots, not just when their parameters mismatch. It's a change in behavior, so it might not be web compatible, but it seems relatively safe. If it sounds like you're happy with that, I'll try to implement it and see what happens.

[mfreed7]

I suppose that could work, presuming you mean always ignore subsequent declarative shadow roots, not just when their parameters mismatch. It's a change in behavior, so it might not be web compatible, but it seems relatively safe. If it sounds like you're happy with that, I'll try to implement it and see what happens.

Hearing no objections, I went ahead and implemented this in Chromium, and changed/added some WPTs. I also put up a corresponding set of HTML/DOM spec PRs. To summarize the changes:

1. If two declarative shadow roots are included within a single host element, only the first one will remain. This is a change in behavior from before, but it hopefully won't encounter compat issues (TBD).
2. If attachShadow() is used on an existing declarative shadow root, and the parameters (e.g. shadow root type, delegates focus, etc.) do not match, an exception is thrown. This, also, is a breaking change, but again hopefully not one that has compat implications.

Hopefully folks are happy with these changes. If so, I'll try shipping them in the next Chrome and verify the compat safety.

[annevk]

Thanks @mfreed7, that works for us. I'll attempt to look at the PRs next week to double check all the details.

[mfreed7]

Thanks @mfreed7, that works for us. I'll attempt to look at the PRs next week to double check all the details.

Great, thanks! I enabled the new behavior (and landed the WPTs) in Chrome 122, so I'll watch for issues.