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

Add Scoped Custom Element Registries #10869

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

annevk
Copy link
Member

@annevk annevk commented Dec 17, 2024

Please do not comment directly on this PR unless asked. It's a big change and we want to keep it manageable. Use #10854 instead.

DOM PR: whatwg/dom#1341.

Tests: web-platform-tests/wpt#50790

(See WHATWG Working Mode: Changes for more details.)


/custom-elements.html ( diff )
/dom.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/parsing.html ( diff )
/scripting.html ( diff )

@annevk annevk marked this pull request as ready for review December 19, 2024 09:52
@annevk annevk added addition/proposal New features or enhancements topic: custom elements Relates to custom elements (as defined in DOM and HTML) topic: shadow Relates to shadow trees (as defined in DOM) labels Dec 19, 2024
annevk added a commit to whatwg/dom that referenced this pull request Dec 19, 2024
@annevk annevk force-pushed the annevk/scoped-custom-element-registries branch from ff4688a to 3ad2431 Compare December 19, 2024 10:07
annevk added a commit to whatwg/dom that referenced this pull request Feb 21, 2025
@annevk annevk force-pushed the annevk/scoped-custom-element-registries branch from 3ad2431 to 28bf12c Compare February 21, 2025 09:42
@annevk annevk force-pushed the annevk/scoped-custom-element-registries branch from f02797a to 98b171e Compare March 6, 2025 11:36
Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Editorial review pass up to the CustomElementRegistry IDL block.

@@ -3181,7 +3181,8 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li><dfn data-x-href="https://dom.spec.whatwg.org/#concept-node-document">node document</dfn> concept</li>
<li><dfn data-x="concept-document-type" data-x-href="https://dom.spec.whatwg.org/#concept-document-type">document type</dfn> concept</li>
<li><dfn data-x="concept-DocumentFragment-host" data-x-href="https://dom.spec.whatwg.org/#concept-documentfragment-host">host</dfn> concept</li>
<li>The <dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-root">shadow root</dfn> concept, and its <dfn data-x-href="https://dom.spec.whatwg.org/#shadowroot-delegates-focus">delegates focus</dfn>, <dfn data-x-href="https://dom.spec.whatwg.org/#shadowroot-available-to-element-internals">available to element internals</dfn>, <dfn data-x-href="https://dom.spec.whatwg.org/#shadowroot-clonable">clonable</dfn>, and <dfn data-x="shadow-serializable" data-x-href="https://dom.spec.whatwg.org/#shadowroot-serializable">serializable</dfn>.</li>
<li>The <dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-root">shadow root</dfn> concept, and its <dfn data-x-href="https://dom.spec.whatwg.org/#shadowroot-delegates-focus">delegates focus</dfn>, <dfn data-x-href="https://dom.spec.whatwg.org/#shadowroot-available-to-element-internals">available to element internals</dfn>, <dfn data-x-href="https://dom.spec.whatwg.org/#shadowroot-clonable">clonable</dfn>, <dfn data-x="shadow-serializable" data-x-href="https://dom.spec.whatwg.org/#shadowroot-serializable">serializable</dfn>, <dfn data-x="shadow-root-custom-element-registry" data-x-href="https://whatpr.org/dom/1341.html#shadowroot-custom-element-registry">custom element registry</dfn>, and <dfn data-x-href="https://whatpr.org/dom/1341.html#shadowroot-keep-custom-element-registry-null">keep custom element registry null</dfn>.</li>
Copy link
Member

Choose a reason for hiding this comment

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

Comment to ensure that the URLs here change before this is merged.

<dfn data-x="concept-node-clone-parent" data-x-href="https://dom.spec.whatwg.org/#clone-a-node-parent"><var>parent</var></dfn>, and the concept of
<dfn data-x="concept-node-clone-subtree" data-x-href="https://dom.spec.whatwg.org/#clone-a-node-subtree"><var>subtree</var></dfn>,
<dfn data-x="concept-node-clone-parent" data-x-href="https://dom.spec.whatwg.org/#clone-a-node-parent"><var>parent</var></dfn>, and
<dfn data-x="concept-node-clone-fallbackRegistry" data-x-href="https://dom.spec.whatwg.org/#clone-a-node-fallbackregistry"><var>fallbackRegistry</var></dfn>, and the concept of
Copy link
Member

Choose a reason for hiding this comment

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

Need to double check that this URL is correct before landing.

<span>active custom element constructor map</span>[<span>NewTarget</span>].</p></li>
</ol>

<li><p>If <var>registry</var> is null, then set <var>registry</var> to <span>current global
Copy link
Member

Choose a reason for hiding this comment

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

Is this equivalent?

Suggested change
<li><p>If <var>registry</var> is null, then set <var>registry</var> to <span>current global
<li><p>Otherwise, set <var>registry</var> to <span>current global

I think it is, because it seems registry can't still be null if the previous branch was taken. The way it's currently written makes me as a reader question that assumption :)

@@ -64717,6 +64738,10 @@ interface <dfn interface>HTMLTemplateElement</dfn> : <span>HTMLElement</span> {
data-x="attr-template-shadowrootserializable">shadowrootserializable</code></dfn> content
attribute is a <span>boolean attribute</span>.</p>

<p>The <dfn element-attr for="template"><code
data-x="attr-template-shadowrootcustomelementregistry">shadowrootcustomelementregistry</code></dfn>
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this the longest attribute name so far, 31 characters. Maybe that's right, but have you thought about shorter names that you also think are OK?

data-x="attr-template-shadowrootcustomelementregistry">shadowrootcustomelementregistry</code>
content attribute.</p>

<p class="note">The IDL attribute does intentionally not have a boolean type so it can be
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, what's the possible extension point here? To give a name to registries or something?

Comment on lines +73428 to +73429
<p>To <dfn export>look up a custom element definition</dfn>, given null or a
<code>CustomElementRegistry</code> object <var>registry</var>, string-or-null
Copy link
Member

Choose a reason for hiding this comment

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

This style seems more common for nullable objects:

Suggested change
<p>To <dfn export>look up a custom element definition</dfn>, given null or a
<code>CustomElementRegistry</code> object <var>registry</var>, string-or-null
<p>To <dfn export>look up a custom element definition</dfn>, given a
<code>CustomElementRegistry</code>-or-null <var>registry</var>, string-or-null

(I found NavigationFocusReset-or-null as the example follow.)

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Editorial review done.

DOMString? <span data-x="dom-CustomElementRegistry-getName">getName</span>(CustomElementConstructor constructor);
<span data-x="idl-Promise">Promise</span>&lt;<span>CustomElementConstructor</span>&gt; <span data-x="dom-CustomElementRegistry-whenDefined">whenDefined</span>(DOMString name);
[<span>CEReactions</span>] undefined <span data-x="dom-CustomElementRegistry-upgrade">upgrade</span>(<span>Node</span> root);
<span data-x="dom-CustomElementRegistry">constructor</span>();
Copy link
Member

Choose a reason for hiding this comment

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

For other reviewers: It's hard to tell from the diff, so the new things here are the constructor and the initialize() method.


<ol>
<li><p>If <var>node</var> is an <code>Element</code> object, then return
<var>parentNode</var>'s <span data-x="element-custom-element-registry">custom element
Copy link
Member

Choose a reason for hiding this comment

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

There is no parentNode argument. Judging by the DOM PR, this should just be the element's custom element registry?

Suggested change
<var>parentNode</var>'s <span data-x="element-custom-element-registry">custom element
<var>node</var>'s <span data-x="element-custom-element-registry">custom element

<li><p>If <var>node</var> is a <code>ShadowRoot</code> object, then return <var>node</var>'s
<span data-x="shadow-root-custom-element-registry">custom element registry</span>.

<li><p>If <var>node</var>'s <span data-x="concept-document-bc">browsing context</span> is null,
Copy link
Member

Choose a reason for hiding this comment

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

Can this check the relevant global object instead, so that a reader doesn't need to think about whether a node's browsing context and relevant global object are always null/non-null at the same time. Also, implementations will probably just get the relevant global object and check if it's null, so just as well to be close to that.

<dd><span data-x="concept-try-upgrade">Tries to upgrade</span> all <span>shadow-including
inclusive descendant</span> elements of <var>root</var>, even if they are not
<span>connected</span>.</dd>

<dt><code data-x=""><var>registry</var>.<span subdfn data-x="dom-CustomElementRegistry-initialize">initialize</span>(<var>root</var>)</code></dt>
<dd>Each <span>inclusive descendant</span> of <var>root</var> with a null registry will have it
Copy link
Member

Choose a reason for hiding this comment

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

The "with a null registry" is accurate, but see question below and let's make sure this matches if anything changes.


<ol>
<li><p>If <var>root</var> is a <code>ShadowRoot</code> node whose <span
data-x="shadow-root-custom-element-registry">custom element registry</span> is null, then set
Copy link
Member

Choose a reason for hiding this comment

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

Is the "is null" condition here and below important? Is there an invariant that a node's custom element registry never changes after it's set to a non-null value, and why? A note or example about this might help.

If that's not an invariant and if there's a use case for changing the custom element registry, then maybe an option for how to handle non-null cases is needed?

(Apologies if this is not a good question, I'm reviewing this before really understanding how everything fits together.)

<p><span data-x="map remove">Remove</span> <var>registry</var>'s <span>relevant global
object</span>'s <span>active custom element constructor map</span>[<var>C</var>].</p>

<p class="note">This is a no-op if <var>C</var> immediately calls <code
Copy link
Member

Choose a reason for hiding this comment

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

Is the case where super() isn't called covered by the tests?

<li><p>If <var>current node</var>'s <span data-x="element-custom-element-registry">custom
element registry</span> is not <var>shadow</var>'s <span
data-x="shadow-root-custom-element-registry">custom element registry</span>, then append
"<code data-x=""> shadowrootcustomelementregistry=&quot;&quot;</code>".</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

How does this fit together with the extension point, the reason that the IDL attribute reflects as a string and not a boolean? Shouldn't this preserve the attribute value for consistency / to avoid a change in behavior later?

@foolip
Copy link
Member

foolip commented Mar 19, 2025

OK, so the main thing I'm left wondering point is the reasoning behind shadowrootcustomelementregistry being a boolean attribute, but the shadowRootCustomElementRegistry IDL attribute reflecting as a string, per the comment in the spec.

If the value is the empty string, shadowRootCustomElementRegistry will be the empty string (falsy), but if it was reflected as a boolean it would be true. If the attribute isn't present, shadowRootCustomElementRegistry would also be the empty string, so how does one distinguish between the typical "attribute is present" and "attribute is absent" cases?

Maybe it makes sense if I learn more about how this might be extended in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: custom elements Relates to custom elements (as defined in DOM and HTML) topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging this pull request may close these issues.

Revamped Scoped Custom Element Registries (DOM side) Revamped Scoped Custom Element Registries
2 participants