[scoped-registry] Null registry element should get registry on adopt instead of insert [chromium/src : main]

0 views

Skip to first unread message

Jayson Chen (Gerrit)

unread,

Jan 9, 2026, 5:07:04 PM (2 days ago) Jan 9

to AyeAye, Mason Freed, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Jayson Chen, Joey Arhar and Mason Freed

Message from Jayson Chen

Set Ready For Review

Open in Gerrit

Related details

Attention is currently required from:

Jayson Chen
Joey Arhar
Mason Freed

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jayson Chen (Gerrit)

unread,

Jan 9, 2026, 5:13:07 PM (2 days ago) Jan 9

to AyeAye, Mason Freed, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Joey Arhar and Mason Freed

Jayson Chen added 6 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Jayson Chen . resolved

Updated the CL to leverage RemovedFrom and InsertedInto so the implementation is cleaner.

Commit Message

Line 9, Patchset 1:This fixes the case where the custom element registry gets changed during

Mason Freed . resolved

uber-nit: please line wrap commit descriptions at 72 chars. This is 73. 😊

Jayson Chen

Done

File third_party/blink/renderer/core/dom/node.cc

Line 975, Patchset 1:

  if (RuntimeEnabledFeatures::ScopedCustomElementRegistryEnabled()) {
    for (Node& descendant_node : NodeTraversal::InclusiveDescendantsOf(*new_child)) {

Mason Freed . resolved

It feels really weird to be putting this only here, in Node::appendChild. Shouldn't it be down in something like ContainerNode::AppendChild or ContainerNode ::InsertNodeVector or somewhere shared with the rest? I.e. shouldn't the same thing happen for insertBefore(), or replaceChildren()?

Jayson Chen

Not applicable as the impl changed.

Line 978, Patchset 1:

        // Note that SustainCustomElementRegistry does not sustain null registry.
        // In the event of needing to sustain null registry, we need to add
        // additional logic to handle that case.

Mason Freed . resolved

Is this a problem?

Jayson Chen

Not applicable as the impl changed.

Line 1108, Patchset 1:

  // registry before the move as an element's registry can only be set once.
  if (RuntimeEnabledFeatures::ScopedCustomElementRegistryEnabled()) {
    for (Node* target_node : nodes) {
      if (auto* target_element = DynamicTo<Element>(target_node)) {
        target_element->SustainCustomElementRegistry();
      }
    }
  }

Mason Freed . resolved

ditto - this is a duplicate, likely because it belongs lower, right?

Jayson Chen

Not applicable as the impl changed.

File third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/element-mutation-expected.txt

File-level comment, Patchset 1:

Mason Freed . unresolved

Ideally this test tests the various ways nodes can be moved around the tree also. E.g.
appendChild
insertBefore
append
prepend
before
after
replaceWith
moveBefore
Ordinarily I wouldn't suggest that everywhere, but I believe it might catch the bug above?

Jayson Chen

Now that we're not implementing in a way that's specific to append but mutation in general, should we still test all the mutation methods?

Open in Gerrit

Related details

Attention is currently required from:

Joey Arhar
Mason Freed

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages