fix(runtime): hide slotted content with no destination in scoped components by tanner-reits · Pull Request #5135 · stenciljs/core

tanner-reits · 2023-12-06T16:08:10Z

What is the current behavior?

Two current behaviors this solves:

If a slot is dynamically rendered in a component using scoped or no encapsulation and is not rendered initially, the content that would go into this slot is marked as hidden. But, if the slot was rendered on a subsequent render, we would not remove the hidden attribute, so the content would still be invisible in the DOM.
Content projected through to a component would not be hidden if it did not have a destination slot like how a component with a Shadow Root would hide any content in the light DOM not mapped to a slot.

Fixes: #2877

What is the new behavior?

Solutions in order of the issues listed above:

We now track an "initially hidden" state for an element. This value is used to "reset" the hidden attribute when content is successfully relocated to a "slot" location. This accounts for if a user manually set the hidden attribute on an element so it would remain hidden after relocation. There may be issues with this value getting programmatically changed throughout a component's lifetime and reset to an incorrect value. We can address this in the future as needed.
Leveraging the initial hidden state from Swiper #1, we now hide any content that is projected through to a scoped component that does not end up in a slot. If an appropriate "slot" is dynamically rendered later, the content will then be relocated and displayed accordingly.

Does this introduce a breaking change?

Yes
No

Testing

Manual testing

Spin up a new Stencil component library
Enable experimentalSlotFixes in the extras object of the Stencil config
Update the project's my-component.tsx and index.html files as follows:

// my-component.tsx

@Component({
  tag: 'my-component',
  shadow: false,
  scoped: true,
})
export class MyComponent {
  @Prop() enabled = false;

  render() {
    return (
      <Host>
        <p>Test</p>
        {this.enabled && (
          <div data-txt="slot wrapper">
            <slot>
              <span>fallback default slot</span>
            </slot>
          </div>
        )}
      </Host>
    );
  }
}

// index.html

<body>
    <my-component>
      <div>
        <span>outside default slot</span>
      </div>
    </my-component>

    <button type="button">Click</button>
</body>

<script>
    document.querySelector('button').addEventListener('click', () => {
      document.querySelector('my-component').setAttribute('enabled', true);
    });
</script>

Run the project and confirm that the div projected through from the index.html file does appear in the DOM despite the slot not being rendered.
Install a build of this branch
Re-run and observe the div is not visible until after clicking the button (which dynamically renders the slot).

Automated testing
Existing unit & e2e tests continue to pass. Added e2e tests for the reproduction case in the associated issue.

Other information

Only available with the experimentalSlotFixes extra flag enabled in a project's Stencil config.

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

It is possible for slotted content to still be invisible in the DOM if the slot was not rendered on the first render. This commit resets the `hidden` attribute of a node on successful relocation. STENCIL-1053

Hides any content that is projected through to a Stencil component that does not have a destination slot. Only for `scoped` components. Fixes #2877 STENCIL-938

…ination

github-actions · 2023-12-06T16:09:42Z

