fix(runtime): relocate slotted content when slot parent element tag changes #5120

tanner-reits · 2023-12-01T19:17:24Z

What is the current behavior?

Stencil can re-render incorrectly when content is slotted in from the "implementation layer" (i.e. from index.html) and a slots parent element tag changes (e.g. from p to span). This is only an issue when using slots outside the Shadow DOM.

The issue is caused by our vDOM algorithm relocating slotted content back to its original location when it determines that a child node has changed. When this happens, we call putBackInOriginalLocation() which will relocate a node back to where it was original defined. But, this can result in the node no being re-relocated if it was put back into a location inside another Stencil element that has already gone through its render cycle. In this case, the node will stay in its original location rather than being relocated through the DOM elements again.

Fixes: #4284 #3913

What is the new behavior?

Instead of using putBackInOriginalLocation() when we identify changed nodes, this PR introduces a new method, relocateToHostRoot(). This method will move all nodes that were slotted into the changed node to the root of the element currently being rendered. So, when we enter the renderVdom() method for the component, it will be able to relocate any slotted content back to its new location.

Does this introduce a breaking change?

Yes
No

Testing

Manual testing
Two test cases can be reproduced using the repros from the linked issues and a build of this branch. These cases are also covered by e2e test.

A third test case can be created similar to the following (testing same behavior as first two, just more complex):

Create a new Stencil component starter
Enable extras.experimentalSlotFixes in the Stencil config
Create the following three components:

@Component({
  tag: 'super-cmp',
  shadow: false,
  scoped: false,
})
export class SuperCmp implements ComponentInterface {
  @Prop() tag: any = 'p';

  render(): HTMLElement {
    return (
      <Host>
        <slot name="tmp"></slot>
        <my-cmp tag={this.tag}>
          <p>YOOO</p>
          <slot></slot>
        </my-cmp>
      </Host>
    );
  }
}

@Component({
  tag: 'my-cmp',
  shadow: false,
  scoped: false,
})
export class MyCmp implements ComponentInterface {
  @Prop() tag: any = 'p';

  render(): HTMLElement {
    return (
      <Host>
        <base-cmp tag={this.tag}>
          <span>HOWDY</span>
          <slot></slot>
        </base-cmp>
      </Host>
    );
  }
}

@Component({
  tag: 'base-cmp',
})
export class BaseCmp implements ComponentInterface {
  @Prop() tag: any = 'p';

  render() {
    return (
      <Host>
        <this.tag>
          <slot />
        </this.tag>
      </Host>
    );
  }
}

Update the src/index.html to have the following body

<super-cmp>
  <p slot="tmp">Test</p>
  Hello
</super-cmp>
<button type="button">Click</button>

<script>
  document.querySelector('button').addEventListener('click', () => {
    document.querySelector('super-cmp').setAttribute('tag', 'span');
  });
</script>

