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.

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

FF124 shadowRootClonable and friends #33325

Merged
merged 20 commits into from
May 17, 2024

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Apr 30, 2024

This documents:

  • HTMLTemplateElement.shadowRootClonable and shadowrootclonable attribute of the <template> element - in progress
  • HTMLTemplateElement.shadowRootDelegatesFocus and shadowrootdelegatesfocus attribute of the <template> element
  • HTMLTemplateElement.shadowRootMode (attribute already done)

This is ready for review (confirmations below are mostly for stuff that predates this PR). I want to answer them, but that can happen as a post process, and the docs are useful if we can't get proper answers.

I'm still waiting on a few confirmations in https://bugzilla.mozilla.org/show_bug.cgi?id=1880188#c5 and #33325 (comment) for stuff like:

  • What are the implications if something is cloned, or it can't be cloned.
  • What happens to the focus if it is not delegated
  • The mode is accessible from the HTMLTemplateElement - can it be changed? Can you create a new shadow DOM by cloning such and element, modifying the mode, and then adding it to the DOM.

Last of all, the examples are not "Live samples" - I did write some test code for them though. I think I may be able to get this to work "Live" when some of the new safeHTML APIs are supported but they are too new.

Related docs work in #33160

@github-actions github-actions bot added Content:HTML Hypertext Markup Language docs Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Apr 30, 2024
Comment on lines 16 to 25
<!-- not sure what happens when conable vs not clonable
My assumption is that cloning the host element will also clone all the shadow root, which includes styles and so on.
For a custom element I guess that means the copied element will take all the behaviours of the original.
Not sure if that is good though. If I've created a template, then I probably want a new-fresh item each time that uses the same inherited styles.
Does cloning "share" the CSS styles of the original or is it an independent copy.

Further, if not clonablee, what happens when I clone the parent element of say a custom component. Does the custom component just not get copied and my new element is the parent element minus a shadow DOM/custom element?

Also this is false by default according to spec, but seems to be true for Chrome?
-->
Copy link
Collaborator Author

@hamishwillee hamishwillee Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saschanaz I was wondering if you could provide some insight on the implications of a shadow root being clonable or not clonable, in particular related to the shadowrootclonable attribute.

  1. The spec indicates that clonable is default false - ie. if shadowrootclonable is not specified. I see some notes around the place that it defaults to true for declarative shadow roots, but Chrome and Firefox seem to match the spec. So my question is, whether we need to record the change in behaviour in the compatibility data, and if so, what version of Chrome started being non-clonable by default.

  2. The spec says shadowRootClonable property on element reflects the shadowrootclonable attribute on <template>. But my tests show that the value can be mutated. So should this say "initially reflects"?

  3. My understanding is that if the the shadow root is clonable you're saying that when you clone the host element you also get everything in the tree below that root. But what are the implications of cloning, and also of not cloning - both generally and in the context of custom elements.

If something is clonable I'd assume that you get a duplicate of the original tree. So if you had a custom element with a clonable shadow root you'd end up with the same CSS, structure etc. Assuming its a true clone, presumably there is no sharing of the CSS and so on, so in theory the elements are independent. Perhaps this means you can't then change all instances of your custom element from one element?

If it isn't clonable then the docs seem to indicate that the shadow root just won't get copied. What does that mean for a custom element though? The docs show custom elements attaching their own shadow root by attaching a shadow in their constructor. Is that code run again if the custom element is cloned?

Any advice on what needs to be said would be much appreciated. I hope to keep it all here in the ShadowRoot.clonable topic, and then link here where clonable is set.

There is also a question below #33325 (comment) about whether or not you can change the mode using JavaScript. Docs for template indicate not, but the property is not read-only in IDL

EDITED: PS Also asked this stuff on https://bugzilla.mozilla.org/show_bug.cgi?id=1880188#c5 and linked here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS Note, me not getting this information should not prevent merging. But I think our docs will remain flawed until we do explain the implications of clonable/not clonable for web components and for other shadow root cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some notes around the place that it defaults to true for declarative shadow roots

In MDN, right? If in the spec I would be interested to know where it mentions such difference.

but Chrome and Firefox seem to match the spec. So my question is, whether we need to record the change in behaviour in the compatibility data, and if so, what version of Chrome started being non-clonable by default.

@emilio pointed out whatwg/html#10107 changed the behavior, and seemingly the corresponding Chromium change happened in https://chromium-review.googlesource.com/c/chromium/src/+/5260748. I can't easily map the merge date Feb 10 into Chrome release version, but per the schedule maybe Chrome 123?

2. The spec says `shadowRootClonable` property on element reflects the `shadowrootclonable` attribute on `<template>`. But my tests show that the value can be mutated. So should this say "initially reflects"?

That's mentioned in the above issue; the intention seems to be that the mutation will take affect when the next clone happens.

If something is clonable I'd assume that you get a duplicate of the original tree. So if you had a custom element with a clonable shadow root you'd end up with the same CSS, structure etc. Assuming its a true clone, presumably there is no sharing of the CSS and so on, so in theory the elements are independent. Perhaps this means you can't then change all instances of your custom element from one element?

