Fix HTMLView.insertBefore to handle reference nodes without a parentNode

[vivinkrishna-ni]

Pull Request

📖 Description

Fix HTMLView.insertBefore to guard against a null parentNode on the reference node when the fragment has child nodes. Previously, this would throw a TypeError: Cannot read properties of null (reading 'insertBefore'). Now it safely returns early, guarding both the if and else branches from being crashed.

This is a fix.

🎫 Issues

Relevant Nimble issue: ni/nimble#2744

👩‍💻 Reviewer Notes

A Firefox-specific race condition occurs when the binding engine processes deferred DOM updates. When a view is removed from the DOM (e.g., via a when binding transitioning to falsy), the resulting DOM mutations trigger event handlers that queue new binding updates.

In Firefox, these new updates can be processed within the same batch, leading to a scenario where insertBefore is called with a reference node whose parent was already removed earlier in the same update cycle.

Chrome processes these updates in a different order that avoids this issue.

The fix adds a null check for node.parentNode before calling node.parentNode.insertBefore() in the fragment-has-children path of HTMLView.insertBefore. The non-null assertion (!) was removed and replaced with an early return guard.

📑 Test Plan

• Verified manually that the fix prevents the crash when insertBefore is called with a reference node that has no parent. Built and verified using the Nimble storybook.
• Added comprehensive unit tests in view.spec.ts covering HTMLView lifecycle, including the specific null-parent scenarios.

✅ Checklist

General

• I have included a change request file using $ npm run change
• I have added tests for my changes.
• I have tested my changes.
• I have updated the project documentation to reflect my changes.
• I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

• I have added a new component
• I have modified an existing component
• I have updated the definition file
• I have updated the configuration file

⏭ Next Steps

[rajsite]

Now it safely returns early, matching the defensive pattern already used in the else branch.

I'm not following, the else branch is not using a defensive pattern for accessing the parentNode. It's using a non-null assertion for accessing the parentNode. It seems like if parentNode was null it would also try to access parentNode.insertBefore.

Why are we confident both branches do not need to be protected?

Do we know the scenario where we hit this case? Can we create a test?

[vivinkrishna-ni]

My bad, the PR description didn't clearly convey that both branches are affected. I've updated the fix to a single guard at the top of insertBefore to address in both branches.

Do we know the scenario where we hit this case?

---

AI GENERATED:

The crash originates from a when directive used in a template. when() (when.ts) returns a binding that evaluates to either a SyntheticViewTemplate (truthy) or null (falsy). This binding result is consumed by updateContentTarget, which manages the view lifecycle for the Comment marker node (this.target) that the when directive is anchored to.

When the when condition transitions from truthy to falsy, updateContentTarget calls view.remove(), which removes the view's DOM nodes (including <slot> elements) from the shadow DOM.

Here's where the race condition occurs (I suspect):

• The process() runs queued tasks in a while loop inside a single requestAnimationFrame callback. Tasks added during an iteration execute in the same batch.
• One task runs updateContentTarget for a when binding that went falsy → calls view.remove() → removes the view's <slot> from the shadow DOM.
• Removing the <slot> triggers a slotchange event. As per the DOM spec, slotchange is dispatched as a microtask.
• Firefox behavior: The microtask checkpoint runs between iterations of the while loop. The slotchange handler fires → observables notify → queueUpdate() appends new tasks to the same tasks array → these are picked up by the while loop immediately.
  • One of these interleaved tasks calls updateContentTarget for a different (or the same) when binding that is now truthy → calls view.insertBefore(this.target) where this.target is the Comment marker node whose parent element was already removed by the other task earlier in the batch. node.parentNode becomes null.
