fix(runtime): update `textContent` patch to mimic Shadow Root #5146

tanner-reits · 2023-12-08T15:49:53Z

What is the current behavior?

Stencil's current textContent patch for scoped elements only deals with a component's default slot. However, the team has recently agreed that our scoped component behavior should mimic a shadow component as closely as possible.

Fixes: #3977

What is the new behavior?

The textContent patch now behaves like the native implementation for an element with a Shadow Root. The getter will return the combined textContent of all child nodes that are located in slot references. And the setter will replace all content within slot reference nodes with a single text node in the default slot reference (if one exists).

Does this introduce a breaking change?

Yes
No

Testing

Manual testing

Create new component starter
Enable extras.experimentalSlotFixes in Stencil config
Update my-component to the following:

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.css',
  scoped: true,
})
export class MyComponent {
  render(): HTMLElement {
    return (
      <Host>
        <button>
          <div class="content-wrapper">
            <span class="prefix">
              <slot name="prefix" />
            </span>
            <slot />
            <slot name="suffix" />
          </div>
        </button>
      </Host>
    );
  }
}

Update index.html to have the following body:

          <my-component>
            <span slot="prefix">Prefix</span>
            <p>Button</p>
            <span slot="suffix">Suffix</span>
          </my-component>

Run npm start
Execute the textContent getter/setter on the element from browser dev tools. Notice that it will only ever change the "Button" node in the default slot
Install a build of this branch
Execute the textContent getter/setter on the element from browser dev tools. Notice that it will now return the value of all slotted content and will replace all nodes when using the setter

Automated testing
Update a few existing tests to account for whitespace differences in return value. Added new e2e tests to verify behavior for scoped components.

Other information

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

This does not include a patch for innerText. That method is trickier to patch to correctly get whitespace and line break formatting.

This commit updates our patch for `textContent` to mimic the Shadow Root implementation Fixes: #3977 STENCIL-687

github-actions · 2023-12-08T15:51:28Z

`--strictNullChecks` error report

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

Unfortunately, it looks like that's an increase of 1 over main 😞.

Violations Not on `main` (may be more than the count above)

Path	Location	Error
src/runtime/bootstrap-custom-element.ts	(107, 9)	TS18048
src/runtime/bootstrap-custom-element.ts	(108, 23)	TS18048
src/runtime/bootstrap-custom-element.ts	(110, 43)	TS2345
src/runtime/bootstrap-custom-element.ts	(111, 52)	TS2538
src/runtime/bootstrap-custom-element.ts	(111, 52)	TS2538
src/runtime/bootstrap-custom-element.ts	(117, 9)	TS18048
src/runtime/bootstrap-custom-element.ts	(117, 9)	TS2322
src/runtime/bootstrap-custom-element.ts	(119, 22)	TS2345
src/runtime/dom-extras.ts	(214, 64)	TS2345
src/runtime/dom-extras.ts	(266, 13)	TS18047
src/runtime/dom-extras.ts	(344, 11)	TS2532
src/runtime/dom-extras.ts	(398, 5)	TS2322

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

Few notes

tanner-reits · 2023-12-08T15:51:18Z

src/runtime/bootstrap-custom-element.ts

