-
Notifications
You must be signed in to change notification settings - Fork 319
Use link element with object URL to style in browsers that do not support adoptedStyleSheets #762
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
Conversation
…t adoptedStyleSheets
|
I'm not sure how good an idea this is given that blob URLs cause memory leaks (i.e. that CSS text needs to be kept in the blob URL store forever, even if no elements that use it exist on the page). |
Its something to consider but i don't see as a big issue because
While this still is not the optimal solution, seems to be better than current approach where if you have 10 elements, the styles are parsed 10 times and 10 copies of it are kept in memory. Maybe @emilio can weigh in since this is based on his idea |
|
I think you are probably right, especially about (2). The blob URLs could leak longer (I believe their lifetime causes them to survive navigations for example, if any other same-agent pages are open) but hopefully that won't be much of an issue. |
|
I'm wondering if the possible effect of this change can be verified with Tachometer benchmarks? cc @aomarks |
According to MDN: "Browsers will release object URLs automatically when the document is unloaded; however, for optimal performance and memory usage, if there are safe times when you can explicitly unload them, you should do so." I have a branch where the object URL is released just after first usage. It works on Firefox but this needs more tests since relies on the browser ability to cache the link styles See at blikblum@8fd479e |
I think that may work just by chance. By the time you load it you have already revoked the URL. A quick test shows that WebKit and Blink (contrary to what I thought!) do cache inline stylesheets in shadow trees. Blaming WebKit, it was implemented in https://bugs.webkit.org/show_bug.cgi?id=163353. Blaming Blink, it was implemented in https://bugs.chromium.org/p/chromium/issues/detail?id=308781. But anyhow, a very simple test-case that shows the perf difference in Firefox:
Given all other engines do have an inline style cache, I guess I may as well go implement that optimization in Firefox. We had a bug on file for this in https://bugzilla.mozilla.org/show_bug.cgi?id=1480146 (cc @bzbarsky). |
|
Note that part of that optimization may be historical too: by the time Blink and WebKit implemented it, |
|
This is very interesting! Thanks for the PR @blikblum and for the additional detail @emilio and @domenic. I thought that Firefox had implemented the same style caching that Chrome and Webkit did. Our bad for not confirming. We have been doing setting up benchmarking in CI recently, but we don't have anything that would isolate styling performance or memory. That would be good to add (cc @aomarks again). One thing that's been apparent recently is that a real polyfill for Constructible Stylesheets would be a lot more helpful than just including fallback support in LitElement. We have design systems implemented with LitElement that would like to allow shared styles without tightly coupling to LitElement's approach. Hopefully this can inform how we might approach that too. |
In my initial test, i revoked the URL after the link element is appended to the dom tree. Just for fun i tried to revoke before appending the link and, to my surprise, it worked. At least on Firefox. I did not test other browsers because my test setup requires native ES import support but will do, if not for fun.
If i understand correct, the inline stylesheets cache approach have some overhead over using link (calculate and compare hash of styles contents). Do you think there's some benefit of using link even when inline cache is implemented? (Testing in Safari should get this answer) |
I think it should be at least marginally better, yeah. |
|
@emilio: will a |
|
Hmm... I don't think you have that guarantee ( |
|
Hello! |
Actually I've implemented the same thing in a non-lit element, to try to share a medium CSS stylesheet with animations and the like, and it resulted in a FOUC on Firefox and Chrome (not tested elsewhere, FOUC on 2 browsers is enough for me). That might be a problem for lit-element since the behaviour is not the same as inline CSS and asynnc links will have visible downsides in a lot of projects. For the record, I ended up importing immediate-use styles in an inline |
|
Firefox 70 now caches stylesheets like Chrome and Safari: https://bugzilla.mozilla.org/show_bug.cgi?id=1480146 Thanks for that change @emilio! |
|
(That being said, |
|
Yeah, but we really can't allow the FOUC. |
|
|
|
In Chrome we see nice improvement from |
|
Sure, it doesn't matter in Chrome because Chrome's CSS parser is not thread-safe. But replaceSync requires you to parse the CSS in the main thread, while replace would allow Gecko to give the page more main-thread time. |
|
I think we've misunderstood each other here. I'm saying const styles = new CSSStyleSheet();
// Happens once
styles.replace(`...`);
class MyElement extends HTMLElement {
constructor() {
super();
// Happens for every instance
this.attachShadow({mode: 'open'}).adoptedStyleSheets = [styles];
}
}This is faster because per-instance there's no element, no additional text content, no need to even go through deduplication/caching machinery. |
|
Well, the difference between replace and replaceSync only happens once, but it doesn't mean that it doesn't matter. CSS parsing is a significant amount of page load, and being able to move it off the main thread was a significant page load win in a lot of sites for Gecko. The difference between adoption and going through the caching machinery should also be relatively small, but sure, there is a potential difference there. |
Uses a link element with href pointing to a object URL instead of a style element for browsers that supports native shadow dom but do not implement
adoptedStyleSheetsReference Issue
Fixes #761