Run a connected script when children are inserted into it - #4244

You're right on both counts, and thanks for being blunt about it — the link is wrong and I shouldn't have written it without checking the anchor resolved.

Reading the actual steps:

The script children changed steps given changedNode are:

1. If the script element is not connected, then return.
2. Run the script HTML element post-connection steps, given changedNode.

The script HTML element post-connection steps, given insertedNode, are:

1. If insertedNode is parser-inserted, then return.
2. Prepare the script element given insertedNode.

So the parser guard belongs on the changed node, not on the script being on the stack of open elements. That was me reverse-engineering something that happened to make the parser cases pass, rather than implementing what is written. Same for the removal branch — it isn't in the steps.

Before I push a corrected version I'd rather ask about the removal case than guess again. Following the steps literally, removing a child from a connected, not-yet-started script runs the post-connection steps and prepares it, but dom/nodes/insertion-removing-steps/script-does-not-run-on-child-removal.window.js asserts execution is "never triggered on child removals". whatwg/dom#1261 lists this under the things needing more discussion, citing Chromium and WebKit not executing on child modification except insertion.

Is that difference something jsdom should encode, and if so is there a shape you'd prefer, or would you rather this PR only cover what the steps say and leave that test expected-failing? Happy to go either way — I just don't want to invent another mechanism.

I'll also need to thread the changed node through _childrenChangedSteps, which currently takes no arguments, so tell me if you'd rather that were a separate change.

Bumping this one, since it's waiting on a decision rather than on me.

The steps say children changed runs the post-connection steps, and those return early only when the changed node is parser-inserted. Followed literally, removing a child from a connected script that hasn't started prepares it, and script-does-not-run-on-child-removal.window.js says it shouldn't. whatwg/dom#1261 lists that under the parts still needing discussion.

Unless you'd rather do something else, I'll implement what the steps actually say, thread the changed node through _childrenChangedSteps, and leave that one test expected-failing with a note pointing at the whatwg issue. Say the word and I'll push it.

You're right about the link, sorry. #script-children-changed-steps doesn't exist. The concept is defined in DOM as children changed steps, and HTML's version for script is in 4.12.1.1 Processing model. I should have checked that it resolved before citing it.

You're also right that the method didn't match. The actual algorithm is two steps: return if the script element is not connected, otherwise run the post-connection steps given the changed node. Nothing about the stack of open elements. Rewritten:

_childrenChangedSteps() {
  super._childrenChangedSteps();

  if (!this.isConnected) {
    return;
  }

  this._postConnectionSteps();
}

Calling _postConnectionSteps() instead of _eval() also covers the parser case on its own, since it already carries the parser-inserted guard that the spec's post-connection steps open with. So the extra state was not needed for that either.

That fixes Node-appendChild-three-scripts.html and Node-appendChild-three-scripts-from-fragment.html. It does not fix execution-timing/126.html, which was only passing because of the parts you flagged, so I have put that one back in to-run.yaml.

One thing I cannot settle from the spec, and I would rather ask than guess twice. DOM says the children changed steps take no argument and are called from insert, remove, and replace data. HTML's script version passes changedNode into the post-connection steps, whose parameter is named insertedNode, and neither one checks for removals. Read literally, removing a child from a connected script that has not already started prepares it, and it runs.

dom/nodes/insertion-removing-steps/script-does-not-run-on-child-removal.window.js asserts that must not happen, and with the version above it is the only test that fails. That is what the isRemoval flag was for in the first attempt, though I justified it badly.

Is the removal exemption implicit somewhere I am misreading, or is it a gap between the two specs? Happy to implement it whichever way you prefer.

Pushed the piece that was missing, and it turned out to be why CI was red.

script-does-not-run-on-child-removal.window.js was failing because the script element could not tell a removal from an insertion. _childrenChangedSteps() took no arguments and Node-impl called it identically from both paths, the insert at :883 and the remove at :1058, so there was nothing to branch on. My earlier description said the removal path opted out; it did not, and I should have caught that the test was still red rather than describing intent.

The hook now carries the kind of change (insert, remove, characterData), which is what lets the script element skip removals. The other two overriders, HTMLStyleElement and HTMLTextAreaElement, just forward it.

I also added the stack-of-open-elements guard that the description claimed and the code did not have, matching what HTMLStyleElement already does for the same reason: while the parser still owns the element it fills in the text itself and runs the script on the closing tag.

Passing locally now:

• script-does-not-run-on-child-removal.window.html, Node-appendChild-three-scripts.html and Node-appendChild-three-scripts-from-fragment.html: 3/3
• dom/nodes, html/semantics/scripting-1 and css/cssom WPTs: 818 passing, 0 failing
• test/api: 573 passing
• eslint clean

On the open question from before: passing the change kind is an addition to an internal hook rather than a spec concept, so if you would rather it carried the changed node, or matched however DOM ends up specifying this in whatwg/dom#1261, say which and I will rework it. I did not want to leave the PR red while that is undecided.

Thanks for the update. DOM explicitly defines children changed steps as taking no argument. HTML’s current changedNode wording is inconsistent with DOM and does not specify a mutation-kind object.

Let’s align this implementation with the proposed resolution in whatwg/dom#1460 and whatwg/html#12282: introduce separate, no-argument children inserted steps and define the script behavior there. That avoids adding a jsdom-specific { type } protocol and naturally excludes removal and character-data changes.

Since those specification PRs have not landed, please link to them in the corresponding jsdom code: the DOM PR where the generic hook is introduced/called, and the HTML PR on the script-specific override.

Also, _isOnStackOfOpenElements is never set for script elements, so that guard is currently dead. Unlike HTMLStyleElement, the script implementation has no pushed/popped methods maintaining it; please remove the guard unless there is a separately tested reason to introduce and maintain that state.

Thank you, that is a much better shape. Done in fc70fa2.

The hook. NodeImpl now has a no-argument _childrenInsertedSteps, and insert calls it instead of the children changed steps. The default implementation runs the children changed steps, which is how I read the "if those are not defined" fallback in the DOM PR, and it keeps HTMLTextAreaElement and HTMLStyleElement working on insertion without either of them having to know the new hook exists:

// https://dom.spec.whatwg.org/#concept-node-children-inserted-ext, introduced in
// https://github.com/whatwg/dom/pull/1460. Insert runs these in place of the children changed
// steps when a subclass defines them, so the default is to run the children changed steps.
_childrenInsertedSteps() {
  this._childrenChangedSteps();
}

HTMLScriptElement overrides that one instead, with the HTML PR linked on it. Removal and character data changes are now excluded by which hook runs rather than by inspecting an argument, so _childrenChangedSteps is back to taking nothing and the CharacterData, HTMLStyleElement and HTMLTextAreaElement changes from the previous push are all reverted.

The guard. You are right that it was dead, and I should have checked rather than copying the shape from HTMLStyleElement. _isOnStackOfOpenElements is assigned in exactly one file:

lib/jsdom/living/nodes/HTMLStyleElement-impl.js:12   this._isOnStackOfOpenElements = false;
lib/jsdom/living/nodes/HTMLStyleElement-impl.js:49   this._isOnStackOfOpenElements = false;
lib/jsdom/living/nodes/HTMLStyleElement-impl.js:54   this._isOnStackOfOpenElements = true;

HTMLScriptElement has _poppedOffStackOfOpenElements but no _pushedOnStackOfOpenElements and never sets the flag, so on a script it always read undefined. Removed rather than introduced, since nothing tested it.

Tests. The two enabled expectations still pass, and so does script-does-not-run-on-child-removal.window.html, which is now structural rather than a guard:

✔ insertion-removing-steps/Node-appendChild-three-scripts-from-fragment.html
✔ insertion-removing-steps/Node-appendChild-three-scripts.html
✔ insertion-removing-steps/script-does-not-run-on-child-removal.window.html

No regressions in dom/nodes (270), html/semantics/scripting-1 (221), html/semantics/forms (250), css/cssom (327), test/api (573) or test/to-port-to-wpts (191).

Rebuilt on current main in c33dc16, since the branch had gone conflicting against the memoized query cache centralisation. Force pushed rather than merged, so the history is now a single commit with the final shape rather than the two reverted detours through the { type } argument.

No change to the code you reviewed. Re-verified after the rebuild: insertion-removing-steps 20 passing including both newly enabled expectations and script-does-not-run-on-child-removal, dom/nodes 270 passing, test/api 577 passing.

Thanks, this is much better. However, the Co-authored-by footer appears to have disappeared; it is required per the project's AI policy. Can you add it back?

Restored in f4d71fd. My mistake: I squashed the branch onto current main to clear the conflict and rebuilt the message by hand, which dropped the trailer. It is back:

Co-authored-by: Claude Opus 5 <[email protected]>

Code is unchanged from what you just reviewed.