The components will render correctly initially, but "Hello" will be in the incorrect location after clicking the button (should be in the span tag in the base-cmp component.
Install a build of this branch into the starter.
Run the project. The "hello" test will no relocate to the span tag after clicking the button. All "statically slotted" content will also render in the expected order ("yooo", "howdy", "hello")

Automated testing
All existing unit & e2e tests continue to pass. Also added new e2e tests for the reproduction cases in the linked issues.

Other information

Only available with the experimentalSlotFixes extra flag enabled.

If a slot is located in an element and that element's tag is dynamically changed (e.g. from `p` to `span`), we need to re-relocate the slot on re-render STENCIL-672: slot element loses its parent reference and disappears when its parent is rendered conditionally Fixes: #4284, #3913

github-actions · 2023-12-01T19:18:52Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1323 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/style/test/optimize-css.spec.ts	23
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts	22
src/compiler/prerender/prerender-main.ts	22
src/testing/puppeteer/puppeteer-element.ts	22
src/runtime/client-hydrate.ts	20
src/screenshot/connector-base.ts	19
src/runtime/vdom/vdom-render.ts	18
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/prerender/prerender-queue.ts	13
src/compiler/sys/in-memory-fs.ts	13
src/runtime/connected-callback.ts	13

Our most common errors

Typescript Error Code	Count
TS2345	399
TS2322	374
TS18048	286
TS18047	101
TS2722	37
TS2532	30
TS2531	23
TS2454	14
TS2352	13
TS2790	10
TS2769	8
TS2538	8
TS2416	6
TS2344	5
TS2493	3
TS2488	2
TS18046	2
TS2684	1
TS2430	1

Unused exports report

There are 14 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/utils/index.ts	269	normalize
src/utils/index.ts	7	escapeRegExpSpecialCharacters
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	61	satisfies
src/compiler/types/validate-primary-package-output-target.ts	61	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

tanner-reits

Couple notes

tanner-reits · 2023-12-01T19:18:38Z

src/runtime/vdom/vdom-render.ts

-      // // this child node in the old element is from another component
-      // // remove this node from the old slot's parent
-      // childNode.remove();


This chunk has been commented out for a while. I've tried uncommenting it a few times in different issues and haven't noticed any change in behavior so figured we just get rid of it to avoid future confusion

I'm in favor of this 👍

tanner-reits · 2023-12-01T19:21:13Z

src/runtime/vdom/vdom-render.ts

@@ -1027,7 +1064,7 @@ render() {
            // has a different next sibling or parent relocated

            if (nodeToRelocate !== insertBeforeNode) {
-              if (!nodeToRelocate['s-hn'] && nodeToRelocate['s-ol']) {
+              if (!BUILD.experimentalSlotFixes && !nodeToRelocate['s-hn'] && nodeToRelocate['s-ol']) {


Idea is that this whole block will go away once the slot fix flag is defaulted. Setting s-hn for elements from index.html actually causes some weird behavior when putting nodes back since it won't relocate a node to the same host node (which we actually want in some scenarios).

I didn't see any negative consequences from removing this while playing around with things, and all our automated tests still pass so feel like we're okay to do this.

rwaskiewicz

2 non-blocking nits

rwaskiewicz · 2023-12-01T21:02:39Z

src/runtime/vdom/vdom-render.ts

-      // // this child node in the old element is from another component
-      // // remove this node from the old slot's parent
-      // childNode.remove();


I'm in favor of this 👍

src/runtime/vdom/vdom-render.ts

christian-bromann

Code LGTM 👍

I was not quite able to reproduce the fixed using a starter project:

❯ ls -ls node_modules/@stencil/
total 0
0 lrwxr-xr-x@ 1 christian.bromann  wheel  67 Dec  4 09:37 core -> ../../../../../Users/christian.bromann/Sites/Ionic/projects/stencil
❯ npm start

> s5120@0.0.1 start
> stencil build --dev --watch --serve

[48:18.8]  @stencil/core
[48:18.9]  [LOCAL DEV] v4.8.0-dev.1701711877.75dbaa0 🦀
[48:19.6]  build, s5120, dev mode, started ...
[48:19.6]  transpile started ...
[48:20.6]  transpile finished in 1.04 s
[48:20.6]  copy started ...
[48:20.6]  generate lazy + source maps started ...
[48:20.6]  copy finished (0 files) in 14 ms
[48:20.8]  generate lazy + source maps finished in 184 ms
[48:20.8]  build finished, watching for changes...
           in 1.24 s

[48:20.8]  http://localhost:3333/

This should not be blocking the PR from being merged.

👍

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

tanner-reits · 2023-12-04T18:11:42Z

@christian-bromann Oops, should've mentioned in the test steps that you need to have experimentalSlotFixes enabled in the Stencil config:

// stencil.config.ts
extras: {
  experimentalSlotFixes: true
}

That could be why you weren't seeing the correct order after clicking the button. I'll update the steps for posterity.

christian-bromann · 2023-12-05T23:04:03Z

@tanner-reits I was able to verify the expected behavior after adding experimentalSlotFixes: true, thanks! 👍

christian-bromann · 2023-12-11T19:25:21Z

Released in v4.8.2

tanner-reits added 4 commits November 30, 2023 16:29

add e2e tests

0e9cd43

code documentation

dd05888

put changes behind slot fix flag

5a12d06

tanner-reits commented Dec 1, 2023

View reviewed changes

resolve new SNC

75dbaa0

tanner-reits marked this pull request as ready for review December 1, 2023 19:38

tanner-reits requested a review from a team as a code owner December 1, 2023 19:38

tanner-reits requested review from christian-bromann and rwaskiewicz December 1, 2023 19:38

rwaskiewicz approved these changes Dec 1, 2023

View reviewed changes

christian-bromann approved these changes Dec 4, 2023

View reviewed changes

Apply suggestions from code review

1101b21

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

tanner-reits added this pull request to the merge queue Dec 6, 2023

Merged via the queue into main with commit 4303d6a Dec 6, 2023
120 checks passed

tanner-reits deleted the treits/fix/slot-parent-tag-change branch December 6, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runtime): relocate slotted content when slot parent element tag changes #5120

fix(runtime): relocate slotted content when slot parent element tag changes #5120

tanner-reits commented Dec 1, 2023 •

edited

Loading

github-actions bot commented Dec 1, 2023 •

edited

Loading

tanner-reits left a comment

tanner-reits Dec 1, 2023

rwaskiewicz Dec 1, 2023

tanner-reits Dec 1, 2023

rwaskiewicz left a comment

rwaskiewicz Dec 1, 2023

christian-bromann left a comment

tanner-reits commented Dec 4, 2023

christian-bromann commented Dec 5, 2023

christian-bromann commented Dec 11, 2023

fix(runtime): relocate slotted content when slot parent element tag changes #5120

fix(runtime): relocate slotted content when slot parent element tag changes #5120

Conversation

tanner-reits commented Dec 1, 2023 • edited Loading

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

github-actions bot commented Dec 1, 2023 • edited Loading

--strictNullChecks error report

reports and statistics

Unused exports report

tanner-reits left a comment

Choose a reason for hiding this comment

tanner-reits Dec 1, 2023

Choose a reason for hiding this comment

rwaskiewicz Dec 1, 2023

Choose a reason for hiding this comment

tanner-reits Dec 1, 2023

Choose a reason for hiding this comment

rwaskiewicz left a comment

Choose a reason for hiding this comment

rwaskiewicz Dec 1, 2023

Choose a reason for hiding this comment

christian-bromann left a comment

Choose a reason for hiding this comment

tanner-reits commented Dec 4, 2023

christian-bromann commented Dec 5, 2023

christian-bromann commented Dec 11, 2023

tanner-reits commented Dec 1, 2023 •

edited

Loading

github-actions bot commented Dec 1, 2023 •

edited

Loading

`--strictNullChecks` error report