`--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

Called out one thing

tanner-reits · 2023-12-06T16:16:17Z

src/runtime/vdom/vdom-render.ts

+            // Store the initial value of `hidden` so we can reset it later when
+            // moving nodes around.
+            if (isInitialLoad) {
+              nodeToRelocate['s-ih'] = nodeToRelocate.hidden ?? false;


It might be possible to run into a situation where an element is programmatically hidden and we end up storing the incorrect state since this only captures the state on initial render. I figured that can be something we deal with as needed since the functionality is currently broken anyway so this is a step in the right direction

christian-bromann

One comment, but overall LGTM 👍

src/runtime/vdom/vdom-render.ts

rwaskiewicz · 2023-12-06T20:42:20Z

@tanner-reits I have a quick clarifying questions regarding the manual testing steps:

The component has the tag my-component, but the query selector in index.html references a "pui-example" - I'm guessing we can replace "pui-example" for "my-component" here?
Does this fix the issue for all encapsulation models? In the manual testing example, we're using shadow: true, which differs from the karma tests in this PR

tanner-reits · 2023-12-06T20:51:13Z

@rwaskiewicz Sorry bout that.

The query in the steps should be 'my-component', that was a copy-pasta error when renaming things from the reproduction repo I was using locally.
This change only modifies scoped. Which looks like that's the encapsulation mode I have in the testing steps so should be good there

rwaskiewicz · 2023-12-06T20:53:04Z

@tanner-reits my bad on question 2 - IDK what I was looking at 😓

Co-authored-by: Christian Bromann <git@bromann.dev>

christian-bromann · 2023-12-11T19:24:06Z

Released in v4.8.2

This commit adds a new extras flag, `experimentalScopedSlotChanges`, that is used to change logic in a few places to align Stencil's `scoped` encapsulation to work more like the native Shadow DOM when using slots. Changes in #5146 and #5135 are changed from being gated by the `experimentalSlotFixes` flag to this new flag so they can be toggled independently.

Tanner Reits added 10 commits November 30, 2023 16:29

add e2e tests

0e9cd43

code documentation

dd05888

put changes behind slot fix flag

5a12d06

resolve new SNC

75dbaa0

reset hidden state of nodes on relocate

0b9146d

It is possible for slotted content to still be invisible in the DOM if the slot was not rendered on the first render. This commit resets the `hidden` attribute of a node on successful relocation. STENCIL-1053

hide slot content without a home in scoped components

0c1f9f9

Hides any content that is projected through to a Stencil component that does not have a destination slot. Only for `scoped` components. Fixes #2877 STENCIL-938

add e2e tests for hiding content without a slot

f2b9c24

Merge branch 'main' into treits/fix/hide-slotted-content-with-no-dest…

b49520d

…ination

revert karma config

9966f15

tanner-reits commented Dec 6, 2023

View reviewed changes

tanner-reits marked this pull request as ready for review December 6, 2023 16:16

tanner-reits requested a review from a team as a code owner December 6, 2023 16:16

tanner-reits requested review from christian-bromann and rwaskiewicz December 6, 2023 16:16

christian-bromann approved these changes Dec 6, 2023

View reviewed changes

src/runtime/vdom/vdom-render.ts Outdated Show resolved Hide resolved

rwaskiewicz approved these changes Dec 6, 2023

View reviewed changes

PR feedback

2d2c299

Co-authored-by: Christian Bromann <git@bromann.dev>

tanner-reits enabled auto-merge December 6, 2023 21:33

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

Merged via the queue into main with commit 77bce27 Dec 6, 2023

tanner-reits deleted the treits/fix/hide-slotted-content-with-no-destination branch December 6, 2023 21:49

tanner-reits mentioned this pull request Jan 3, 2024

feat(runtime): add extras flag for scoped slot changes #5220

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(runtime): hide slotted content with no destination in scoped components#5135

fix(runtime): hide slotted content with no destination in scoped components#5135
tanner-reits merged 11 commits intomainfrom
treits/fix/hide-slotted-content-with-no-destination

tanner-reits commented Dec 6, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Dec 6, 2023

Uh oh!

tanner-reits left a comment

Uh oh!

tanner-reits Dec 6, 2023

Uh oh!

christian-bromann left a comment

Uh oh!

Uh oh!

rwaskiewicz commented Dec 6, 2023

Uh oh!

tanner-reits commented Dec 6, 2023

Uh oh!

rwaskiewicz commented Dec 6, 2023

Uh oh!

christian-bromann commented Dec 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tanner-reits commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

Uh oh!

github-actions bot commented Dec 6, 2023

--strictNullChecks error report

reports and statistics

Unused exports report

Uh oh!

tanner-reits left a comment

Choose a reason for hiding this comment

Uh oh!

tanner-reits Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rwaskiewicz commented Dec 6, 2023

Uh oh!

tanner-reits commented Dec 6, 2023

Uh oh!

rwaskiewicz commented Dec 6, 2023

Uh oh!

christian-bromann commented Dec 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tanner-reits commented Dec 6, 2023 •

edited

Loading

`--strictNullChecks` error report