Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[@astrojs/lit] Slotted content is wrapped in <astro-slot> elements which breaks native slot functionality #5475

Closed
1 task done
ba55ie opened this issue Nov 25, 2022 · 14 comments · Fixed by #5791
Closed
1 task done
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)

Comments

@ba55ie
Copy link
Contributor

ba55ie commented Nov 25, 2022

What version of astro are you using?

1.16.11

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

Windows, Linux

Describe the Bug

When using a client directive or the Class constructor for Lit Web Components an extraneous astro-slot element is created which interferes with styling of content within native slot elements.

When looking at the example MyCard elements, the first two don't get the correct css styling. The h1 should be hotpink and the footer text should be gray.

The added <astro-slot><h1>...</h1></astro-slot> element interferes with the styling, because the ::slotted selector only supports compound top-level selectors: https://developer.mozilla.org/en-US/docs/Web/CSS/::slotted

There are also some render issues. I also tried the is:raw directive, but that doesn't seem to make a difference.

image

image

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-wqbjru?file=src/components/my-card.ts

Participation

  • I am willing to submit a pull request for this issue.
@ba55ie ba55ie changed the title Hydrated/JSX Lit Web Components create an astro-slot element that interferes with native slot elements Hydrated/JSX Lit Web Components create an astro-slot element that interferes with native slot element styling Nov 25, 2022
@ba55ie ba55ie changed the title Hydrated/JSX Lit Web Components create an astro-slot element that interferes with native slot element styling Hydrated/JSX Lit Web Components create an astro-slot element that interferes with native slot elements Nov 25, 2022
@hecspc
Copy link

hecspc commented Nov 25, 2022

We are facing the same problem. Not only the <astro-island> element make the ::slotted selector useless, but also the pattern to inspect the slot elements.

We do have some components that have the next structure:

<tab-bar>
  <tab>Item 1<tab>
  <tab>Item 2</tab>
<tab-bar>

This pattern is quite regular with other components like Switcher, List and similar. As you can imagine, the container component, in the example above the tab-bar component, needs to know its children. We do so querying the slots elements or using the event @slotchange, as suggested in the Lit documentation.

It invalidates the use of the decorator @queryAssignedElements since this is only working at the top level and not recursively and this is unfortunate since this decorator is widely used.

Also, these components could grow dynamically, inserting more items into the container. Normally they would be inserted as children of the container element, but with the astro-slot component results into a strange structure difficult to parse

<list>
  <astro-slot>
     <list-item>Old Item</list-item>
     <list-item>Old Item</list-item>
  </astro-slot>
   <list-item>New Item</list-item>
   <list-item>New Item</list-item>
</list>

Here there is an example
https://stackblitz.com/edit/github-jybjrd?file=src/pages/index.astro

@ba55ie
Copy link
Contributor Author

ba55ie commented Nov 29, 2022

@hecspc Thank you for the additional explanation. You are right about the DOM APIs that aren't working as well.

It seems like the bug is in the @astrojs/lit integration and not in the core itself:

yield `<astro-slot>${value || ''}</astro-slot>`;

@ba55ie
Copy link
Contributor Author

ba55ie commented Nov 30, 2022

I created a PR: #5500

To test it locally replace in node_modules/@astro.js/lit/server.js line 63-76 with:

if (slots) {
	for (const [slot, value] of Object.entries(slots)) {
		const htmlString = value || '';

		if (slot === 'default') {
			// This will render inside the Lit Component default slot.
			yield htmlString;
		} else {
			// Add the slot attribute to all the nested HTML elements,
			// so that it can be rendered inside the appropriate Lit Component slot.
			yield htmlString.replace(/<(?<tag>\w+)/gm, `<$<tag> slot="${slot}"`);
		}
	}
}

Result

image

@hecspc
Copy link

hecspc commented Nov 30, 2022

@ba55ie I am afraid that the element <astro-slot> is needed for hydration as shown in the line packages/astro/src/runtime/server/astro-island.ts#105.

Actually making a quick search for astro-slot in the repo you can see that this element is included for all integrations like preact, solid, svelte, vue or react. So just removing the element will lead to more important issues when trying to hydrating the components.

