Expose shadowRoot on element internals

Question

Expose shadowRoot on element internals

JanMiksovsky opened this issue 5 years ago · 59 comments

During the March 2020 web components virtual F2F, we discussed the fact that an element with a closed shadow root has no standard way to refer to its own shadow root:

class MyElement extends HTMLElement {
  constructor() {
    super();
    this.attachShadow({ mode: "closed" });
  }

  connectedCallback() {
    // Other callbacks/methods can't access shadow.
    this.shadowRoot.innerHTML = "Hello"; // Throws: Can't set innerHTML of null
  }
}

Because of this, any component that creates a closed root must maintain its own private reference to the root.

Additionally, the possibility of declarative Shadow DOM introduces a scenario in which a rehydrated component may need access to a shadow root that was automatically created for it.

For these reasons, it seems worthwhile to expose shadowRoot on an element's internals:

class MyElement extends HTMLElement {
  #internals;

  constructor() {
    super();
    this.attachShadow({ mode: "closed" });
    this.#internals = this.attachInternals();
  }

  connectedCallback() {
    // Other callbacks/methods can access shadow via internals.
    this.#internals.shadowRoot.innerHTML = "Hello";
  }
}

As before, the component must still maintain a private reference (to internals instead of the shadow root), but at least now:

A component that wants to have both a closed shadow root and internals has to track only a reference to the internals. Through that, it can always obtain a reference to the shadowRoot.
A component with declarative Shadow DOM could invoke attachInternals and then inspect the internals to determine whether a shadow root has already been created.

Answer 1 · 2020-03-25T02:26:23.000Z

