fix(runtime): prevent additional attempted move of slot content

[tanner-reits]

What is the current behavior?

Slotted content may incorrectly get the hidden attribute applied when slotted through multiple levels of components. This is especially noticeable when the final destination does not wrap the slot in an additional element (like a span or div) since we could not correctly resolve an "anchor" for the node to be relocated.

Essentially, our slot relocation algorithm would identify that the node is slotted content that should live in a different location than it's original "host" element. This works fine until you try slotting content through many Stencil components. In that case, we identify the node as needing relocation, but eventually find ourselves in a situation where we think the node needs to move, but can't resolve where to move it to (because it's already there), so we think it doesn't have a home and hide it. That's why you could inspect the element in dev tools and see it's where it should be, but visually hidden.

Fixes: #4523

What is the new behavior?

We now track a "slot host" element tag to check against when performing slot relocation to help prevent a case where a node may be marked as needing relocation when it is already in it's correct final location.

Does this introduce a breaking change?

• Yes
• No

Testing

Automated tests

All existing unit and e2e tests continue to pass. Added a new e2e test that is a mirror image of the reproduction case in the original issue.

Manual testing

Installed this branch in the reproduction case and enabled experimentalSlotFixes. The app correctly renders the slotted content and the DOM tree matches what is expected.

Other information

Only available with the experimentalSlotFixes config option enabled.

[github-actions]

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1399 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/mock-doc/serialize-node.ts	36
src/dev-server/server-process.ts	32
src/compiler/build/build-stats.ts	27
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts	25
src/compiler/style/test/optimize-css.spec.ts	23
src/testing/puppeteer/puppeteer-element.ts	23
src/compiler/prerender/prerender-main.ts	22
src/runtime/vdom/vdom-render.ts	20
src/runtime/client-hydrate.ts	19
src/screenshot/connector-base.ts	19
src/compiler/config/test/validate-paths.spec.ts	16
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/compiler/transpile/transpile-module.ts	14
src/runtime/vdom/vdom-annotations.ts	14
src/sys/node/node-sys.ts	14
src/compiler/build/build-finish.ts	13
src/compiler/prerender/prerender-queue.ts	13

Our most common errors

Typescript Error Code	Count
TS2345	424
TS2322	398
TS18048	310
TS18047	100
TS2722	38
TS2532	34
TS2531	23
TS2454	14
TS2352	13
TS2769	10
TS2790	10
TS2538	8
TS2344	5
TS2416	4
TS2493	3
TS18046	2
TS2684	1
TS2488	1
TS2430	1

Unused exports report

There are 12 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports

File	Line	Identifier
src/runtime/bootstrap-lazy.ts	21	setNonce
src/screenshot/screenshot-fs.ts	18	readScreenshotData
src/testing/testing-utils.ts	198	withSilentWarn
src/utils/index.ts	145	CUSTOM
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	115	Env
src/compiler/app-core/app-data.ts	117	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	62	satisfies
src/compiler/types/validate-primary-package-output-target.ts	62	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

[tanner-reits]

Looks like a bigger PR than it really is. Most of the file changes are renaming a build flag and adding a test

[tanner-reits]

Just decided to swap out this build flag with a more generic one to put all our vDOM/slot changes behind rather than adding new flags for each fix. All this happens in 3d7029a

[tanner-reits]

I'll probably do a bit more manual testing on this before we're ready to merge, but wanted to at least get this up before end of sprint

[alicewriteswrongs]

just had one question after reading

[alicewriteswrongs]

what's the reason for checking !BUILD.experimentalSlotFixes here?

[tanner-reits]

@alicewriteswrongs Because we only want the rest of that check to happen if experimentalSlotFixes is enabled. So, if it isn't, we'll short-circuit and enter the code block before evaluating the new s-sh element property.

[alicewriteswrongs]

ohhh ok that makes sense - so the code block is always relevant (slot fixes or not) but that s-sh business is gated behind experimentalSlotFixes - makes sense!