AFAIK no magical element reference, things are cloned and independent.

If it isn't clonable then the docs seem to indicate that the shadow root just won't get copied. What does that mean for a custom element though? The docs show custom elements attaching their own shadow root by attaching a shadow in their constructor. Is that code run again if the custom element is cloned?

Not sure what doc you mean, but I guess it's about existing custom element constructor and lifecycle callbacks e.g. connectedCallback? I don't see how clonable=false would affect the callback.

But in general I'm not super familiar with declarative shadow dom, @avandolder or @emilio should know better than I do.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilio pointed out whatwg/html#10107 changed the behavior, and seemingly the corresponding Chromium change happened in https://chromium-review.googlesource.com/c/chromium/src/+/5260748. I can't easily map the merge date Feb 10 into Chrome release version, but per the schedule maybe Chrome 123?

This shipped in Chrome v124. That CL did add the behavior, but the flag was flipped later.

2. The spec says `shadowRootClonable` property on element reflects the `shadowrootclonable` attribute on `<template>`. But my tests show that the value can be mutated. So should this say "initially reflects"?

That's mentioned in the above issue; the intention seems to be that the mutation will take affect when the next clone happens.

Importantly, the shadowRootClonable IDL reflection changes the shadowrootclonable content attribute on an HTMLTemplateElement that already exists in the DOM. At that point, none of the declarative shadow DOM behaviors do anything since it is no longer being parsed. So yes you can mutate shadowRootClonable, but that doesn't change the clonable bit on any shadow root, because there's no shadow root at all. Just a <template> element.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mfreed7 and @saschanaz - much appreciated.

  1. On the topic of IDL reflection etc, I get that now (thanks @mfreed7 - you sorted that for me in the another issue).

  2. @saschanaz - yes, MDN was the only place that talked about clonable being true by default. Fixed now.

  3. Given the change to make clonable false by default is in 124 and this is also the first release where clonable could be specified in attachShadow() and exposed on ShadowRoot, I propose I add a note to the init.clonable parameter in the compatibility data associated with that release something like:

    "Before version 124 ShadowRoots were always clonable.".

Copy link
Collaborator Author

@hamishwillee hamishwillee May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it isn't clonable then the docs seem to indicate that the shadow root just won't get copied. What does that mean for a custom element though? The docs show custom elements attaching their own shadow root by attaching a shadow in their constructor. Is that code run again if the custom element is cloned?

Not sure what doc you mean, but I guess it's about existing custom element constructor and lifecycle callbacks e.g. connectedCallback? I don't see how clonable=false would affect the callback.

The problem is that I don't have a mental model of what happens when you clone a web component, so I it isn't clear to me whether you should set something as clonable or not, and when. Also not clear to me the cases where you might have a declarative shadow root and also attach it.

If you look at An autonomous custom element in "Using custom elements" it shows creation of a popup element. In the connectedCallback() we attach a shadow DOM where the shadow root is open but not cloneable.

According to that page, this callback is called when you attach the element to the page. So when you clone a node containing our element what happens? I presume nothing - when the node is added to the page is when the connectedCallback() will run to set up the web component/element?

@saschanaz Is that why you think it is irrelevant whether the root is clonable or not for this case? If so, then thanks, we're done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS This PR is OK though, so I'll merge it and take any additional responses into the related PRs. Thanks again for your help.

@hamishwillee hamishwillee marked this pull request as ready for review May 3, 2024 07:11
@hamishwillee hamishwillee requested review from a team as code owners May 3, 2024 07:11
@hamishwillee hamishwillee requested review from wbamberg and chrisdavidmills and removed request for a team May 3, 2024 07:11
Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done @hamishwillee; good quality docs, all makes sense, nothing too major in the comments, pending the confusion over the shadowRootMode behavior.

files/en-us/web/api/htmltemplateelement/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmltemplateelement/index.md Outdated Show resolved Hide resolved
files/en-us/web/html/element/template/index.md Outdated Show resolved Hide resolved
files/en-us/web/html/element/template/index.md Outdated Show resolved Hide resolved
files/en-us/web/html/element/template/index.md Outdated Show resolved Hide resolved
files/en-us/web/html/element/template/index.md Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator Author

@chrisdavidmills This is good to review. I'm going to tidy part of it in #33600 (which adds a shadowrootserializable).

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @hamishwillee; I think you've dealt with the shadow root property weirdness very well. I just highlighted one small language issue; approving!

files/en-us/web/api/htmltemplateelement/index.md Outdated Show resolved Hide resolved
@hamishwillee hamishwillee force-pushed the ff125_shadow_root_clonable branch from 08798bb to 79e565f Compare May 17, 2024 01:08
@hamishwillee hamishwillee merged commit 26091e4 into mdn:main May 17, 2024
8 checks passed
@hamishwillee hamishwillee deleted the ff125_shadow_root_clonable branch May 17, 2024 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:HTML Hypertext Markup Language docs Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants