Comments (27)
I think it would be better if:
- By default they do not clone, if that is indeed the desired behavior.
- Adding a
shadowrootclonable
attribute makes them clonable.
So if you use them inside template
you'd have to add that attribute.
from html.
I think it would be better if:
- By default they do not clone, if that is indeed the desired behavior.
- 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?
from html.
That sounds reasonable to me, and makes clonable consistent with all other attributes, and with itself in the scripting and non-scripting case.
from html.
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:
- 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.
- 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.
from html.
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...
from html.
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 whatwg/dom#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>
from html.
Why did we make declarative shadow roots clonable by default? whatwg/dom#831 and whatwg/dom#1137 are not very clear on this.
Also, hasn't this already shipped as part of declarative shadow trees?
from html.
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.
from html.
Alright, I wrote that up in #10117. It'll have a merge conflict with #10069 once that lands, but I can fix it up after that. The DOM PR (whatwg/dom#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.
from html.
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?
from html.
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.
from html.
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.
from html.
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?
from html.
Thoughts:
- 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.
- 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.
from html.
Thoughts:
- 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 whatwg/dom#1252 for this.
- 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?
from html.
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 whatwg/dom#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.
from html.
@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...
from html.
@annevk I'm confused, so you don't want
attachShadow()
with a mismatchedmode
to report any error, instead eating and potentially returning a completely differentShadowRoot
than whatattachShadow
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.
from html.
@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.
from html.
@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
).
from html.
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.
from html.
So the open questions are:
- Should some ShadowRoot attributes be mutable? Which ones?
- 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?
from html.
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?
from html.
I have the opposite (but also not super-strong) preference. mode
at least seems worth raising an error for.
from html.
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.
from html.
Let's do that. I will attempt to get all the things we agreed on merged next week.
from html.
Let's do that. I will attempt to get all the things we agreed on merged next week.
Great! Thanks. I just updated whatwg/dom#1246 accordingly. Let me know if I missed something.
from html.
Related Issues (20)
- Upcoming WHATNOT meeting on 2024-09-12 HOT 1
- Request: Add Japanese and Korean Translations Links to the Wiki HOT 3
- Is it ok for `appearance:base` `<select>` not to require user activation before `showPicker()`? HOT 3
- Missing tasks in parallel steps in HTML Standard
- Meeting 8 for joint OpenUI-WHATWG/HTML-CSSWG task force on styleable form controls HOT 1
- HTML spec: section 4.11.1, for `<details>` (`HTMLDetailsElement`), should probably briefly but explicitly state that the element uses shadow DOM, and should probably link to section 15.5.5 to direct readers to further information HOT 1
- createImageBitmap's Promise resolving HOT 1
- Automated `window.exportPdf()`, Leveraging Browser's Print Save As PDF functionality HOT 3
- Provide an API for getting visible caret position in textarea/input element HOT 1
- Allowing to start cross-document transition immediately
- HTML Integration with ESM Source Phase HOT 2
- Small CSS change to spec documentation for ease of use HOT 6
- Behaviour of "reload" navigationType in " inner navigate event firing algorithm" algorithm HOT 3
- Upcoming WHATNOT meeting on 2024-09-19 HOT 1
- `colgroup` elements should map `width` attribute to the dimension property `width`
- Float16 support (WebGL, DataView, etc...) HOT 3
- BigInt serialization HOT 3
- Description corresponding of "Any other attribute" of embed element is not found HOT 1
- `beforeunload` should fire for documents with active Media Capture
- It is possible to create CloseWatchers for dialog elements and popovers which aren't in fully active document HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from html.