fix(runtime): update textContent patch to mimic Shadow Root#5146
fix(runtime): update textContent patch to mimic Shadow Root#5146christian-bromann merged 7 commits intomainfrom
textContent patch to mimic Shadow Root#5146Conversation
This commit updates our patch for `textContent` to mimic the Shadow Root implementation Fixes: #3977 STENCIL-687
|
| Path | Location | Error | Message |
|---|---|---|---|
| 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 |
| } | ||
| if (BUILD.scopedSlotTextContentFix) { | ||
| patchTextContent(Cstr.prototype, cmpMeta); | ||
| if (BUILD.scopedSlotTextContentFix && cmpMeta.$flags$ & CMP_FLAGS.scopedCssEncapsulation) { |
There was a problem hiding this comment.
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.
| } | ||
| if (BUILD.scopedSlotTextContentFix) { | ||
| patchTextContent(HostElement.prototype, cmpMeta); | ||
| if (BUILD.scopedSlotTextContentFix && cmpMeta.$flags$ & CMP_FLAGS.scopedCssEncapsulation) { |
There was a problem hiding this comment.
Same as previous comment
|
|
||
| // If this is a default slot, add the text node in the slot location. | ||
| // Otherwise, destroy the slot reference node | ||
| if (node['s-sn'] === '') { |
There was a problem hiding this comment.
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.
| 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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Would it make sense to call trim() in the textContent getter?
| // 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah yes, forgot about the treeshaking aspect of that flag. I'll add that back before the component flag check
|
Released in |
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.
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.
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.
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.
What is the current behavior?
Stencil's current
textContentpatch forscopedelements only deals with a component's default slot. However, the team has recently agreed that ourscopedcomponent behavior should mimic ashadowcomponent as closely as possible.Fixes: #3977
What is the new behavior?
The
textContentpatch now behaves like the native implementation for an element with a Shadow Root. The getter will return the combinedtextContentof 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?
Testing
Manual testing
extras.experimentalSlotFixesin Stencil configmy-componentto the following:index.htmlto have the followingbody:npm starttextContentgetter/setter on the element from browser dev tools. Notice that it will only ever change the "Button" node in the default slottextContentgetter/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 setterAutomated testing
Update a few existing tests to account for whitespace differences in return value. Added new e2e tests to verify behavior for
scopedcomponents.Other information
Only available with the
experimentalSlotFixesextra 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.