Update element adoption logic for scoped registry

[ja-y-son]

Fixing #1429 to make sure element is getting the correct scoped custom element registry upon adoption.

• At least two implementers are interested (and none opposed):
  • Webkit
  • Chromium
• Tests are written and can be reviewed and commented upon at:
  • Add test cases for null registry element upon adoption web-platform-tests/wpt#56423
• Implementation bugs are filed:
  • Chromium: https://issues.chromium.org/issues/399124619
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1874414
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=302970
  • Deno (only for aborting and events): …
  • Node.js (only for aborting and events): …
• MDN issue is filed: N/A
• The top of this comment includes a clear commit message to use.

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

---

Preview | Diff

[annevk]

Why is this not using the "effective global custom element registry" strategy discussed? It's not clear to me this is the same or why we need to be looking at document at all.

[ja-y-son]

Updated to use "effective global custom element registry" strategy as discussed. Was trying to see if I could get the point across with another approach but it seems like "effective global custom element registry" is still the better way to go. Lmk if I'm missing anything on this idea.

[annevk]

This looks good to me. We'll need tests next.

For the tests I'd like us to exercise some of the more complicated scenarios with some nodes having been assigned registries and others not, etc.

[ja-y-son]

Just added some tests but I haven't fully verified them yet. Feel free to take a look.

[ja-y-son]

@annevk Want to verify that this is the expected behavior from this change on effective gloabl registry. Consider this test case from adoption.window.js

test(t => {
  const contentDocument = document.body.appendChild(document.createElement('iframe')).contentDocument;
  t.add_cleanup(() => contentDocument.defaultView.frameElement.remove());
  const element = document.createElement('div');
  assert_equals(element.customElementRegistry, customElements);

  const scoped = new CustomElementRegistry();
  const scopedElement = contentDocument.createElement('div', { customElementRegistry: scoped });
  contentDocument.body.appendChild(scopedElement);
  assert_equals(scopedElement.customElementRegistry, scoped);
  scopedElement.appendChild(element);
  assert_equals(element.customElementRegistry, contentDocument.customElementRegistry);
}, "Adoption with global registry into a scoped registry");

Given that now we look at the parent's registry's effective global registry, the last assert of the test should probably be

assert_equals(element.customElementRegistry, null);

given that the parent is using a scoped registry.
Is this expected? Or we want it to keep getting global registry for this element? It feels a bit odd that it originally has a global registry but after adoption it goes to null registry. Perhaps we should make sure that non-null registry element shouldn't get null registry after adoption (unless there are no global registry available)?

[annevk]

Yeah, I suppose distinguishing those cases would be better. I doubt anyone is going to run into this in practice, but we might as well make it nice.

[annevk]

I don't understand this. If you mean for this to only apply in the Otherwise branch you need to nest the first bit of the Otherwise branch as well.

[ja-y-son]

I think I made it unnecessarily complicated in the previous iteration. Given that our main goal is that global registry element shouldn't become null registry during adoption, I think we can simply include this category (global registry) to use document's registry's effective global registry, so it wouldn't be getting null from parent. Does this make sense? Or perhaps I'm missing something here.

[annevk]

Yeah I think this works. I'll re-review on Monday to be sure. If @sorvell, @rniwa, and maybe @keithamus could have a look as well that'd be great.

(Checking inclusiveDescendant's custom element registry again bothers me a little bit, but I don't see a good way out of that. I also want to double check the "DocumentFragment but not ShadowRoot" language.)

[annevk]

Suggested change

	<li><p>If <var>inclusiveDescendant</var>'s <a for=Element>custom element registry</a>
	is non-null or <var>inclusiveDescendant</var>'s <a for=tree>parent</a> is null or
	<var>inclusiveDescendant</var>'s <a for=tree>parent</a> is a {{DocumentFragment}} but not
	a {{ShadowRoot}}, then set <var>registry</var> to <var>document</var>'s
	<a for=Document>custom element registry</a>.
	<li><p>If <var>inclusiveDescendant</var>'s <a for=Element>custom element registry</a> is
	non-null, <var>inclusiveDescendant</var>'s <a for=tree>parent</a> is null, or
	<var>inclusiveDescendant</var>'s <a for=tree>parent</a> is a {{DocumentFragment}} but not
	a {{ShadowRoot}}, then set <var>registry</var> to <var>document</var>'s
	<a for=Document>custom element registry</a>.