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] web components don't adopt SSR'd DSD from view-transitions #9953

Closed
1 task
hrmcdonald opened this issue Feb 2, 2024 · 17 comments
Closed
1 task
Labels
feat: view transitions Related to the View Transitions feature (scope) needs response Issue needs response from OP

Comments

@hrmcdonald
Copy link
Contributor

hrmcdonald commented Feb 2, 2024

Astro Info

Astro                    v4.2.8
Node                     v18.19.0
System                   macOS (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             auto-import
                         @astrojs/lit
                         @astrojs/mdx
                         @astrojs/react

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Our design system is built with Lit web components. Our docs site is built with Astro. We use the @astrojs/lit plugin to SSR all our components on pages and it works great.

However, recently we have been testing out Astro's newer view-transition functionality and we noticed a major issue going on here that is blocking us from adopting this functionality right now.

While the initial page load works as expected, any SSR'd web components loaded in a view-transition do not adopt their SSR'd shadow root. I was pointed to the fact that the browser's declarative shadow DOM parsing only runs during HTML parsing. I don't know enough about view-transitions or how Astro handles them to be able to know if this is why these DSD roots are not getting adopted properly. Is this a browser bug? Should or could it be handled by Astro?

What's the expected result?

In the example I have linked below, I have created a very simple Astro site with two pages, a web component on each, and view transitions enabled.

The first page you load (can be either one) everything parses and renders as expected. But once you click the link to transition in the other page, you will notice the second page's web component link does not render correctly. It is there, but it has not adopted its SSR'd shadow DOM and thus the link on the second page is not visible.

Initial load:
image

Page when loaded via view-transition:
image

Ideally, we want web components loaded during a view transition to get parsed like normal so they adopt their DSD. Otherwise we can't really utilize view transitions with SSR'd web components.

Link to Minimal Reproducible Example

https://stackblitz.com/~/edit/withastro-astro-ytwbdv?file=src%2Fpages%2Findex.astro&title=Astro%20Starter%20Kit:%20Blog

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Feb 2, 2024
@hrmcdonald hrmcdonald changed the title [astrojs/lit] web components don't adopt SSR'd shadow root from view-transitions [astrojs/lit] web components don't adopt SSR'd DSD from view-transitions Feb 2, 2024
@martrapp martrapp removed the needs triage Issue needs to be triaged label Feb 3, 2024
@martrapp
Copy link
Member

martrapp commented Feb 3, 2024

Hi @hrmcdonald, thank you for opening this issue!

It looks like we are not putting the template in the shadowRoot when we parse the new DOM from the file.

@martrapp
Copy link
Member

martrapp commented Feb 3, 2024

The problem only seems to occur in development mode.
@hrmcdonald, could you please check/confirm if it works for you with build/preview?

@martrapp martrapp added the needs response Issue needs response from OP label Feb 4, 2024
@hrmcdonald
Copy link
Contributor Author

It does appear that things are working properly in a production build.

@martrapp martrapp added - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs response Issue needs response from OP labels Feb 6, 2024
@martrapp
Copy link
Member

martrapp commented Feb 6, 2024

Still a bug so. Have to dive deeper into hydration. Any suggestions welcome. Will ask on discord, too :)

@martrapp
Copy link
Member

martrapp commented Feb 6, 2024

Preliminary analysis: For view transitions, we load the DOM with DOMParser.parseFromString(). No special attention is paid to templates and we have no special handling for shadowRoots. At first I thought that DSD support was simply missing, but then I got confused by the fact that it works with astro build. Maybe it's just a matter of re-initializing after swap. To be continued ...

@augustjk
Copy link
Contributor

augustjk commented Feb 6, 2024

FYI declarative shadow DOM won't be parsed with DOMParser.parseFromString(). Chrome shipped the ability to do so with a {includeShadowRoots: true} option passed in but it was before the spec was finalized. The correct way to parse string to a document while also letting the browser parse and attach the declarative shadowroots would be to use Document.parseHTMLUnsafe() which was added to the spec here whatwg/html#9538.

If you don't need an entire document and would like something similar to setting .innerHTML that also parses DSD, Element.prototype.setHTMLUnsafe() has also been added to the spec.
Here's an example of it in use with a feature detection and fallback to a ponyfill: https://github.com/lit/lit/blob/31c181a8a225b807719a3e52b980bf02dcb64255/packages/labs/testing/src/lib/fixtures/ssr-fixture.ts#L75-L86

Currently the unsafe HTML methods are available in the recently released Firefox 122, and soon to come Safari 17.4 (currently available in Safari Technology Preview). Chrome is in the process of implementing.

@martrapp
Copy link
Member

martrapp commented Feb 6, 2024

