Fix domFor hitting null dom elements in while loop

[thequailman]

Description

The domFor while loop assumes that dom will always be not null, but it seems like some environments (like happy-dom) may incorrectly report the domSize when the underlying siblings contain nulls.

Motivation and Context

I believe this fixes #2990, at least based on my testing.

Also, looking at the diff between 2.2.2 and 2.2.3 (when I think this behavior was introduced), the loop here that domFor replaced seems to check the child nodes for nulls:

v2.2.2...v2.2.3#diff-f50a6d08fa58d22d2e71df97ae8fc5727ea53bc49c69f457ef12bc6df3c6870bL583

I think this behavior was missed in the domFor implementation.

How Has This Been Tested?

I previously had a bunch of breaking components with Mithril 2.2.3+ and happy-dom. Upon investigation, it was happening with conditional component removals via ternaries. This change fixes all of my tests.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• I have added tests to cover my changes.
  • I'm not sure how to properly introduce null dom vnodes.
• All new and existing tests passed.
• My change requires a documentation update, and I've opened a pull request to update it already:
• I have read https://mithril.js.org/contributing.html.

[dead-claudia]

Could you confirm how domSize is getting misreported? Last thing I want is happy-dom breaking in some other subtle way and causing more problems.

Keep in mind, domSize is public API. It's not an implementation detail. And this is not the only part of the code base making assumptions of its consistency.

[dead-claudia]

See comment

[thequailman]

I can try but it's hard to trace this. It seems to only happen when deeply nested DOM elements are removed during m.mount. is it possible that a nextSibling could become null during the loop?

I agree that this is likely a happy dom bug, but idk what I'm looking for on their end. I haven't been able to create reproducible version of this in a single JS file.

[dead-claudia]

Edit: improve examples

I haven't been able to create reproducible version of this in a single JS file.

@thequailman Even a many-file repro is fine, and I was hoping for at least a repo to pull. I've even dealt with a few bug reports here where people outright linked websites because they weren't sure where to start.

For minimizing repros, I approach it algorithmically and systematically. Remove a bit if code, test, keep if bug continues to repro, and repeat with something else until nothing else can be removed. Rarely, I find myself copying dependency code during this process, but it does sometimes help, especially if it's not framework code.

I've gotten some very large minimal repros out of that process, but they were in fact minimal or close to it. It also helped me narrow down this bug report to a very subtle spec detail that only the browser we were buggy in was correctly implementing. And the tests for this PR are all fairly subtle and involved. But yeah, best way to find and minimize repros in my experience is to be as scientific as possible about it.

[thequailman]

Was able to reproduce it, it appears to be something with the select element, very bizarre. Using the latest version of mithril and @happy-dom/global-registrator:

import m from "mithril";

import { GlobalRegistrator } from "@happy-dom/global-registrator";
GlobalRegistrator.register();

function component() {
  return {
    view: () => {
      return [m("select"), m("div")];
    },
  };
}

m.mount(document.body, component);

m.mount(document.body, null);

This should produce an error: TypeError: Cannot read properties of null (reading 'nextSibling')

You can "fix" it by changing the tag from select to another div, the error doesn't reappear.

[thequailman]

Closing, this is related to a happy-dom bug: capricorn86/happy-dom#1532