@@ -57,8 +56,8 @@ export const proxyCustomElement = (Cstr: any, compactMeta: d.ComponentRuntimeMet
    if (BUILD.appendChildSlotFix) {
      patchSlotAppendChild(Cstr.prototype);
    }
-    if (BUILD.scopedSlotTextContentFix) {
-      patchTextContent(Cstr.prototype, cmpMeta);
+    if (BUILD.scopedSlotTextContentFix && cmpMeta.$flags$ & CMP_FLAGS.scopedCssEncapsulation) {


This additional condition is the same one that was a top-level check in the patchTextContent method. Just moved it here so we don't need to change things later when we make the experimentalSlotFixes the default behavior and remove these other extras options.

tanner-reits · 2023-12-08T15:51:28Z

src/runtime/bootstrap-lazy.ts

@@ -160,8 +159,8 @@ export const bootstrapLazy = (lazyBundles: d.LazyBundlesRuntimeData, options: d.
        if (BUILD.appendChildSlotFix) {
          patchSlotAppendChild(HostElement.prototype);
        }
-        if (BUILD.scopedSlotTextContentFix) {
-          patchTextContent(HostElement.prototype, cmpMeta);
+        if (BUILD.scopedSlotTextContentFix && cmpMeta.$flags$ & CMP_FLAGS.scopedCssEncapsulation) {


Same as previous comment

tanner-reits · 2023-12-08T15:53:48Z

src/runtime/dom-extras.ts

+
+          // If this is a default slot, add the text node in the slot location.
+          // Otherwise, destroy the slot reference node
+          if (node['s-sn'] === '') {


I decided to make it so that the text node just won't be put into the DOM at all if there isn't a default slot reference node. Reason being you can't use hidden on text nodes so you'd have to style it to actually hide it, but it would still show up in textContent based on the spec.

christian-bromann

One comment

christian-bromann · 2023-12-08T17:14:45Z

test/karma/test-app/scoped-slot-text/karma.spec.ts

@@ -30,7 +30,7 @@ describe('scoped-slot-text', () => {

    cmpLabel.textContent = 'New text to go in the slot';

-    expect(cmpLabel.textContent).toBe('New text to go in the slot');
+    expect(cmpLabel.textContent.trim()).toBe('New text to go in the slot');


For my understanding, why is this change necessary? Is it defined in the spec how spaces or new lines are treated when getting the textContent of a node and does our patch behaves different here?

Whitespace is the annoying part while doing these text patches. I did my best to produce the same output that the exact same component using shadow: true would return. The resulted in the the string always (in my finding) having spaces padding the start and end.

Would it make sense to call trim() in the textContent getter?

rwaskiewicz · 2023-12-08T19:14:35Z

src/runtime/bootstrap-custom-element.ts

-  // default our pseudo-slot behavior
-  if (BUILD.experimentalSlotFixes && BUILD.scoped) {
+  // TODO(STENCIL-914): this check and `else` block can go away and be replaced by just the `scoped` check
+  if (BUILD.experimentalSlotFixes && cmpMeta.$flags$ & CMP_FLAGS.scopedCssEncapsulation) {


For my own understanding, why did we remove the BUILD.scoped check here? Wouldn't we want this to get removed from the build output if there aren't any scoped components in a project?

Ah yes, forgot about the treeshaking aspect of that flag. I'll add that back before the component flag check

christian-bromann · 2023-12-11T19:28: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 6 commits December 6, 2023 12:06

only patch scoped components

080930c

patch textContent for scoped components

7168f40

This commit updates our patch for `textContent` to mimic the Shadow Root implementation Fixes: #3977 STENCIL-687

fix e2e tests for new implementation

9b5f902

ignore comment nodes in getter

1f1755b

fix issue with removing multiple nodes in slot for setter

d341be5

add e2e tests for textContent patch

4dbd686

tanner-reits commented Dec 8, 2023

View reviewed changes

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

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

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

christian-bromann reviewed Dec 8, 2023

View reviewed changes

rwaskiewicz reviewed Dec 8, 2023

View reviewed changes

re-add build flag checks

2a87841

tanner-reits requested a review from rwaskiewicz December 8, 2023 20:02

rwaskiewicz approved these changes Dec 8, 2023

View reviewed changes

christian-bromann approved these changes Dec 8, 2023

View reviewed changes

christian-bromann added this pull request to the merge queue Dec 11, 2023

Merged via the queue into main with commit 55c56d6 Dec 11, 2023
120 checks passed

christian-bromann deleted the treits/fix/scoped-text-content-patch branch December 11, 2023 17:59

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

fix(runtime): update `textContent` patch to mimic Shadow Root #5146

fix(runtime): update `textContent` patch to mimic Shadow Root #5146

tanner-reits commented Dec 8, 2023

github-actions bot commented Dec 8, 2023 •

edited

Loading

tanner-reits left a comment

tanner-reits Dec 8, 2023

tanner-reits Dec 8, 2023

tanner-reits Dec 8, 2023

christian-bromann left a comment

christian-bromann Dec 8, 2023

tanner-reits Dec 8, 2023

christian-bromann Dec 8, 2023

rwaskiewicz Dec 8, 2023

tanner-reits Dec 8, 2023

christian-bromann commented Dec 11, 2023

fix(runtime): update textContent patch to mimic Shadow Root #5146

fix(runtime): update textContent patch to mimic Shadow Root #5146

Conversation

tanner-reits commented Dec 8, 2023

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

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

--strictNullChecks error report

reports and statistics

Unused exports report

tanner-reits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-bromann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-bromann commented Dec 11, 2023

fix(runtime): update `textContent` patch to mimic Shadow Root #5146

fix(runtime): update `textContent` patch to mimic Shadow Root #5146

github-actions bot commented Dec 8, 2023 •

edited

Loading

`--strictNullChecks` error report