Hey @augustjk!

Thank you so much for your detailed explanation and advice! I really appreciate it!

Some how, we get things right when building for production. Let me learn some more about Lit 😄

@hrmcdonald
Copy link
Contributor Author

hrmcdonald commented Feb 6, 2024

Actually, when inspecting the production build a bit closer, I don't think things are working as expected still. It seems that in a prod build for some reason the web components are just re-initializing themselves properly when loaded via view-transition where they don't in dev. However, they still don't seem to be properly adopting their DSD template.

The stackblitz example is simple enough that it looks OK on the surface, but for anything more complex this still leads to a lot of issues.

@martrapp
Copy link
Member

martrapp commented Feb 8, 2024

Took a while.
Several thing come together here

  • @astrojs/lit had no support for view transitions. Relying on DOMContentLoaded for calling the hydrateShadowRoo() polyfil works for normal page loads, but not for view transitions
  • an uncaught exception interrupts the view transition and prevents the hydrateShadowRoot() polyfill being called from astro:after-swap. @hrmcdonald maybe you could fix this in the minimal reproduction?
    image
  • The polyfill is too smart to be useful on Chrome, as it detects that Chrome has support for {includeShadowRoots: true} parsing. It checks and just returns.
    We could parse with {includeShadowRoots: true} but as @augustjk noted, even for Chrome this doesn't seems to be the future-proof way forward. The alternatives however are so bleeding-edge that they are not even mentioned on mdn or caniuse. Nevertheless if I can not force the polyfill to be a bit more cooperative, {includeShadowRoots: true} might be the only option.

@martrapp martrapp added needs repro Issue needs a reproduction and removed - P3: minor bug An edge case that only affects very specific usage (priority) labels Feb 10, 2024
Copy link
Contributor

Hello @hrmcdonald. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with needs repro will be closed if they have no activity within 3 days.

@hrmcdonald
Copy link
Contributor Author

I updated the repro stackblitz to use link instead of href as a prop. I'm not sure what to recommend as the best path forward though from here. We can hold off on utilizing view transitions until browser support is better here.

@martrapp martrapp removed the needs repro Issue needs a reproduction label Feb 11, 2024
@martrapp
Copy link
Member

That at least changed the error message for href to link. ;-)
image
I am afraid that as long as there are uncaught exceptions in the browser console, they will prevent the shadow root from initializing correctly. I have no experience with lit and can't really help with fixing the lit problem. To get rid of the original error, I simply deleted the @property() href: string; and hardcoded the href in the anchor. But then the issue disappeared for me too.

Please help me with a reproduction that does not throw errors in the browser console during loading or view transitions.

@martrapp martrapp added the needs repro Issue needs a reproduction label Feb 11, 2024
Copy link
Contributor

Hello @hrmcdonald. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with needs repro will be closed if they have no activity within 3 days.

@martrapp martrapp removed the needs repro Issue needs a reproduction label Feb 11, 2024
@martrapp
Copy link
Member

Please note that the uncaught error also occurs independently of view transitions.
If <ViewTransitions /> is removed from Base.astro, the error will show up on page load.
So it's not so much a question of missing browser support for view transitions, but we should figure out how to fix the basic lit problem here.

@matthewp matthewp added the feat: view transitions Related to the View Transitions feature (scope) label Feb 12, 2024
@martrapp
Copy link
Member

@hrmcdonald here is the stackblitz for a minimal reproducible example that works for me.
https://stackblitz.com/c/edit/withastro-astro-wmttqt?file=src%2Fcomponents%2Fmy-custom-button.js&title=Astro%20Starter%20Kit:%20Blog
It doesn't throw errors, sets the shadow DOM, and shows working view transitions.
Testes on Chrome Version 121.0.6167.185 (Official Build) (64-bit).

Does that work for you, too?
Do you see any further issues?

@martrapp martrapp added the needs response Issue needs response from OP label Feb 14, 2024
@martrapp
Copy link
Member

@hrmcdonald I'm closing this issue now as there was no further response. Please feel free to reopen it at any time if you gain new insights.

@hrmcdonald
Copy link
Contributor Author

hrmcdonald commented Feb 26, 2024

Sorry for the delayed response here @martrapp. I don't know why the error you called out is getting thrown with decorator properties in the example, but even in your example that fixes that issue, while it looks like things are working properly, the web component is not initializing properly. Its SSR'd template shadow root is not getting properly adopted:

image

See how the SSR'd declarative shadow dom template is still orphaned here. It looks like the web component is just initializing from scratch instead of adopting the SSR'd shadow dom template like it should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: view transitions Related to the View Transitions feature (scope) needs response Issue needs response from OP
Projects
None yet
Development

No branches or pull requests

4 participants