Allow multiple top-level nodes when rendering StreamField blocks

[laymonage]

But don't change what's passed to the BoundWidget class (which means still just the first element), so this shouldn't affect any of the BoundWidgets.

Fixes #11935. Will add tests if this is indeed what we want... Test added.

[squash-labs]

Manage this branch in Squash

Test this branch here: https://laymonageboundwidget-multi-top-znj81.squash.io

[laymonage]

With this, we replace the placeholder with all the nodes in the HTML (that was put in the div tempContainer). However, we keep only passing the tempContainer.firstChild as the element to the bound widget class (see that the above const dom = tempContainer.firstChild; and the below return value do not change).

We change the runInlineScripts call from the dom (first child) to the tempContainer in case there are scripts at the top-level (as the case with #11935).

Then, we need to move the runInlineScripts call to before the placeholder is swapped, because once it is, tempContainer will be empty. We can't do runInlineScripts(dom.parentElement) since we can't be sure that the parent does not have other scripts already in place.

I suppose one possible gotcha is if the script relies on the fact that it used to run after the widget was put inside the DOM.

A bit related, in TeleportController, we run script before the template element is put inside the DOM:

wagtail/client/src/controllers/TeleportController.ts

Line 50 in 4f5ffa8

target.append(...this.templateFragment.childNodes);

wagtail/client/src/controllers/TeleportController.ts

Lines 94 to 108 in 4f5ffa8

	get templateFragment() {
	const content = this.element.content;
	// https://developer.mozilla.org/en-US/docs/Web/API/Node/cloneNode (returns the same type)
	// https://github.com/microsoft/TypeScript/issues/283 (TypeScript will return as Node, incorrectly)
	const templateFragment = content.cloneNode(true) as typeof content;
	// HACK:
	// cloneNode doesn't run scripts, so we need to create new script elements
	// and copy the attributes and innerHTML over. This is necessary when we're
	// teleporting a template that contains legacy init code, e.g. initDateChooser.
	// Only do this for inline scripts, as that's what we're expecting.
	runInlineScripts(templateFragment);
	return templateFragment;
	}

(It's inside a getter for templateFragment which is called before the .append() call.)

Alternatively I guess we could create an array that holds any script elements in tempContainer.childNodes, then pass that to runInlineScripts, but that means the util needs to be updated to accept an array of script elements.

[gasman]

Thanks for investigating, @laymonage!

If we're not going to take a hard line on insisting that the widget HTML must be a single element, then I think we ought to make it as accommodating as possible, rather than an in-between state where we're making unwritten assumptions such as the first element being the correct one to pass to BoundWidget. (For example, this would be broken again just by adding a <!-- comment --> at the top - I remember thinking that I was possibly sweeping this under the carpet a bit when adding the trim() so that we at least didn't have to worry about stray whitespace...)

Given that the only thing BoundWidget does with the passed-in element is select the input element from it, I think we could reasonably alter the signature of BoundWidget so that it accepts either a single element or an array / NodeList. (To minimise the fall-out for any third-party BoundWidget subclasses that have already been updated for the 6.1 signature and specifically expect a single element, we can make sure that we only pass a list if multiple elements are found.)

I'm also sceptical about whether it's safe to invoke runInlineScripts while the elements are still in tempContainer, especially since they're not part of the document tree at that point. I think that'll be an easy fix once we've made the above change to pass the collection of elements around as an array / NodeList, though.

Will pick these changes up now.

[gasman]

@laymonage Updated version now pushed as b310b11.

[laymonage]

Thanks @gasman! Your changes work well, tested on Safari 17.5, Chrome 125, and Firefox 125.

Just one small comment, but otherwise it looks good to me! (Can't approve as I'm the PR author 😄)

[laymonage]

Since we've extracted the selector to a variable...

Suggested change

	const scripts = element.querySelectorAll(
	'script:not([src])',
	) as NodeListOf<HTMLScriptElement>;
	const scripts = element.querySelectorAll<HTMLScriptElement>(selector);

[gasman]

Oops, yes, that was indeed what I was aiming for :-) Thanks!

[gasman]

Merged in c28a260 (main) / 8499f69 (stable/6.1.x).