• Chrome behavior: The same slotchange microtask fires, but Chrome processes the resulting binding updates in a coherent order (either doesn't get interleaved into the same batch, or the DOM state is resolved differently such that the parent is still attached) where the marker's parent is still attached.

---

However, I have not yet identified a definitive fix for this behavior, and I have not been able to determine whether it is a Firefox bug or something we need to handle in our code. For now, to prevent this error from being thrown and to avoid application crashes, I would like to add a defensive null check. This should be a safe, non-destructive mitigation while we continue investigating the root cause, and if confirmed, we may create a bug for Firefox.

Can we create a test?

Created a new test file view.spec.ts covering all functionality in view.ts, including the new parentNode null check. Would it be better to add the test for all view functions in the same PR, or should we track that separately?

Please let me know if this makes sense. The guard is purely defensive against a race condition in how each browser handles microtask scheduling.

[rajsite]

You added so many tests and this description is so long, can you please distill what you are actually confident about vs what is AI generated? How thoroughly did you actually review all this test code before sharing?

[vivinkrishna-ni]

can you please distill what you are actually confident about vs what is AI generated?

I am confident in the workaround that safely checks parentNode to prevent the console error. I also investigated potential browser differences in microtask scheduling around slotchange events. However, I have not yet confirmed this with a reduced repro (due to time constraints), so I am not certain about the reasoning that AI provided.

I have separated out the above message for clear differentiation of the AI-generated part. Although that content was AI-generated, I reviewed it to ensure it was theoretically correct. Apologies for sharing a large amount of content earlier without clearly indicating what was AI-generated.

The test cases (also AI-generated) were reviewed and verified before I shared them. If I missed anything on the tests, please let me know.

[rajsite]

The microtask checkpoint runs between iterations of the while loop.

If you actually believe this is happening then this is a bug. A micro task cannot interleave the execution of a synchronous while loop. That's a pretty big claim though.

[rajsite]

This should be a safe, non-destructive mitigation
...
The guard is purely defensive against a race condition in how each browser handles microtask scheduling.

Is the guard purely a defensive measure? The code author is expecting nodes to move from one spot to another. The guard has cases where the nodes don't move. Why are you confident it is safe for them to not move?

[rajsite]

Looking at the changes so far I'd say the following:

• Seems unlikely that the guard will cause any regressions in existing browsers today that are working well. The code is already written with an assumption that parentNode is not null.
• I'm not confident the guard is safe in browsers we hit errors in today. I haven't seen a convincing discussion that under all usages of this code that when parentNode is null it is safe to effectively skip the call to insertBefore. Maybe skipping the call puts the app in a bad state, do you have a clear reason it won't?
  • Based on the known issue discussed in this PR it seems like there is evidence that the app is placed in an unexpected state. In this case "no items found" no longer renders. Hard to know for sure if that's because insertBefore is skipped because of the guard but seems possibly related: Fix Firefox crash in anchored-region by replacing when directive with CSS-based slot visibility nimble#2923
• The guard silently skips the behavior of insertBefore. If other browser behavior changes and this path would be hit in other browsers it's silently ignored and like above it's not clear to me that is actually safe to ignore.

If you have a convincing reason the change is safe for all scenarios where parentNode is null, please discuss.

If not, lets do the following:

• Have some kind of console.log that's printed when parentNode is null. I'm guessing the code path is potentially hit frequently so consider a count limit on the log (i.e. only print the first 5 times or something). If it's not hit excessively then we can consider doing a log without a count limit.
  • That way if there are other weird behaviors in firefox today there is some indication to a debugger of the unexpected code path being hit
  • That way future changes in browser behavior won't be silently ignored
• Reduce the test cases being added to just what is relevant to these changes, i.e. exercise that the log and count limit behave well.

[rajsite]

@vivinkrishna-ni Molly and I spent some time debugging the behavior this afternoon and we think we narrowed down the problem to the disposeContiguousBatch function:

fast/packages/web-components/fast-element/src/templating/view.ts

Lines 257 to 275 in b6d033d

	public static disposeContiguousBatch(views: SyntheticView[]): void {
	if (views.length === 0) {
	return;
	}
	range.setStartBefore(views[0].firstChild);
	range.setEndAfter(views[views.length - 1].lastChild);
	range.deleteContents();
	for (let i = 0, ii = views.length; i < ii; ++i) {
	const view = views[i] as any;
	const behaviors = view.behaviors as Behavior[];
	const oldSource = view.source;
	for (let j = 0, jj = behaviors.length; j < jj; ++j) {
	behaviors[j].unbind(oldSource);
	}
	}
	}

What seems to be happening is that in Firefox when a repeat directive is used to populate a slot, that on successive runs of the repeat directive the range api usage for disposeContiguousBatch ends up deleting nodes outside of the slot.

Some pseudocode that illustrates light dom populated by a repeat directive and an element in the shadow root that gets deleted:

<my-el>
    repeat(5, html`<my-item></my-item>`)
    #shadow-root
        <slot></slot>
        <div>some element next to the slot, like the no items div, ends up getting deleted! Crazy 🤯</div>
</my-el>

You can reproduce it by putting an element like <div>test</div> next to <slot name="option"> and see that the element disappears from the DOM after the @ mention is first used:

https://github.com/ni/nimble/blob/629480505b98404f37f1c3542c2acdfb8c4a7af6/packages/nimble-components/src/rich-text/mention-listbox/template.ts#L41

That seems like a bug in Firefox and we have some ideas to try and address it. @mollykreis is going to take a look into it further on Monday so we can pause the approach shown in this PR for now. I'll move the PR to draft while we investigate a bit closer.