We (Apple's WebKit team) initially suggested two alternatives to this approach:

attachShadow during custom element construction replaces the declarative shadow DOM
attachShadow during custom element constructor returns the declarative Shadow DOM instead of creating one.

We'll review this alternative approach (exposing shadow root on ElementInternals).

Answer 2 · 2020-03-25T18:26:02.000Z

@rniwa

attachShadow during custom element constructor returns the declarative Shadow DOM instead of creating one.

that seems a lot more complicated, Element.prototype.shadowRoot getter will return the instance during construction, and return null after the construction phase when declarative is closed. Sounds like hazardous for developers.

Answer 3 · 2020-03-26T02:27:29.000Z

@rniwa

attachShadow during custom element constructor returns the declarative Shadow DOM instead of creating one.

that seems a lot more complicated, Element.prototype.shadowRoot getter will return the instance during construction, and return null after the construction phase when declarative is closed. Sounds like hazardous for developers.

That's not at all what I'm suggesting. I'm suggesting that attachShadow would return the existing declarative shadow root instead of creating a new one.

Answer 4 · 2020-03-26T16:54:50.000Z

@rniwa During Mason's presentation, we mentioned that the problem with this approach is that it is not backward compatible, components that are not taking declarative shadow root into consideration, will fail because they will assume an empty shadowRoot was created.

Answer 5 · 2020-03-26T17:05:24.000Z

@caridy I don't see how that will ever happen. A component will necessarily have to cooperate to produce a declarative version of its shadow root. How else would it get a valid one?

Answer 6 · 2020-03-26T18:37:46.000Z

@justinfagnani as we discussed in the meeting, we believe that it should be backward compatible, letting existing components and simple components to function even when used through SSR via declarative shadow root. Yes, the component will wipe out the existing shadow if the component doesn't take it into consideration, that's fine. @mfreed7 agreed with that IIRC. We should probably have this issue in the declarative shadow root proposal repo, I don't think that decision affects this request for element internals to expose the shadow root.

Answer 7 · 2020-03-26T18:56:58.000Z

@caridy can you outline how a component would even get a valid shadow root generated for it without cooperation from the element or framework it's built in? It affects this feature request because creating a new shadow root is a very new behavior.

Answer 8 · 2020-03-26T23:22:05.000Z

Thanks @JanMiksovsky for opening this issue. It seems that there are two relevant potential use cases here:

Existing components, built without knowledge of declarative Shadow DOM. These call this.attachShadow() and assume a. it does not throw, and b. it returns an empty shadow root. It then fills the shadowroot with shadow content.
Components that are built with knowledge of declarative Shadow DOM, and that wish to use a closed shadow root. In this case, the component will want to retrieve the existing (closed) shadowRoot so that it can verify the content and/or hook up events and references. It does not want to blow away and re-create existing declarative content.

It would seem that the most clean API to address both cases above would be:

For use case #1, make attachShadow() blow away any existing declaratively-created shadow root, and return a new, empty shadowroot. (Alternatively, this could delete the existing contents of the shadowroot, and return a reference to the existing shadowroot.)
For use case #2, add a shadowRoot accessor to ElementInternals, to allow "declarative aware" components to retrieve their closed shadow root. Alternatively, an extra argument could be added to attachShadow() which causes it to retrieve the existing shadow root without removing its contents. Either would seem to achieve the goal. Though ElementInternals.shadowRoot does seem considerably better, ergonomically.

@justinfagnani, I could imagine a framework that simply renders all content server-side, and then calls getInnerHTML() to retrieve the declarative content to send down to the client. In this case, the component and the framework would not need to "coordinate", and you could then get use case #1 above that wakes up with declarative content but doesn't know how to deal with it.

Answer 9 · 2020-03-26T23:41:03.000Z

It seems awkward that all SSR aware components would have to write code like this:

class CustomHTMLElement extends HTMLElement {
    #internals;

    constructor() {
        super();
        #internals = this.attachInternals();
        if (!#internals.shadowRoot)
            this.attachShadow({mode: 'closed'});
    }
}

With the alternative I'm proposing (make attachShadow return the exiting declarative shadowRoot), we could something like this (obviously with a shorter name):

class CustomHTMLElement extends HTMLElement {
    #shadowRoot;

    constructor() {
        super();
        #shadowRoot = this.attachShadow({mode: 'closed', returnExistingDeclarativeShadowRoot: true});
    }
}

We could do both too.

Answer 10 · 2020-03-26T23:48:47.000Z

Many templating libraries that support hydration will still need to branch on some factor to either call the initial rendering or hydrating entrypoint. The existence of a shadow root is a pretty decent signal for this, IMO.

Answer 11 · 2020-03-26T23:59:28.000Z

@rniwa , just to double-check your proposal:

#shadowRoot = this.attachShadow({mode: 'closed', returnExistingDeclarativeShadowRoot: true});

will return the existing declarative shadow root, with contents included.

#shadowRoot = this.attachShadow({mode: 'closed'});

will instead blow away the existing contents of the shadow root and return an empty root. In both cases, no exception will be thrown if a declarative shadow root already exists. However, if there is already an imperatively-created shadow root, these will both throw, as they do today.

Is that right? If so, I think I'm ok with that proposal also.

Answer 12 · 2020-03-27T00:01:40.000Z

In both cases, no exception will be thrown if a declarative shadow root already exists. However, if there is already an imperatively-created shadow root, these will both throw, as they do today.

Right.

Many templating libraries that support hydration will still need to branch on some factor to either call the initial rendering or hydrating entrypoint. The existence of a shadow root is a pretty decent signal for this, IMO.

Now I realize the situation is a lot more complicated than that because of the sync parsing behavior in which case the custom element gets created before any of its children get parsed. In that scenario (non-upgrading case), you'd have to wait until all the children had been parsed for hydration.

Answer 13 · 2020-03-27T00:05:04.000Z

In that scenario (non-upgrading case), you'd have to wait until all the children had been parsed for hydration.

With the current declarative SR proposal, this is fine, as the entire shadow root is created when all the shadow children are parsed.

If we found an acceptable way to do streaming, this would be an issue to tackle too.

Edit: I suppose the constructor could be called before the declarative shadow root is created at all. This would be a great time to add the childrenParsed or whatever callback. That's a problem that custom element authors have been hitting for a while now.

Answer 14 · 2020-03-27T00:15:26.000Z

In that scenario (non-upgrading case), you'd have to wait until all the children had been parsed for hydration.

With the current declarative SR proposal, this is fine, as the entire shadow root is created when all the shadow children are parsed.

No, it's not fine because the constructor of a custom element is always called before its children are parsed or created so in sync/non-upgrade construction case, the constructor will always run and then it can attach a non-declarative shadow root.

If we found an acceptable way to do streaming, this would be an issue to tackle too.

Edit: I suppose the constructor could be called before the declarative shadow root is created at all.

This is not could. It's guaranteed to happen in that order.

This would be a great time to add the childrenParsed or whatever callback. That's a problem that custom element authors have been hitting for a while now.

That doesn't necessarily solve the problem fully because then the custom element has to delay attaching a shadow root until that happens in the case there is a declarative shadow root, in which case, someone else can attach a shadow root, or worse yet, anyone can access to template's content and have reference to those nodes.

Answer 15 · 2020-03-27T01:02:04.000Z

in which case, someone else can attach a shadow root, or worse yet, anyone can access to template's content and have reference to those nodes.

Declarative shadow roots are necessarily a cooperative construct - the generator of the shadow root and the element have to coordinate - and I'm definitely not worried about breaking encapsulation this way. A component will have to opt-in to SSR somehow and have to trust the declarative SR contents. If a component author is worried that there will somehow be an attack via declarative SR, then they should not support it, ie, it should throw when they call attachShadow as it does now.

This is why I don't see a forgiving version of attachShadow as necessary. The component should only call that if it knows it's not going to receive a declarative SR.

Answer 16 · 2020-03-27T02:22:10.000Z

in which case, someone else can attach a shadow root, or worse yet, anyone can access to template's content and have reference to those nodes.

Declarative shadow roots are necessarily a cooperative construct - the generator of the shadow root and the element have to coordinate - and I'm definitely not worried about breaking encapsulation this way.

Breaking encapsulation in this scenario would definitely be a show stopper for us.

A component will have to opt-in to SSR somehow and have to trust the declarative SR contents. If a component author is worried that there will somehow be an attack via declarative SR, then they should not support it, ie, it should throw when they call attachShadow as it does now.

This is not about the malicious case of something attacking since there is no security boundary right now. However, it's very easy for some residual script on a page to start accessing random nodes via MutationObserver, etc...

Answer 17 · 2020-04-02T22:42:49.000Z

I've just posted a larger comment on the 831 issue, which should be read before this one for context. But I wanted to point out one problem with the attachShadow({mode: 'closed', returnExistingDeclarativeShadowRoot: true}); version of this feature. That will not work for the case where there is an imperative/declarative race, and the imperative code "wins". In that case, the component will likely want to detect that the declarative content hasn't loaded, and wait until later to hydrate. But there will be no way to detect that situation, since calling attachShadow() will actually attach a shadow root and block the declarative content. If, on the other hand, we go with an ElementInternals.shadowRoot accessor, this situation can be properly detected no matter who wins the race.

Answer 18 · 2020-08-11T16:46:32.000Z

I would like to re-iterate my comment, and propose that we solve this issue by adding the following to the ElementInternals interface:

interface ElementInternals {
  readonly attribute ShadowRoot? shadowRoot;
};

internals.shadowRoot: Returns internals's target element's shadow root. This attribute will return both open and closed shadow roots. If shadow is null, then return null.

If there is general agreement on this, then I will put together a spec PR, and will get this implemented in Chromium.

Answer 19 · 2020-08-13T17:45:02.000Z

To allow testing, I've implemented this in Chromium behind the DeclarativeShadowDOM flag, with WPT tests. Very straightforward implementation, and I believe it solves the issues brought up here. But comments appreciated.

Answer 20 · 2020-08-14T13:53:46.000Z

So this would couple encapsulation of element internals and closed shadow trees. It seems there is some weirdness in that previously encapsulated shadow trees could end up revealed through this. Not sure that's a showstopper.

Answer 21 · 2020-08-14T18:36:56.000Z

@annevk this is very interesting, but first let me see if I fully understand the problem here:

a component that extends another component, and attaches a shadow root with mode closed, could be leaking the shadow root instance in the super inadvertently if the super attaches the element internals
a component that is extended by another component, could always access its shadow root attached from a subclass via element internals.

Is that an accurate assessment of the encapsulation issue?

Note: I believe this leakage is mostly a problem already if the super changes the attachShadow method, to intersect calls and gain access to the shadowRoot.

Answer 22 · 2020-08-17T05:31:32.000Z

I wasn't really thinking about subclassing. I was thinking about a custom element with a shadow root that does not prevent "internals" from being attached.

I'm not sure how the subclassing scenario would work. How would the subclass get access to the internals/shadow root of the super?

Answer 23 · 2020-08-17T15:11:43.000Z

I wasn't really thinking about subclassing. I was thinking about a custom element with a shadow root that does not prevent "internals" from being attached.

@annevk I thought element internals attachment was only possible during the construction phase, in which case, anyone getting a handle on the element after construction should not be able to gain access to the shadow root, unless they subclass it.

I'm not sure how the subclassing scenario would work. How would the subclass get access to the internals/shadow root of the super?

If the super doesn't attach the internals, the subclass could just for the purpose of gaining access to the shadow root instance. But since most code uses dot notation, it is already possible for a subclass to trick the super by providing its own implementation of attachShadow, or attachInternals.

Answer 24 · 2020-08-17T15:14:13.000Z

I don't see that in https://html.spec.whatwg.org/#dom-attachinternals.

Answer 25 · 2020-08-17T15:23:31.000Z

oh wow! for some reason I clearly remember that part of the conversation about restricting who can call it and when to call it since it has the is value check anyways. Alright! I suspect it is too late to resurrect that discussion (I'm curious to know the reasons why was that dropped along the way).

Give the current state, is it a real problem? It seems to me that now anyone who wants to truly encapsulate their shadow must also attach internals just to prevent others from accessing the shadow root :(

Answer 26 · 2020-08-17T18:43:55.000Z

FWIW, only Chrome has shipped attachInternals so far (Firefox & Safari haven't) so I think we can consider changing the behavior (and possibly rename it if there is a compatibility concern).

Answer 27 · 2020-08-18T21:51:01.000Z

So the spec and the implementation definitely allow attachInternals() from anywhere, not just the constructor. Mainly for my own benefit, but also for the benefit of this thread, I reviewed the history on the original issue thread:

The first comment discussing this fact is here from @caridy. A bit later, @caridy also suggests the attachInternals() name and API, and mentions that it should be called from the constructor. A number of immediately subsequent comments from @annevk and @rniwa are supportive of the "constructor only" limitation. This comment from @domenic brings up that it might be difficult to implement, and @tkent-google confirms here that implementation is difficult. And then the key point: @domenic reported back from TC39 with some further input, including that the prior proposals didn't work with subclassing. And the statement that attachInternals() works like attachShadow(): if the custom element constructor doesn't call either of those, anybody else is free to call them later, with whatever consequences that entails. Both @annevk and @rniwa comment that they are concerned that this means custom element authors all need to call attachInternals() protectively, and instead they preferred a needsInternals attribute to opt elements in to ElementInternals. After some discussion, the discussion switches to an "opt-out" that is more ergonomic than having custom elements call attachInternals() and throw away the result. That discussion bikesheds for a while on naming before petering out without a conclusion, here. And the API is implemented without an opt-in or opt-out. The conclusion is therefore that if custom element authors are concerned about exposing ElementInternals, they should make sure to call attachInternals() in the constructor.

Given the above, the current situation is that both attachShadow() and attachInternals() are available for anyone to call. Protection is afforded to custom elements through the fact that they can call both in the constructor, and subsequent calls throw exceptions.

Ok, that's the history - now back to this proposal. Closed shadow roots can be protected by calling attachInternals(), and the proposed ElementInternals.shadowRoot should be as safe as anything else in ElementInternals.

Thoughts?

Answer 28 · 2020-08-18T23:02:47.000Z

Regardless of the past discussion about attachInternals, it seems problematic that anyone can just call attachInternals and gain access to a closed shadow root. In particular, between the time a shadow root is attached and custom element's definition gets loaded (i.e. customElements.define is called), anyone can come in & call attachInternals.

Now that I'm thinking about this problem more, perhaps the easiest solution is to forbid ElementInternals to be created unless the underlying element's custom element state is "custom". This forces custom element's constructor to be the very first thing to have the ability to call attachInternals.

Answer 29 · 2020-08-18T23:20:03.000Z

That discussion bikesheds for a while on naming before petering out without a conclusion, here. And the API is implemented without an opt-in or opt-out.

Isn’t the opt-out specified already in HTML, though?

(https://html.spec.whatwg.org/multipage/custom-elements.html#element-definition in step 14)

In Chrome (86.0.4235.0) this appears to work:

[...] the easiest solution is to forbid ElementInternals to be created unless the underlying element's custom element state is "custom". This forces custom element's constructor to be the very first thing to have the ability to call attachInternals.

That seems to be the case in the Chrome impl. For shadow attachment it’s a bit weirder cause there’s no similar constraint:

This appears to be specified here, step 3.

The can’t-upgrade-cause-shadow-exists case is handled at step 8.1 in “upgrade an element”.

Answer 30 · 2020-08-19T02:55:12.000Z

Thanks for digging that up @mfreed7! And yeah, as @bathos shows we did in fact add the opt-out.

@rniwa until define() is called there is no definition for the element, right? attachInternals() will throw if that's the case. So that case is already covered as far as I can tell.

Being able to call attachInternals() makes you pretty powerful already. The main question is whether that power should be equivalent to shadow tree access. Again, that might well be okay. I just want everyone to be conscious of the implications.

Answer 31 · 2020-08-19T05:16:32.000Z

Thanks for digging that up @mfreed7! And yeah, as @bathos shows we did in fact add the opt-out.

@rniwa until define() is called there is no definition for the element, right? attachInternals() will throw if that's the case. So that case is already covered as far as I can tell.

define being called doesn't mean the constructor is the only thing running. While each element is getting upgraded, other scripts can access other yet-to-be-upgraded elements.

Answer 32 · 2020-08-19T05:40:13.000Z

@rniwa, good catch ... looks like this issue can be demonstrated in Chrome’s implementation currently like this:

x = document.createElement('x-');

customElements.define('x-', class extends HTMLElement {
  constructor() {
    super().attachInternals();
  }
});

x.attachInternals(); // works
customElements.upgrade(x); // throws

Answer 33 · 2020-08-19T22:20:36.000Z

Ahh great - I didn't realize the opt-out got implemented after all. Thanks for the pointers. So elements can easily opt out of internals, or call attachInternals() in the constructor to prevent leakage of the shadowRoot.

I do agree with the @rniwa / @bathos example - attachInternals() can be called by someone else between the call to define() and the element actually being upgraded. That's not great. Is there any problem with @rniwa's suggestion here?

Now that I'm thinking about this problem more, perhaps the easiest solution is to forbid ElementInternals to be created unless the underlying element's custom element state is "custom". This forces custom element's constructor to be the very first thing to have the ability to call attachInternals.

This seems like it would fix the above problem.

Answer 34 · 2020-08-20T03:13:20.000Z

I do agree with @rniwa's suggestion as the minimum change needed to unblock this. That change is not backwards compatible though... but only breaks for very edge cases, I hope that chrome can take that bullet at this point. Thoughts on that?

As for some extra restrictions, what else can be done? I do remember, vaguely, the discussions about subclassing during tc39 meeting. I just think that the fact all existing components not using internals, but relying on closed shadow root are subject to leaking their implementations details, to be concerning.

So elements can easily opt out of internals, or call attachInternals() in the constructor to prevent leakage of the shadowRoot.

Moreover, attaching internals is not for free (in terms of perf), now you need to attach internals whenever you use closed shadow as a way to protect yourself even thought you're not going to use the internals object at all, that is also concerning. disabling the internals via the static field is ok for some scenarios, but does not work well when subclassing either since the subclass can revoke the super's definition.

Answer 35 · 2020-08-20T08:12:43.000Z

@caridy In situations where I’ve needed an element to be truly encapsulated, it’s also been effectively unsubclassable (because the reactions are themselves needed to be reliable/private — CE reaction methods deleted after registration). I’ve just gone ahead and added a new.target requirement to those elements to throw the same Illegal constructor TypeError you’d get trying to register a subclassed native element. I’m having trouble imagining a scenario where “doesn’t have internals” is important but subclassing is actually supported, since almost any meaningfully subclassable CE already must be leaky somewhere for that to work. Or another way to put that is that “the subclass can revoke the super's definition” is just as true for, say, “subclass can invoke super.attributeChangedCallback with altered arguments or not at all”.

Answer 36 · 2020-08-20T13:57:30.000Z

Is there any problem with @rniwa's suggestion here?

I would be interested in seeing a specific proposed change, i.e. what algorithm steps to modify. As suggested I believe it is a no-op, as a custom element's state becomes "custom" at the same observable time as the definition becomes present. But, I might be missing something, so a specific spec PR/tests would be helpful.

Answer 37 · 2020-08-20T23:43:10.000Z

Is there any problem with @rniwa's suggestion here?

I would be interested in seeing a specific proposed change, i.e. what algorithm steps to modify.

The change is very simple. We'd add a new step immediately below [step 1 of attachInternals](https://html.spec.whatwg.org/multipage/custom-elements.html#dom-attachinternals as follows:
2. If element's custom element state is not "custom", throw "NotSupportedError" DOMException.

As suggested I believe it is a no-op, as a custom element's state becomes "custom" at the same observable time as the definition becomes present. But, I might be missing something, so a specific spec PR/tests would be helpful.

With the above change, the following test should pass:

const someElement = document.createElement('some-element');

customElements.define('some-element', class extends HTMLElement {
  constructor() {
    super().attachInternals();
  }
});

assert_throws('NotSupportedError', () => someElement.attachInternals());

Answer 38 · 2020-08-21T00:16:45.000Z

@rniwa I see that, like you said, the custom element state isn’t set to "custom" till upgrade here. Using that as the condition does appear to prevent someElement.attachInternals(). But it looks like it would prevent attaching in the constructor, too.

In Upgrade an element, the state is set to “failed” at step 3 (weird, but the note explains it takes care of an early exit for reentrancy). At step 6 the constructor is added to the stack and at step 8, the constructor runs. It isn’t till step 10, though, that — provided everything went swell — it changes from “failed” to “custom”.

I think that custom element state doesn’t (currently) align with the point where attachment should become available (unless I’m missing something). The gist makes sense to me but maybe a distinct flag would be needed?

Answer 39 · 2020-08-21T00:29:55.000Z

Right. There's no point in specs where we can express "after new X()" since X is entirely author code. The best we can do is "after super()" since that's code we control.

Answer 40 · 2020-08-21T00:33:54.000Z

@rniwa I see that, like you said, the custom element state isn’t set to "custom" till upgrade here. Using that as the condition does appear to prevent someElement.attachInternals(). But it looks like it would prevent attaching in the constructor, too.

In Upgrade an element, the state is set to “failed” at step 3 (weird, but the note explains it takes care of an early exit for reentrancy). At step 6 the constructor is added to the stack and at step 8, the constructor runs. It isn’t till step 10, though, that — provided everything went swell — it changes from “failed” to “custom”.

I think that custom element state doesn’t (currently) align with the point where attachment should become available (unless I’m missing something). The gist makes sense to me but maybe a distinct flag would be needed?

Oh right. I was thinking of the synchronous construction case only. We probably need to introduce a new custom element state like "precustomized" and set to it right before step 8.2 ("Let constructResult be the result of constructing C, with no arguments.") of upgrading an element. Then the change I'm suggesting will check whether the custom element state is either custom or precustomzied.

Answer 41 · 2020-08-21T01:01:26.000Z

@domenic It took a while for what you were saying to click. I suspect I got myself confused by forgetting to account for the fact that the upgrade algorithm still hits the regular HTMLConstructor construction steps if all goes correctly, and those steps are setting the state to "custom". This happening at super() is what I’d expect. I'm now pretty confused by the purpose of step 10 of upgrade an element.

Sorry @rniwa, my previous comment was indeed just a case of me missing something. AFAICT now I’m pretty sure the one-small-change solution would already work without an extra state.

Answer 42 · 2020-08-31T19:04:24.000Z

Oh right. I was thinking of the synchronous construction case only. We probably need to introduce a new custom element state like "precustomized" and set to it right before step 8.2 ("Let constructResult be the result of constructing C, with no arguments.") of upgrading an element. Then the change I'm suggesting will check whether the custom element state is either custom or precustomzied.

After re-reading Upgrade an element a few times, I think the above suggestion sounds like it'll work. If there's general agreement here, I'm happy to put together a spec PR, and modify the Chromium implementation. Any objections? @domenic?

Answer 43 · 2020-08-31T21:09:12.000Z

I thought I'd update the code examples from #871 (comment).

class CustomHTMLElement extends HTMLElement {
	#internals = this.attachInternals()
	#shadow = this.#internals.shadowRoot ?? this.attachShadow({mode: 'closed'})
}

vs

class CustomHTMLElement extends HTMLElement {
	#existingShadow = this.attachShadow({returnExistingDeclarativeShadowRoot: true})
	#shadow = this.#existingShadow ?? this.attachShadow({mode: 'closed'})
}

And if @rniwa you meant that a single call to attachShadow would create a new root with the given mode only if an existing one doesn't exist, then your original example simplifies to

class CustomHTMLElement extends HTMLElement {
	#shadow = this.attachShadow({mode: 'closed', returnExistingDeclarativeShadowRoot: true})
}

Answer 44 · 2020-08-31T21:30:19.000Z

Seems you all want to prevent the following. But how?

class SomeEl extends HTMLElement {
	#internals = this.attachInternals()
}

customElements.define('some-el', SomeEl)

class ExposeInternalsextends extends SomeEl {
	attachInternals() {
		this.publicInternals = super.attachInternals()
		this.shadow = this.publicInternals.shadowRoot // open or closed root, doesn't matter.
		return this.publicInternals
	}
}

// monkey-patched define method waited to overwrite the some-el definition.
customElements.define('some-el', ExposeInternals)

EDIT: Oh, I missed the super().attachInternals() trick.

Is it placed onto the prototype only during construction, to prevent monkey patching the globals?

Answer 45 · 2020-08-31T22:59:03.000Z

After re-reading Upgrade an element a few times, I think the above suggestion sounds like it'll work. If there's general agreement here, I'm happy to put together a spec PR, and modify the Chromium implementation. Any objections? @domenic?

I'm happy to trust you if you think it will work. Ultimately my only concern is that it might not work; this seemed intractible when I originally specced this out. But if you can produce an implementation patch and tests showing that it does, that'll probably be a quicker route than us trying to reason through the algorithms in the abstract :).

Answer 46 · 2020-08-31T23:41:17.000Z

I'm happy to trust you if you think it will work. Ultimately my only concern is that it might not work; this seemed intractible when I originally specced this out. But if you can produce an implementation patch and tests showing that it does, that'll probably be a quicker route than us trying to reason through the algorithms in the abstract :).

Ok, thanks. I've started doing just that, so I'll check back here once I've had a chance. This week is busy, so possibly next week. In the meantime, if anyone sees any obvious (or not so obvious) issues, do let me know.

Answer 47 · 2020-09-08T20:13:02.000Z

Ok, thanks. I've started doing just that, so I'll check back here once I've had a chance. This week is busy, so possibly next week. In the meantime, if anyone sees any obvious (or not so obvious) issues, do let me know.

I implemented this in Chromium, and added some WPT tests. As far as I can tell, this works pretty well. The tests should include most of the troublesome code samples from this issue thread, but please let me know if I missed a corner case that should be tested.

In particular, I think the "intractability" you encountered last time was likely in trying to disallow calls to attachInternals() after the constructor runs, so that it is only callable from the constructor? This patch/change merely ensures that the constructor is the first place that it can be called.

Based on the above, I'll put together a spec PR. Sound ok?

Answer 48 · 2020-09-11T05:18:03.000Z

Ok, thanks. I've started doing just that, so I'll check back here once I've had a chance. This week is busy, so possibly next week. In the meantime, if anyone sees any obvious (or not so obvious) issues, do let me know.

I implemented this in Chromium, and added some WPT tests. As far as I can tell, this works pretty well. The tests should include most of the troublesome code samples from this issue thread, but please let me know if I missed a corner case that should be tested.

In particular, I think the "intractability" you encountered last time was likely in trying to disallow calls to attachInternals() after the constructor runs, so that it is only callable from the constructor? This patch/change merely ensures that the constructor is the first place that it can be called.

That makes sense. We've already added disableInternals address the issue of calling attachInternals after constructor so restricting it before constructor call would address the access control issue for good.

Based on the above, I'll put together a spec PR. Sound ok?

Please do.

Answer 49 · 2020-09-11T17:13:21.000Z

That makes sense. We've already added disableInternals address the issue of calling attachInternals after constructor so restricting it before constructor call would address the access control issue for good.

Based on the above, I'll put together a spec PR. Sound ok?

Please do.

Great, thanks! Will do, likely next week.

Answer 50 · 2020-09-11T22:29:04.000Z

I got time a bit sooner - here is the spec PR. Comments appreciated.

Answer 51 · 2020-09-12T03:44:40.000Z

Can we split the change for attachInternals to its own PR instead of tying it to the change to expose shadowRoot?

I also wonder if there is a way to restrict attachShadow until the constructor is called. Right now, when you call attachInternals and get the shadow root, you don't know whether it's coming from the declarative shadow root or some shadow root other scripts have attached. The combination of upgrading and backwards compatibility makes this challenging. Because of upgrades, we wouldn't know whether a given element will allow shadow root or not. If backwards compatibility weren't an issue, we could make attachShadow fail until the constructor is called.

One possibility is to automatically fail an upgrade when non-declarative shadow root had been attached before the constructor has been called when internals is enabled. Something like this:

const someElement = document.createElement('some-element');
someElement.attachShadow({mode: 'closed'});

customElements.define('some-element', class SomeElement extends HTMLElement {
    static enableInternals = true
    #internals
    constructor() {
        super();
        #internals = this.attachInternals();
        if (!#internals.shadowRoot)
            populateShadow(#internals.attachShadow());
    }
});

customElements.upgrade(someElement); // This will fail to upgrade someElement.

const otherElement = document.createElement('some-element');
otherElement.attachShadow({mode: 'closed'}); // This will throw because shadow root has already been attached.

Answer 52 · 2020-09-14T20:16:28.000Z

Can we split the change for attachInternals to its own PR instead of tying it to the change to expose shadowRoot?

Definitely - done. There are now two:

I also wonder if there is a way to restrict attachShadow until the constructor is called. Right now, when you call attachInternals and get the shadow root, you don't know whether it's coming from the declarative shadow root or some shadow root other scripts have attached. The combination of upgrading and backwards compatibility makes this challenging. Because of upgrades, we wouldn't know whether a given element will allow shadow root or not. If backwards compatibility weren't an issue, we could make attachShadow fail until the constructor is called.

One possibility is to automatically fail an upgrade when non-declarative shadow root had been attached before the constructor has been called when internals is enabled. ...

One alternative suggestion might just be to add a flag for declarative shadow roots, so that the constructor can tell the difference? E.g. shadowRoot.isDeclarative or something?

Answer 53 · 2020-09-14T21:58:51.000Z

Can we split the change for attachInternals to its own PR instead of tying it to the change to expose shadowRoot?

Definitely - done. There are now two:

Prevent attachInternals() use before custom element constructor

Add ElementInternals.shadowRoot

Thanks.

I also wonder if there is a way to restrict attachShadow until the constructor is called. Right now, when you call attachInternals and get the shadow root, you don't know whether it's coming from the declarative shadow root or some shadow root other scripts have attached. The combination of upgrading and backwards compatibility makes this challenging. Because of upgrades, we wouldn't know whether a given element will allow shadow root or not. If backwards compatibility weren't an issue, we could make attachShadow fail until the constructor is called.
One possibility is to automatically fail an upgrade when non-declarative shadow root had been attached before the constructor has been called when internals is enabled. ...

One alternative suggestion might just be to add a flag for declarative shadow roots, so that the constructor can tell the difference? E.g. shadowRoot.isDeclarative or something?

That would solve the issue that custom element author can tell whether a shadow root was attached by scripts or declarative shadow root but doesn't solve the issue that the existing scripts which call attachShadow is forced to disclose the shadow root to custom elements, which isn't great either.

Here's one more idea. ElementInternals can only expose ShadowRoot if it's created after ElementInternals itself, or a some flag is set, and declarative shadow root would set this flag by default.

Answer 54 · 2020-09-14T22:11:48.000Z

One alternative suggestion might just be to add a flag for declarative shadow roots, so that the constructor can tell the difference? E.g. shadowRoot.isDeclarative or something?

That would solve the issue that custom element author can tell whether a shadow root was attached by scripts or declarative shadow root but doesn't solve the issue that the existing scripts which call attachShadow is forced to disclose the shadow root to custom elements, which isn't great either.

Is this a common/recommended use case? A script that attaches a shadow root to a non-upgraded custom element, and a custom element that then has no control of its own shadow content? This seems problematic anyway - my guess would be that a normal custom element definition would already be throwing exceptions on attachShadow(). Perhaps I'm missing the use case here.

Here's one more idea. ElementInternals can only expose ShadowRoot if it's created after ElementInternals itself, or a some flag is set, and declarative shadow root would set this flag by default.

I'm generally ok with the idea of setting a flag, but I think it would be an ergonomic problem if CE authors had to always remember to call attachInternals() prior to calling attachShadow(), or the shadow root wouldn't be available. Maybe this isn't a huge deal, but it does seem "odd". Perhaps instead, the flag gets set if attachShadow() is called while the custom element is in the "precustomized" or "customized" state? (Or by a declarative shadow root.) This would get you what you want, I think, in that calls to attachShadow() prior to the constructor will not be exposed to ElementInternals, but without the strict ordering of calls to attachInternals() and attachShadow() within the constructor. WDYT?

Answer 55 · 2020-09-14T23:15:13.000Z

One alternative suggestion might just be to add a flag for declarative shadow roots, so that the constructor can tell the difference? E.g. shadowRoot.isDeclarative or something?

That would solve the issue that custom element author can tell whether a shadow root was attached by scripts or declarative shadow root but doesn't solve the issue that the existing scripts which call attachShadow is forced to disclose the shadow root to custom elements, which isn't great either.

Is this a common/recommended use case? A script that attaches a shadow root to a non-upgraded custom element, and a custom element that then has no control of its own shadow content? This seems problematic anyway - my guess would be that a normal custom element definition would already be throwing exceptions on attachShadow(). Perhaps I'm missing the use case here.

I'm pretty sure this isn't a common use case but it's still not a good idea to violate the previously agreed contract that attachShadow returns a ShadowRoot that's otherwise inaccessible.

Here's one more idea. ElementInternals can only expose ShadowRoot if it's created after ElementInternals itself, or a some flag is set, and declarative shadow root would set this flag by default.

I'm generally ok with the idea of setting a flag, but I think it would be an ergonomic problem if CE authors had to always remember to call attachInternals() prior to calling attachShadow(), or the shadow root wouldn't be available. Maybe this isn't a huge deal, but it does seem "odd". Perhaps instead, the flag gets set if attachShadow() is called while the custom element is in the "precustomized" or "customized" state? (Or by a declarative shadow root.) This would get you what you want, I think, in that calls to attachShadow() prior to the constructor will not be exposed to ElementInternals, but without the strict ordering of calls to attachInternals() and attachShadow() within the constructor. WDYT?

Yeah, allowing it based on custom element state would be okay as well. The issue is really about someone calling attachShadow prior to custom element being upgraded.

Answer 56 · 2020-09-14T23:32:01.000Z

Yeah, allowing it based on custom element state would be okay as well. The issue is really about someone calling attachShadow prior to custom element being upgraded.

Thanks. So it sounds like you'd be ok with this proposed solution then? If so, and assuming no objections from anyone else, perhaps I'll start prototyping this, and I'll put together another spec PR.

Answer 57 · 2020-09-15T00:10:31.000Z

Yeah, allowing it based on custom element state would be okay as well. The issue is really about someone calling attachShadow prior to custom element being upgraded.

Thanks. So it sounds like you'd be ok with this proposed solution then? If so, and assuming no objections from anyone else, perhaps I'll start prototyping this, and I'll put together another spec PR.

With that amendment, it seems like declarative Shadow DOM would be compatible with closed shadow trees so we can assume this technical issue has been resolved.

There is still a pending question of whether we support the declarative Shadow DOM at conceptual level. I need to get back to you on that.

Answer 58 · 2020-09-15T22:25:15.000Z

With that amendment, it seems like declarative Shadow DOM would be compatible with closed shadow trees so we can assume this technical issue has been resolved.

Great. I've added two more spec PRs for this change, one in HTML and one in DOM. This makes four open PRs here, which could use reviews:

There is still a pending question of whether we support the declarative Shadow DOM at conceptual level. I need to get back to you on that.

Ok, thanks for the update. I would really appreciate your thoughts soon - we are proceeding with an Origin Trial as we speak, and I'd love to ship this feature soon. I would prefer not to do that without your input and support. As I mentioned in the main thread, I believe the currently-proposed solution is worthwhile, for reasons other than pure performance.

Answer 59 · 2020-10-05T18:29:04.000Z

Thanks for the PR reviews here, I'm glad we could put together a solution to this problem.

While this is closed, I will continue to monitor the use counters I recently added for usage of the attachInternals() API prior to the constructor running, and overall attachInternals() usage at any time. The early numbers are very encouraging in terms of compat: I don't see any usage yet for the pre-constructor syntax that is now outlawed. The new spec is implemented in Chromium behind the Experimental Web Platform Features flag, but at this point I don't see any barriers to bringing it out from behind the flag.