Style tag injected into document head #571
Comments
|
Those are utility styles that were split out of the base stylesheet, as you've mentioned. The scroll lock class is used by a number of components via the internal scroll utility and will always be applied to the The toast stack styles are used by the alert component. When you have an Now that there's no base stylesheet requirement, I don't think it makes sense to put these into the light and dark stylesheets. They'd need to be duplicated, and it would mean third-party themes that don't use light/dark will have to reimplement them. It could be argued that the toast stack styles can be moved into the alert component, but that limits their reuse. (You might want to utilize the stack for other types of alerts, for example.) The case for moving scroll lock into a separate component is even less compelling.
Sort of. They're not being used in components. They're being used by components because it's impossible to encapsulate what they're doing from within a shadow root. I hope that explains why those exist. If you have a suggestion based on this new info, let me know! |
claviska
added
question
I mean this in the sense that I wouldn't usually expect importing a web component to modify my document scope styles. I expect the component's styles to be encapsulated and not affect the outside. In this case, I imported Also, this is "viral", in the sense if that if distribute a component that internally uses shoelace components, my end users are now going to have this unnecessary Thanks for the info about toast -- that's interesting! Is the reason you hoist toasts to the document scope to work around any z-index issues, where a child stacking context might prevent the toast from being on the top layer? I've been wondering about patterns for this myself recently too. A downside of hoisting (apart from breaking encapsulation) is that you lose any custom property values or ::part styles that the user might have applied to their toast, unless they also defined those values in the document scope, right? |
I agree, but I'm not aware of a better way. Perhaps the scroll utility could add the styles on lock and remove them on unlock. They still need to apply to the body, though. Aside: I use an
It's so you can more easily position the stack and not worry about heavy positioning algorithms and z-index conflicts (since alerts can appear anywhere in the DOM).
It's worth noting that, when you call There could be an
|
This is true, which is why the docs call out that the alert will be hoisted when they call |
Yeah, this makes sense, thanks for all the context! Hopefully whatwg/html#6349 will solve this properly eventually.
Maybe an improvement could be to hoist to a shadow root in the body, instead of into the light dom of the body? E.g. there could actually be a
|
|
Another issue with this inline styles is that they are preventing Shoelace to be used in contexts where a CSP preventing inline styles is in place. That may not be common, but it's a blocker for me :) |
|
If your CSP blocks inline styles then it will block both For example, it’s extremely common to apply something like |
|
Why can't something like I'm checking if I can use the |
|
Because the class has to reference a style that the library provides. Either we do: …and toggle the class on the body or we set its This problem isn’t exclusive to Shoelace. Bootstrap, for example, will also break. |
|
I don't understand why the |
|
They used to be, but folks seemed uncomfortable with global styles so utility classes were removed in beta.48. What was previously known as the “base” stylesheet no longer exists. Quite frankly, it was easier to put them in a base stylesheet but that also meant every single theme must implement them. One advantage to the current approach is that utilities just work. Another is that keeping them up to date is no longer the responsibility of theme developers. |
|
I see, thanks for the explanation! |
|
This caught me out as well when trying to tighten up my CSP. So currently I have to open up the CSP for Shoelace. It's the unsafe-inline which is the issue and is considered a security risk, which means it will get flagged in a Pen Test. |
|
Again, if your CSP restricts inline styles it will block both For example, the image comparer uses an inline style to set a calculated If anything, a CSP should limit specific properties and/or linked resources. Worried about malicious tracking images? Prevent those instead of blocking all valid uses of inline styles. Worried about malicious behaviors? Block the I'm willing to move the utility styles back into a stylesheet, but removing all instances of |
|
I think that even with the CSP blocking instances of |
|
It's less about how to work around it and more about why is there a need to work around it? IMO a blanket policy against all inline styles is too restrictive. It makes it harder to author components and harder for contributors because something very common and expected is now suddently an antipattern. 😕 CSPs should (if they don't already) allow more granular settings so as to only block properties and resources that are potentially harmful. That seems more logical than removing an important and useful piece of the platform that we've relied on for decades. |
|
Possibly some good news here. @fabricedesre is right in that setting the For example, the If that's the case, I'm happy to move the injected utility styles into the light/dark themes to work around it. Does everyone feel that this is a reasonable resolution? If so, I'll make the change before the next release. |
You're right, every web component tutorial I have seen so far uses this pattern.
If I set my CSP to Then my whole site is immune to both XSS and CSS injection. The style property could not be set by reflected code as the JavaScript would not be unsafe-inline. This is then a really nice defence in depth for the whole site as if I forget to sanitize even one input I am still protected.
If you have an easy fix for this, that would be amazing and well appreciated. |
|
I've updated the way theme files are maintained so they're raw CSS instead of Lit stylesheets. This was necessary because the magic that was producing the raw CSS before didn't allow for interpolation. To do this properly would have required a lot more complexity than I care to add, so instead the Lit styles are generated from the raw CSS now. With that problem solved, moving the utility styles into the theme stylesheets and scrapping the |
|
Forgot to mention: this will be available in the upcoming 2.0.0-beta.65 release. |
|
I'm posting this here for prosperity. This was an alternate approach for the I wasn't able to import the styles into Node because Lit requires a browser. I wasn't able to mock the necessary browser APIs using JSDOM for whatever reason. I ended using an Express app + Puppeteer to resolve the modules and get the raw CSS in a nasty way: import chalk from 'chalk';
import express from 'express';
import fs from 'fs';
import puppeteer from 'puppeteer';
import prettier from 'prettier';
import stripComments from 'strip-css-comments';
const app = express();
const port = 8088;
console.log('Generating stylesheets');
// Serve static files from the dist directory
app.use(express.static('./dist'));
// Create a fake endpoint to import and inject theme styles
app.get('/', function (req, res) {
res.set('Content-Type', 'text/html');
const html = `
<!DOCTYPE html>
<html lang="en">
<head><meta charset="UTF-8"></head>
<body>
<script type="module">
import light from 'http://localhost:${port}/themes/light.styles.js';
import dark from 'http://localhost:${port}/themes/dark.styles.js';
function appendStyles(id, styles) {
const style = document.createElement('style');
style.id = id;
style.textContent = styles;
document.body.append(style);
}
appendStyles('light', light.cssText);
appendStyles('dark', dark.cssText);
</script>
</body>
</html>
`;
res.send(Buffer.from(html));
});
const server = app.listen(port, () => {
console.log(`Stylesheet server is listening on port ${port}`);
});
// Launch Puppeteer targeting the / endpoint so we can scrape themes from it
(async () => {
try {
const browser = await puppeteer.launch();
const page = await browser.newPage();
await page.goto(`http://localhost:${port}/`, {
waitUntil: 'domcontentloaded'
});
// Stylesheets are injected into style containers with #light and #dark ids
const stylesheets = await page.evaluate(() => ({
light: document.getElementById('light').textContent,
dark: document.getElementById('dark').textContent
}));
// Write the raw CSS files
Object.keys(stylesheets).forEach(key => {
const css = stylesheets[key];
const formattedStyles = prettier.format(stripComments(css), { parser: 'css' });
fs.writeFileSync(`./dist/themes/${key}.css`, formattedStyles, 'utf8');
});
await browser.close();
server.close();
} catch (err) {
console.error(chalk.red('Error generating stylesheets!'));
console.error(err);
}
})();This approach works, but it's slow and heavy, which is why I reverted themes to be written in raw CSS. It's easier to generate the Lit-friendly modules than do it the other way around. 🤦🏻♂️ Are CSS modules a thing yet? 😅 |
Describe the bug
Importing a shoelace component inserts a
<style>tag into the document<head>.If shoelace components are being used in a shadow root, then this style won't have any effect.
Would it be possible to include these styles in the default stylesheet instead?
(I see it used to be in https://unpkg.com/browse/@shoelace-style/shoelace@2.0.0-beta.47/dist/themes/base.css, but now it's a separate and automatically inserted into the
<head>from https://unpkg.com/browse/@shoelace-style/shoelace@2.0.0-beta.57/dist/styles/component.styles.js).This isn't strictly causing a problem because the class names are unique enough, but I did find it a little odd and seemingly unnecessary, and it violates the principle of encapsulation.
To Reproduce
Observe new style tag in head:
The text was updated successfully, but these errors were encountered: