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

Complications from shadyCSS @apply shim and lit polyfill support #21748

Open
4 of 8 tasks
steverep opened this issue Aug 20, 2024 · 5 comments
Open
4 of 8 tasks

Complications from shadyCSS @apply shim and lit polyfill support #21748

steverep opened this issue Aug 20, 2024 · 5 comments
Assignees

Comments

@steverep
Copy link
Member

Checklist

  • I have updated to the latest available Home Assistant version.
  • I have cleared the cache of my browser.
  • I have tried a different browser to see if it is related to my browser.
  • I have tried reproducing the issue in safe mode to rule out problems with unsupported custom resources.

Describe the issue you are experiencing

Users have been reporting that some custom cards stopped working after being pushed to the legacy build (mistakenly for the iOS companion app). As @piitaya pointed out in home-assistant/iOS#2908 (comment), this is the result of those cards using dynamic styles in their templates, which are not supported by the shadyCSS polyfill. However, we only load that polyfill when shadow DOM is not supported, so it shouldn't be an issue. I decided to look into what's going on here.

What we do load unconditionally in the legacy build is lit/polyfill-support, which is needed whenever the web components polyfills are active. That support adds some functions for Lit with a short-circuit that looks like:

  if (
    window.ShadyCSS === undefined ||
    (window.ShadyCSS.nativeShadow && !window.ShadyCSS.ApplyShim)
  ) {
    return;
  }

It turns out that this short circuit doesn't work because we still use some <paper-*> components, which always load the shim for CSS @apply (via @polymer/polymer/polymer-legacy.js). So the end result is that the lit/polyfill-support effects are always active on the legacy build. Whether @apply is used by the cards or not, Lit's polyfill support ends up also forcing those shadyCSS limitations.

Describe the behavior you expected

Fortunately, there's a number of ways to address these complications:

  • Custom cards should stop using dynamic styles in their templates. Aside from these complications, they are terribly inefficient in lit, and will also present issues if a CSP is being enforced. Where CSS alone cannot do the job, lit's classMap or styleMap directives should be used. @piitaya I think you suggested adding some notes to the docs on this and perhaps a developer blog post - can you take care of that?
  • We don't load lit/polyfill-support on the modern build at all, so clearly we don't need it just for the @apply shim (probably because no lit elements actually use it, just the paper-* polymer elements). I'll modify this so we only load it when shadyDOM is active.
  • Remove remaining uses of <paper-*> elements, and thus the @apply shim will stop polluting our bundles along with plenty more polymer stuff. Thanks to @silamon, these have mostly been eradicated and there's only a few locations left (sidebar and dashboard tabs).
  • Lastly, we have a few locations where we use @apply with legacy paper styles. It's time to get rid of those, especially since the @apply proposal was abandoned years ago as a bad idea.

Steps to reproduce the issue

Can verify why the lit polyfill support is active just by looking at window.shadyCSS in the browser console.

What version of Home Assistant Core has the issue?

dev

What was the last working version of Home Assistant Core?

No response

In which browser are you experiencing the issue with?

n/a

Which operating system are you using to run this browser?

n/a

State of relevant entities

No response

Problem-relevant frontend configuration

No response

Javascript errors shown in your browser console/inspector

No response

Additional information

No response

@steverep
Copy link
Member Author

@bramkragten or @piitaya, I removed some orphan @apply uses in #21751, but it looks like we actually have a few uses in lit elements in the developer tools:

3 results - 2 files

src/panels/developer-tools/event/developer-tools-event.ts:
  162:           @apply --paper-font-body1;
  183:           @apply --paper-font-title;

src/panels/developer-tools/template/developer-tools-template.ts:
  314:           @apply --paper-font-code1;

Can we get rid of those? I suspect further investigation might show those amount to very minor bugs/differences between modern and legacy for those elements because of this issue.

@silamon
Copy link
Contributor

silamon commented Aug 21, 2024

There are more usage of the apply in the codebase but they are polymer related.

There is not really much more left to remove polymer completely.

  1. Dashboard tabs and developer tools tabs are pretty much done, but there's a prelimerary pr open at the moment before I open this one. The arrows are lost unfortunately, but the desktop scrolling works with some additional code.
  2. Sidebar, there's been a few closed prs already to replace it. The new m3 list doesn't have a selected property, so we need to migrate it to mwc list without doing a refactoring since it may end up closed as well.
  3. Remove polymer and the css variables. The hardest part is the fact that it is still being used all over the pages, we might need to introduce specific ha css variables that follow the material design so we are less dependant to changes to the material libraries.

@silamon
Copy link
Contributor

silamon commented Aug 21, 2024

@bramkragten or @piitaya, I removed some orphan @apply uses in #21751, but it looks like we actually have a few uses in lit elements in the developer tools:

3 results - 2 files

src/panels/developer-tools/event/developer-tools-event.ts:
  162:           @apply --paper-font-body1;
  183:           @apply --paper-font-title;

src/panels/developer-tools/template/developer-tools-template.ts:
  314:           @apply --paper-font-code1;

Can we get rid of those? I suspect further investigation might show those amount to very minor bugs/differences between modern and legacy for those elements because of this issue.

You can deduplicate them. That is look up what properties they set and copy them over and use css variables. But removing those will still leave the apply in the ha-style and I think it needs removal their too.

@steverep
Copy link
Member Author

You can deduplicate them. That is look up what properties they set and copy them over and use css variables. But removing those will still leave the apply in the ha-style and I think it needs removal their too.

At least for this issue, I left out the ones in ha-style as they are appended to the document rather than a lit element, so they shouldn't cause any modern/legacy differences. Keeping them just means we need to keep the shim.

Copy link

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.
Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍
This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 19, 2024
@silamon silamon removed the stale label Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants