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.

Sign up for GitHub

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

Jump to bottom

template element declarative shadow tree IDL attributes #10211

Closed
annevk opened this issue Mar 19, 2024 · 8 comments
Closed

template element declarative shadow tree IDL attributes #10211

annevk opened this issue Mar 19, 2024 · 8 comments
Labels
removal/deprecation Removing or deprecating a feature topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@annevk
Copy link
Member

annevk commented Mar 19, 2024

I was looking at this again today and I'm not sure how these make sense. At least historically we never added IDL attributes for parser-only features: <html manifest>. Why would we do so here?

By the time the template element is parsed it's no longer there so the attributes are not useful. And if you create a template element imperatively it's not going to be useful as parser input. Feature testing can be done by running the parser.

I suggest we try removing them.

(WebKit only ever shipped shadowRootMode.)

cc @smaug---- @rniwa @mfreed7 @emilio
@annevk annevk added removal/deprecation Removing or deprecating a feature topic: shadow Relates to shadow trees (as defined in DOM) labels Mar 19, 2024
@mfreed7
Copy link
Contributor

mfreed7 commented Mar 19, 2024

So recall that shadowroot was renamed to shadowrootmode precisely and only to make room for the IDL shadowRootMode reflection. As I pointed out at the time, there's no developer-facing reason to need this. However, we did go through with renaming the attribute, with significant work and ecosystem pain to migrate code to the new name, and deprecate the old name.

Given that we already went through all of that, I'd hope we wouldn't decide that now we don't need IDL reflections after all. And I'd hope we'd stick to one concept for all of the <template> attributes, meaning we keep all of the reflections such as shadowRootClonable.

I.e. let's just keep the IDL reflections as we've spec'd them, please.

@annevk
Copy link
Member Author

annevk commented Mar 19, 2024

That link doesn't work for me. And shadowrootmode still seems like the right attribute name. I'm just not convinced we need these IDL attributes. At least historically we decided against adding them for parser-only features.

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 19, 2024

That link doesn't work for me.

Try again - it looks like PR comments have to be "Unresolved" for links to work? @keithamus is that a known issue?

And shadowrootmode still seems like the right attribute name. I'm just not convinced we need these IDL attributes. At least historically we decided against adding them for parser-only features.

The point was that the name got renamed from shadowroot because there was concern that shadowRoot returned null instead of a string, so the entire feature had to be renamed. shadowrootmode is also a perfectly fine name, but without the reflection conflict, there would have been no need to unship the existing feature.

@annevk
Copy link
Member Author

annevk commented Mar 19, 2024

Thanks! Quite interesting. @rniwa wrote there:

Assuming that we'd eventually add a serialization mechanism for shadow DOM, let it be getInnerHTML or something else, it seems plausible that these template elements are encountered by author scripts. From purely ergonomic standpoint, it would be confusing for this IDL attribute on template element to not reflect a content attribute of the same name.

But I don't really see it. I don't think there's ever a case where you interact with a template element and getting or setting these IDL attributes is meaningful.

@domenic +1'd rniwa's statement, but also added:

It also avoids the problem of adding an IDL attribute only for feature detection, which is an antipattern.

Which is where I'm stuck on. I think these IDL attributes don't have a valid use case.

You're correct that shadowroot could also have worked, but I like that all attributes have a consistent prefix now.

@annevk annevk added the agenda+ To be discussed at a triage meeting label Mar 21, 2024
@annevk annevk mentioned this issue Mar 21, 2024
Merged
5 tasks
@mfreed7
Copy link
Contributor

mfreed7 commented Mar 21, 2024

But I don't really see it. I don't think there's ever a case where you interact with a template element and getting or setting these IDL attributes is meaningful.

I agree.

It also avoids the problem of adding an IDL attribute only for feature detection, which is an antipattern.

Which is where I'm stuck on. I think these IDL attributes don't have a valid use case.

They never had real use cases - that was pretty clear from the prior discussion. They were added for purity reasons - other content attributes have IDL reflections, so these should also. Has that changed?

@annevk
Copy link
Member Author

annevk commented Mar 21, 2024

Well, <html manifest> (now removed) predates these attributes by a fair number of years and never had an IDL attribute precisely because it was a parser instruction only. To me that makes more sense as that also hints at developers that these attributes are special. I don't think there's any other meaningful precedent.

@domenic
Copy link
Member

domenic commented Mar 23, 2024

I think keeping these is the right move.

  1. The precedent set by <html manifest> is not very strong. It's a single attribute, and it's dead and has been for years. Being consistent with that dead attribute's choice to be inconsistent with the rest of the platform seems unimportant to me. In other words, we don't have any live precedents of parser-only attributes that we're trying to be consistent with, so in the absence of such precedents, sticking with the usual behavior all attributes have makes more sense.

  2. I think there are use cases for manipulating shadowRootMode and friends from JavaScript. They are basically around building up HTML strings that you plan to later give to the parser. For example:

    const t = document.createElement("template");
t.shadowRootMode = "open";
t.setHTMLUnsafe(...);
const templateSerialized = t.getHTML();

// Now maybe write `templateSerialized` to disk, or
// if you're doing server-side rendering send it to the client,
// or if you're doing some sort of "assemble the DOM in one frame
// and send it across to another frame" strategy you can do that, or...

// Later:
someParent.setHTMLUnsafe(templateSerialized);

  3. Somewhat synthesizing the above points, I think there are parts of the larger HTML ecosystem (outside browsers per se) that work best when every attribute is reflected. The JSX and React model, in particular, strongly favors properties over attributes. My understanding is that server-side rendering in these ecosystems works most seamlessly when you can write JSX something like return <template shadowRootMode={myShadowRootMode}>...</template> on the server, and then the framework converts that into basically Object.assign(document.createElement("template"), { shadowRootMode: myShadowRootMode }).getHTML() as a response body. If we omitted a property, then these sorts of frameworks would need some hacky workarounds to be able to set shadowrootmode="" at all. This is not really a strong argument by itself, but I think in the absence of any real gains from omitting the property, it helps tip the balance toward being consistent with the rest of the platform.

@annevk annevk removed the agenda+ To be discussed at a triage meeting label Mar 25, 2024
@annevk
Copy link
Member Author

annevk commented Mar 25, 2024

That's somewhat compelling, 2 in particular. Okay, happy to drop this.

@annevk annevk closed this as not planned Won't fix, can't repro, duplicate, stale Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
removal/deprecation Removing or deprecating a feature topic: shadow Relates to shadow trees (as defined in DOM)
Milestone
No milestone
Development

No branches or pull requests
4 participants
@domenic @annevk @mfreed7 and others