emilio
commented
4 years ago Given replace() allows @import now (right?) there's no reason to not allow insertRule with the same semantics, unless I'm missing something.
Open emilio opened 4 years ago
emilio
commented
4 years ago Given replace() allows @import now (right?) there's no reason to not allow insertRule with the same semantics, unless I'm missing something.
rakina
commented
4 years ago I don't really remember the details but I think one of the reasons we wanted to disallow @import in insertRule was because it was because of fetch groups (see https://github.com/WICG/construct-stylesheets/issues/15#issuecomment-432238521). I'm not sure why the static vs dynamic differs here, probably @bzbarsky can explain more?
bzbarsky
commented
4 years ago That was back when the idea was that the same sheet could be shared across multiple documents. Static vs dynamic was relevant because for the static case there was an obvious document to use that actually made sense, whereas for dynamic there wasn't.
But in today's spec https://wicg.github.io/construct-stylesheets/#dom-documentorshadowroot-adoptedstylesheets setter step 2 prevents use across documents, so there is always a well-defined document and its fetch group can just be used. Unless we still plan to relax that restriction in the future, I don't see a reason to prevent dynamic @import.
tabatkins
commented
4 years ago Yeah, looks like we should be good to remove the restriction now.
css-meeting-bot
commented
4 years ago The CSS Working Group just discussed Add a note about the reasoning to forbid `insertRule(@import)`, or remove the condition?, and agreed to the following:
RESOLVED: Disallow @import in all constructable stylesheet apis with a note that we're doing it to match current state of modules and this might relax in futuretabatkins
commented
4 years ago Distilling the discussion:
Chris raised the problem the JS importing a CSS Module currently disallows @import rules (as a way of punting on whether the imported sheets should be part of the module graph (and thus shared between distinct imported sheets that @import the same URL) or not (distinct stylesheet objects)). If authors aren't going to be able to use @import in CSS Modules, it'll be weird to allow it here; it's unclear that toolchains will usefully manage that distinction.
Emilio's hidden point in his OP is that currently, .replace() on a constructed stylesheet does allow @import; it was weird that .insertRule() disallowed it. People on the call agreed this should be consistent.
Overall resolution is that we should keep the restriction, and make .replace() consistently restrict it as well.
We should also push for solving the CSS Module @import discussion one way or the other, and then be consistent with that in constructed stylesheets.
emilio
commented
4 years ago Given the discussion on the blink-dev mailing-list we should discuss what the error handling mechanism here should look like.
I've asked @astearns to put it on the agenda for the next week telecon, does that seem fine with everyone?
cc @mfreed7 @chirshtr
emilio
commented
4 years ago Oh, for reference the blink-dev thread is: https://groups.google.com/a/chromium.org/d/msg/blink-dev/RKG8oxp22RY/fdFnG1rGCgAJ
nordzilla
commented
4 years ago I think the following behavior for Constructed StyleSheets would make the most sense:
replace() ignores @import rules and continues to parse the rest of the sheet.replaceSync() ignores @import rules and continues to parse the rest of the sheet.insertRule() throws a SyntaxError if the rule is an @import rule.It is more consistent with existing behaviors to continue to parse the rest of the sheet, rather than to throw a NotAllowedError and reject the entire parse if there is an @import rule.
chrishtr
commented
4 years ago Does insertRule() throw SyntaxError in other cases?
emilio
commented
4 years ago Yes:
document.styleSheets[0].insertRule("@foobar") // SyntaxError: An invalid or illegal string was specifiedWhile I think I argued for "silently drop" in the call a bit ago, I think I've swung the other way, and believe we should reject the sheet entirely. That's more consistent with JS imports, which fail the module load entirely. They do that for a good reason, too - it makes it safer to upgrade into supporting @import later, because it's very likely that authors aren't depending on a sheet being completely ignored. (While a sheet working slightly wonky because some of its @imports don't load is more plausibly ignorable by an author.)
css-meeting-bot
commented
4 years ago The CSS Working Group just discussed Add a note about the reasoning to forbid `insertRule(@import)`, or remove the condition?, and agreed to the following:
RESOLVED: @import doesn't parse in constructable stylesheets and as a result throw syntax errorschrishtr
commented
4 years ago
- RESOLVED: @import doesn't parse in constructable stylesheets and as a result throw syntax errors
This resolution text is confusing. What we actually resolved is to throw syntax errors in insertRule, but otherwise ignore @import silently.
mfreed7
commented
4 years ago
- RESOLVED: span>@</spanimport doesn't parse in constructable stylesheets and as a result throw syntax errors
This resolution text is confusing. What we actually resolved is to throw syntax errors in insertRule, but otherwise ignore span>@</spanimport silently.
This was discussed in the blink-dev intent to ship thread, but for those arriving here from the Chromium deprecation message, here is the more detailed description of the changes, at least as they are implemented starting in Chromium 84:
devingfx
commented
1 year ago Hi!
So what is the recommandation to import stylesheets in a custom elements though? (still without duplication with constructed stylesheets?)
aralroca
commented
5 months ago Hi!
So what is the recommandation to import stylesheets in a custom elements though? (still without duplication with constructed stylesheets?)
@devingfx I use it in this way (if there is a better way, please comment):
const sheet = new CSSStyleSheet();
const css: string[] = [];
for (const { cssRules } of document.styleSheets) {
for (const rule of cssRules) css.push(rule.cssText);
}
sheet.replaceSync(css.join(""));
shadowRoot.adoptedStyleSheets.push(sheet);IMHO there should exist a native way to do it that includes the global styles and also applies to the Declarative Shadow DOM. For example:
<template shadowrootmode="open" adoptglobalstyles>
</template>Or:
this.adoptGlobalStyles(); // also reacts dynamically to changes on global styles
rramsey
commented
5 months ago Ok, kind of new to components and wondering why this wouldn't work in a stencil based custom element:
@Component({
tag: 'my-custom-element',
styleUrls: [
'my-custom-element.css',
],
shadow: true,
})
export class MyCustomElement{
render() {
return (
<div class="to-top">
<link rel="stylesheet" type="text/css" href="https://path.to.cdn/my-colors.css" />
<p class="myclass">the quick brown fox jumped over the lazy dog</p>
</div>
};
}
}Then in my-custom-element.css file I have:
p {
color: var(--color-from-cdn);
} Testing locally with just npm run start in my StencilJS element it is correctly pulling the css files down from the cdn (visible in Chrome's Network tab) and I can see that it is applying the color.
So what am I missing? Granted, these are very basic css files and usage. So maybe it is ok until I try to get too fancy?
tia for any advice or comments
devingfx
commented
5 months ago @rramsey This discussion is about native custom elements, not any fancy pre-compiler doing what it want with your code... (PS: Dunno Stencil but it's out of scope :S )
@aralroca Thanks but this is a hack, not a recommendation...
I asked OP: Thanks forbiding everything in waiting for "future relaxing" but what is the solution to practicaly use constructed style sheet without @import ?
[sarcasm with=❤️] Maybe the providencial new toys: Copilot™ will help making web specs great again? [/sarcasm]
rramsey
commented
5 months ago @devingfx - no problem. That was just an example from code I knew that worked. I haven't actually tested this javascript, just simplified code I found on the web:
let myTemplate = document.createElement('template');
myTemplate.innerHTML = `
<style>
:host {
--varOne: blue;
}
</style>
<link rel="stylesheet" type="text/css" href="https://path.to.cdn/my-colors.css" />
<p style="color: var(--varOne);">I am a blue paragraph.</p>
<p style="color: var(--varTwo);">My color comes from the cdn variable defined in :host in my-colors.css on an external website.</p>
`;
customElements.define('my-cool-element', class extends HTMLElement {
constructor() {
super();
let shadowRoot = this.attachShadow({mode: 'open'});
shadowRoot.appendChild(myTemplate.content.cloneNode(true));
}
});The principal is the same though: what, if anything, is wrong with putting a link to an external stylesheet in the html that is going inside the shadowdom? Is it any different (better? worse?) than just using javascript to create a link with document.createElement('link') and attaching it to the body element?
Or feel free to delete and point me to a better place to ask. :)
devingfx
commented
5 months ago @rramsey
The goal of constructed stylesheet is to avoid the duplication of parsing/creating a CSSOM document in memory. In you code exemple, if you instance several <my-cool-element> each one will load the css file (so your browser cache will help to avoid actual several downloads) and parse it into a CSSOM (you know, the JS object) for each one separatedly, and then call a render pass.
Let's say your css is bag of crap generated by a preprocessor and come to be 100Kb in size, let's say the CSSOM JS version take the same amount of RAM (what is surely not true), and let's say you instance 100 of elements, then you RAM will be filled with 10Mb.
With constructed stylesheets, the parsing and CSSOM creation is made only once, and all the custom elements will share the same CSSOM... (only 100Kb in RAM with the previous exemple).
Another advantage is the live modification of this CSSOM: if you change values of you CSS variables in it (constructed.cssRules[0].setProperty()), all elements will instantly be aware of it instantly, against having to modifi each element's internal el.shadowRoot.firstChild.sheet.cssRules[0].style.setProperty(...)
rramsey
commented
5 months ago @devingfx - thanks, that makes sense. Since I knew the file sizes, I wasn't as concerned about that. Seems like this comes back to the trade-offs you have to think about in creating multiple custom elements that share the same styles.
Do you embed the css in the element, potentially creating a dozen elements, all with exactly the same css? Do you link to a common style sheet, in our instance a cdn file that really is just a list of variables and colors based on our branding and design, but then have the stylesheet possibly downloaded multiple times and increase the size of the sheets in ram in your example? Having the values in the cdn is great because there is a single source of truth for all your elements and if the design team wants to change --my-purple from #7C18FF to #6027EB, you change it in one spot and suddenly all of your components have the correct coloring after a literal 10 second fix in notepad/vi/emacs. Or do you do the really hard thing, add the js to a dozen elements to grab the source of truth from the cdn, parse it, update the constructed rules, then display the component.
You are correct, the first couple of options are quick, easy, and potentially increase bloat. The last option is probably the preferred option, but requires more code to do properly. Plus it has to be done before the component is actually added to the dom.
I will hope that I can up my skills and do the second version. But sometimes, when you know the files in question, taking the easy route is tempting. I know that our color file, uncompressed, with comments is 1,480 bytes. So even if it were duplicated 100 times, that's a 140kb. You can't download a logo from most sites for less than that and god help you if you are surfing amazon or loading a google font. :)
Thank you for taking the time for the explanation, I really do appreciate it and want to learn.
I get the reasoning to forbid
@importfromreplaceSync, but it's not clear to me why forbidding it ininsertRuleis desired, or necessary. We allowinsertRulewith@importwith other stylesheets.Looking a bit at the history this comes from https://github.com/WICG/construct-stylesheets/issues/56 and such.
Do we still have hopes to eventually be able to share these stylesheets across documents? If so it makes sense to add a note to the spec saying that this is why this restriction is there.
If not, this restriction should probably be removed?
cc @bzbarsky @rakina @tabatkins