I am afraid that this issue is a problem of the chosen astro structure and it is much deeper that just an integration problem. I hope that this can be solved but due to the lack of support or not enough traction of lit I don't think that this is going to get the enough attention to be solved.

But until this is not solved the lit integration or any webcomponents can not be used in depth with a proper components set that are not just the simple ones from the examples.

The irony is that astro-island and astro-slot are web components!

@ba55ie
Copy link
Contributor Author

ba55ie commented Nov 30, 2022

Lit has its own exprimental-hydrate-support.js and the <astro-slot> doesn't seem to be needed for Lit hydration. The <astro-island> is still present and this component loads the hydration scripts.

https://lit.dev/docs/ssr/client-usage/

I am working on a large SSR project and all components seem to hydrate just fine.

@ba55ie ba55ie changed the title Hydrated/JSX Lit Web Components create an astro-slot element that interferes with native slot elements [@astrojs/lit] Slotted content is wrapped in <astro-slot> elements which breaks native slot functionality Dec 1, 2022
@ba55ie ba55ie changed the title [@astrojs/lit] Slotted content is wrapped in <astro-slot> elements which breaks native slot functionality [@astrojs/lit] Slotted content is wrapped in <astro-slot> elements which breaks native slot functionality Dec 1, 2022
@ba55ie ba55ie changed the title [@astrojs/lit] Slotted content is wrapped in <astro-slot> elements which breaks native slot functionality [@astrojs/lit] Slotted content is wrapped in <astro-slot> elements which breaks native slot functionality Dec 1, 2022
@ba55ie
Copy link
Contributor Author

ba55ie commented Dec 1, 2022

Close, but no cigar. 😢

I got it somewhat working, but it doesn't seem very solid. I also had to take into account the use case of nested Web Components which resulted in a messy regex.

Hopefully somebody from Astro or the Lit team can chime in? Maybe @justinfagnani ?

@ba55ie
Copy link
Contributor Author

ba55ie commented Dec 1, 2022

Btw, this is the PR that introduced the <astro-slot> functionality: #3652

@justinfagnani
Copy link

I just took a look at the Stackblitz and I'm not sure what we can do from the Lit side here. If an element expects children of a certain type, or needs to directly style children, then inserting another element like <astro-slot> is going to interfere no matter what.

Lit doesn't actually do anything with slots. They are entirely a native feature. Client-side and server-side we just render templates into a shadow root. The platform takes care of the rest.

It seems like there needs to be some way for Astro to not add <astro-slot> elements.

@justinfagnani
Copy link

Looking at the linked PR, can the changes to the Lit renderer that inject <astro-slot> be reverted? https://github.com/withastro/astro/pull/3652/files#diff-5726c9aa2e37e9167488c7f949d39a731f5f9aa83c6cac69aa5b0d317bc007f8R63

cc @natemoo-re

@matthewp
Copy link
Contributor

matthewp commented Dec 6, 2022

I think we probably shouldn't be adding the astro-slot elements here, which is used to emulate slots in non-wc frameworks. I would think that not doing so would fix this issue.

@matthewp matthewp added the - P3: minor bug An edge case that only affects very specific usage (priority) label Dec 6, 2022
@ba55ie
Copy link
Contributor Author

ba55ie commented Jan 6, 2023

I think we probably shouldn't be adding the astro-slot elements here, which is used to emulate slots in non-wc frameworks. I would think that not doing so would fix this issue.

Yup, the tricky part was restoring the original slot="name" attributes on the child elements of the <astro-slot slot="name"> elements.

I've created a new PR.

@matthewp
Copy link
Contributor

matthewp commented Jan 6, 2023

Fixed by #5776

@matthewp matthewp closed this as completed Jan 6, 2023
@matthewp
Copy link
Contributor

matthewp commented Jan 6, 2023

@ba55ie can you submit this same PR against main so that it gets into 2.0 as well? We won't be merging the 1.x branch back manually.

@ba55ie
Copy link
Contributor Author

ba55ie commented Jan 7, 2023

@matthewp Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants