fix(runtime): add display style to slot-fb elements #5028

tanner-reits · 2023-11-07T15:13:28Z

What is the current behavior?

Stencil wraps slot fallback content in a slot-fb element when we patch slot behavior in non-shadow components. This can cause issues with styles like display: flex that target slotted elements.

Fixes: #2937

What is the new behavior?

This adds a global style to set display: contents on all slot-fb elements. This is the same style that gets set on the slot elements in the native shadow DOM.

Does this introduce a breaking change?

Yes
No

Testing

Manual testing

Confirmed the fix in the reproduction in the linked issue

Automated testing

Added e2e tests to verify that the new styles are correctly applied
Restored the existing slot fallback tests that were disabled

Other information

github-actions · 2023-11-07T15:15:13Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1388 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/client-hydrate.ts	19
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/build/build-finish.ts	13
src/compiler/prerender/prerender-queue.ts	13

Our most common errors

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

Unused exports report

There are 15 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	251	resolve
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	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

Noting a few things

tanner-reits · 2023-11-07T15:14:21Z

src/runtime/bootstrap-lazy.ts

@@ -32,7 +32,7 @@ export const bootstrapLazy = (lazyBundles: d.LazyBundlesRuntimeData, options: d.
  const customElements = win.customElements;
  const head = doc.head;
  const metaCharset = /*@__PURE__*/ head.querySelector('meta[charset]');
-  const visibilityStyle = /*@__PURE__*/ doc.createElement('style');
+  const dataStyles = /*@__PURE__*/ doc.createElement('style');


Renamed this since it wasn't really only responsible for visibility styles anymore

solid rename 👍

tanner-reits · 2023-11-07T15:15:01Z

src/runtime/bootstrap-lazy.ts

+  const nonce = plt.$nonce$ ?? queryNonceMetaTagContent(doc);
+  if (nonce != null) {
+    dataStyles.setAttribute('nonce', nonce);


This shouldn't have been contingent on the invisiblePrehydration stuff in the first place

tanner-reits · 2023-11-07T15:15:47Z

src/runtime/runtime-constants.ts

@@ -60,4 +60,6 @@ export const HYDRATED_STYLE_ID = 'sty-id';
 export const HYDRATE_CHILD_ID = 'c-id';
 export const HYDRATED_CSS = '{visibility:hidden}.hydrated{visibility:inherit}';

+export const SLOT_FB_CSS = 'slot-fb{display:contents}slot-fb[hidden]{display:none}';


We have to have two style rules here rather than using :not([hidden]) since we support some browsers that don't support :not() yet

Can you do me a favor and add a (code) comment here with that information? That might not be immediately obvious if someone were reading the source file as-is.

tanner-reits · 2023-11-07T15:16:20Z

test/karma/test-app/slot-fallback/karma.spec.ts

@@ -1,7 +1,6 @@
 import { setupDomTests, waitForChanges } from '../util';

-// TODO(STENCIL-18) Restore this test and fix the underlying issue.
-xdescribe('slot-fallback', () => {
+describe('slot-fallback', () => {


Went ahead and restored these tests while I was in here since I needed to add some new ones anyway.

alicewriteswrongs

looks good to me!

rwaskiewicz · 2023-11-08T20:23:59Z

src/runtime/runtime-constants.ts

@@ -60,4 +60,6 @@ export const HYDRATED_STYLE_ID = 'sty-id';
 export const HYDRATE_CHILD_ID = 'c-id';
 export const HYDRATED_CSS = '{visibility:hidden}.hydrated{visibility:inherit}';

+export const SLOT_FB_CSS = 'slot-fb{display:contents}slot-fb[hidden]{display:none}';


Can you do me a favor and add a (code) comment here with that information? That might not be immediately obvious if someone were reading the source file as-is.

rwaskiewicz · 2023-11-08T20:34:25Z

src/runtime/bootstrap-lazy.ts

+  dataStyles.innerHTML = SLOT_FB_CSS;
+  dataStyles.setAttribute('data-styles', '');
+  head.insertBefore(dataStyles, metaCharset ? metaCharset.nextSibling : head.firstChild);


For my own understanding, why do we hoist this out of this conditional?

Because the styles for slot-fb should not be contingently created and applied based on the invisiblePrehydration config option. So, we always create and insert the style element, but will continue to only inject the invisiblePrehydration styles based on that config value.

Also added a quick comment in the code to help clarify that a bit.

Can you expound on that for me? invisiblePrehydration is true by default, which meant we were setting the innerHTML and data-styles by default for most folks only if hydratedClass or hydratedAttribute were set (or both). In fact, before invisibleHydration was added (a3d2928), this section looked like the following in ac49d7c:

if (BUILD.hydratedClass || BUILD.hydratedAttribute) { visibilityStyle.innerHTML = cmpTags + HYDRATED_CSS; visibilityStyle.setAttribute('data-styles', ''); head.insertBefore(visibilityStyle, metaCharset ? metaCharset.nextSibling : head.firstChild); }

That sill happens. So now, if you have a component using a slot outside the Shadow DOM (i.e. scoped or no encapsulation) and have the conditions for invisiblePrehydration, you get styles like the following:

slot-fb { display: contents } slot-fb[hidden] { display: none } my-component { visibility:hidden } .hydrated { visibility:inherit }

rwaskiewicz · 2023-11-08T20:34:45Z

src/runtime/styles.ts

@@ -67,6 +67,8 @@ export const addStyle = (styleContainerNode: any, cmpMeta: d.ComponentRuntimeMet
          styleContainerNode.insertBefore(styleElm, styleContainerNode.querySelector('link'));
        }

+        styleElm.innerHTML += SLOT_FB_CSS;


For my own understanding, why do we add this here?

This is how we style the slot-fb element for CE builds. This appends the slot-fb styles to the style element in the DOM. Added a code comment as well

This probably ties directly into my question above (GH won't let me link it since I haven't submitted the review yet):

Double checking - why should we always add this style to DOM, even if a component doesn't have any slots? I think that's still lost on me, even with the added comment

How do we know this is a slot-fb element?

You're right, we shouldn't. For some reason didn't realize we actually know if we need to apply this for each component. Updated to wrap this in a conditional based on CMP_FLAGS.hasSlotRelocation

rwaskiewicz · 2023-11-08T20:36:58Z

test/karma/test-app/invisible-prehydration-false/karma.spec.ts

    tearDownStylesScripts();
    await setupDom('/invisible-prehydration-false/index.html', 1000);

    // Is the component hydrated?
    expect(document.querySelector('prehydrated-styles').innerHTML).toEqual('<div>prehydrated-styles</div>');

-    expect(document.head.querySelectorAll('style[data-styles]').length).toEqual(0);
+    expect(document.head.querySelectorAll('style[data-styles]').length).toEqual(1);


Previously in these tests, checking for style[data-styles] was the only way to differentiate between invisible-prehydration being on/off. Can we update these tests to evaluate the content of that selector as well?

Oh, meant to mention this on my initial comment review. I tried to do this, but at least one of the tests would fail when checking the content. But, if you ran the failing test(s) on their own, they would pass. So, there's something weird with multiple tests running that are related. These tests use a component and script from a different build, so maybe something with that?

But I also tried updating the tests so they each had their own component, but ran into basically the same issue so the elements in the DOM head may not be getting created correctly between tests.

Let me know if you have any ideas and I can try them out.

rwaskiewicz · 2023-11-10T15:32:52Z

src/runtime/bootstrap-lazy.ts

@@ -185,16 +185,20 @@ export const bootstrapLazy = (lazyBundles: d.LazyBundlesRuntimeData, options: d.
    });
  });

+  // These style should always be constructed and inserted into the DOM
+  // We'll conditionally add some more styles later on
+  dataStyles.innerHTML = SLOT_FB_CSS;


Double checking - why should we always add this style to DOM, even if a component doesn't have any slots? I think that's still lost on me, even with the added comment

We shouldn't. Didn't realize we know specifically if any components are using slots outside the Shadow DOM. So, I added a boolean that will be set true if any components in the lazy build do that and that is used to conditionally add the slot-fb styles to the stylesheet in the DOM head.

rwaskiewicz · 2023-11-10T15:36:20Z

src/runtime/bootstrap-lazy.ts

+  dataStyles.innerHTML = SLOT_FB_CSS;
+  dataStyles.setAttribute('data-styles', '');
+  head.insertBefore(dataStyles, metaCharset ? metaCharset.nextSibling : head.firstChild);


Can you expound on that for me? invisiblePrehydration is true by default, which meant we were setting the innerHTML and data-styles by default for most folks only if hydratedClass or hydratedAttribute were set (or both). In fact, before invisibleHydration was added (a3d2928), this section looked like the following in ac49d7c:

if (BUILD.hydratedClass || BUILD.hydratedAttribute) { visibilityStyle.innerHTML = cmpTags + HYDRATED_CSS; visibilityStyle.setAttribute('data-styles', ''); head.insertBefore(visibilityStyle, metaCharset ? metaCharset.nextSibling : head.firstChild); }

rwaskiewicz · 2023-11-10T15:38:31Z

src/runtime/styles.ts

@@ -67,6 +67,8 @@ export const addStyle = (styleContainerNode: any, cmpMeta: d.ComponentRuntimeMet
          styleContainerNode.insertBefore(styleElm, styleContainerNode.querySelector('link'));
        }

+        styleElm.innerHTML += SLOT_FB_CSS;


This probably ties directly into my question above (GH won't let me link it since I haven't submitted the review yet):

Double checking - why should we always add this style to DOM, even if a component doesn't have any slots? I think that's still lost on me, even with the added comment

How do we know this is a slot-fb element?

tanner-reits added 6 commits November 6, 2023 13:19

update slot-fb display style for lazy build

f5317a9

update slot-fb display styles for CE build

fe4486e

rename visibilityStyle -> dataStyles

38f2c65

fix broken tests

0101d36

restore slot-fallback e2e tests

14138e9

add tests for slot-fb display styles

bec431f

tanner-reits commented Nov 7, 2023

View reviewed changes

tanner-reits marked this pull request as ready for review November 7, 2023 15:44

tanner-reits requested a review from a team as a code owner November 7, 2023 15:44

tanner-reits requested review from rwaskiewicz and alicewriteswrongs November 7, 2023 15:44

alicewriteswrongs approved these changes Nov 8, 2023

View reviewed changes

rwaskiewicz suggested changes Nov 8, 2023

View reviewed changes

add some comments

78d40fd

tanner-reits requested a review from rwaskiewicz November 9, 2023 14:58

rwaskiewicz reviewed Nov 10, 2023

View reviewed changes

tanner-reits added 2 commits November 10, 2023 11:55

conditionally add slot-fb styles

4a53474

revert test changes

6901e52

tanner-reits requested a review from rwaskiewicz November 10, 2023 17:27

rwaskiewicz approved these changes Nov 10, 2023

View reviewed changes

tanner-reits added this pull request to the merge queue Nov 11, 2023

Merged via the queue into main with commit 72c1f1a Nov 11, 2023
120 checks passed

tanner-reits deleted the treits/fix/slot-fb-styles branch November 11, 2023 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runtime): add display style to slot-fb elements #5028

fix(runtime): add display style to slot-fb elements #5028

tanner-reits commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 •

edited

Loading

tanner-reits left a comment

tanner-reits Nov 7, 2023

alicewriteswrongs Nov 8, 2023

tanner-reits Nov 7, 2023

tanner-reits Nov 7, 2023

rwaskiewicz Nov 8, 2023

tanner-reits Nov 7, 2023

alicewriteswrongs left a comment

rwaskiewicz Nov 8, 2023

rwaskiewicz Nov 8, 2023

tanner-reits Nov 9, 2023

rwaskiewicz Nov 10, 2023

tanner-reits Nov 10, 2023 •

edited

Loading

rwaskiewicz Nov 8, 2023

tanner-reits Nov 9, 2023

rwaskiewicz Nov 10, 2023

tanner-reits Nov 10, 2023

rwaskiewicz Nov 8, 2023

tanner-reits Nov 9, 2023

rwaskiewicz Nov 10, 2023

tanner-reits Nov 10, 2023

rwaskiewicz Nov 10, 2023

rwaskiewicz Nov 10, 2023

fix(runtime): add display style to slot-fb elements #5028

fix(runtime): add display style to slot-fb elements #5028

Conversation

tanner-reits commented Nov 7, 2023

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

github-actions bot commented Nov 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs 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

tanner-reits Nov 10, 2023 • edited Loading

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

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

github-actions bot commented Nov 7, 2023 •

edited

Loading

`--strictNullChecks` error report

tanner-reits Nov 10, 2023 •

edited

Loading