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

Integrate Constructable Stylesheet support #689

Closed
kevinpschaaf opened this issue Dec 12, 2018 · 8 comments
Closed

Integrate Constructable Stylesheet support #689

kevinpschaaf opened this issue Dec 12, 2018 · 8 comments

Comments

@kevinpschaaf
Copy link
Member

Description

Chrome has announced an intent to implement "Constructable Stylesheets" and an implementation of the latest spec is available behind a flag. Using shared constructable stylesheets between e.g. instances of the same custom element as opposed to <style> element instances per template is expected to be a significant performance optimization.

This is a feature request to investigate feature detecting availability of Constructable Stylesheets (e.g. via 'adoptedStylesheets' in document) and adding a fork in the initial rendering to extract <style> elements from lit-generated templates and add stylesheets constructed from that CSS to the adoptedStylesheets array of the container's root node instead of stamping/appending <style> elements.

This can likely piggy-back on the same machinery used in shadyRender, which does a similar operation for ShadyCSS polyfill integration.

@LarsDenBakker
Copy link
Contributor

LarsDenBakker commented Dec 12, 2018

Wouldnt it be more efficient for custom element base classes to create style elements separately and register them rather than rendering and then query the dom to extract them?

For example styles can be a static getter on an element class.

@kenchris
Copy link
Contributor

kenchris commented Dec 12, 2018

Wouldnt it be more efficient for custom element base classes to create style elements separately and register them rather than rendering and then query the dom to extract them?

This won't render and extract them.

[update] Ah, I see what you are referring to now. @kevinpschaaf was suggesting extracting them from the html`` content. But I assume that will be done before any CSS parsing and rendering has been done, so it should be fine.

Check this example from the spec authors:

const myElementSheet = new CSSStyleSheet();
class MyElement extends HTMLElement {
  constructor() {
    super();
    const shadowRoot = this.attachShadow({mode: "open"});
    shadowRoot.adoptedStyleSheets = [myElementSheet];
  }
  
  connectedCallback() {
    // Only actually parse the stylesheet when the first instance is connected.
    if (myElementSheet.cssRules.length == 0) {
       myElementSheet.replaceSync(styleText);
    }
  }
}

This allows the same style to be shared across multiple instances of the same element.

Browsers do attempt to do style matching and sharing internally but its pretty brittle- and as with declaring it inside html`` as today, it means that it will get parsed each time, then matched up against matching styles and first then potentially shared.

@kevinpschaaf
Copy link
Member Author

Correct, this would be a one-time-per-class extraction of the <style> element(s) from the cached template that lit-html makes into a constructable stylesheet, and then a once-per-instance operation to assign the shared constructed stylesheet to the instance's adoptedStylesheets. Doing it this way (having users still write idiomatic <style> tags in their lit templates) means it will naturally just fall back to the current way of stamping<style> elements into shadowRoots on older browsers, while allow newer browsers to get the optimization without any changes to user code.

It does have the limitation of stylesheets not being mutable via ${bindings} (hence we'd probably do this as an opt-in/out-able render option), but that limitation already exists when using the polyfill via shadyRender, as the operation will be very similar: currently in shadyRender, upon first-ever render of a TemplateResult (which ensures any nested TemplateResults are interpolated at least once, which supports "style sharing" use cases, at least), we extract the <style> (from both the template and first rendering), shim it for ShadyCSS, and insert it into the head. For constructable stylesheets, the "shim and insert into head" step would just be changed to "construct a stylesheet" and then "add it to adoptedStylesheets on each instance".

@LarsDenBakker
Copy link
Contributor

LarsDenBakker commented Dec 18, 2018

But why have this awkward API?

lit-html has a render function for dynamically updating DOM. But with this we're telling users to put in a piece of static dom which they're not allowed to update and we magically move it somewhere else. It feels very counter intuitive, and it probably adds some overhead too?

Wouldn't this be a much better API for a custom element:

class MyElement extends LitElement {
  static get styles() {
    return html`
      <style>
        ...
      </style>
      ${sharedStyleModuleA}
      ${sharedStyleModuleB}
    `;
  }  

  render() {
    return html`
     ...HTML content
    `;
  }
}

This way it's much clearer for the user what's going on, and it's much easier for lit-html/LitElement to implement optimizations. You also don't need to pass any scopes/containers to the render function.

Possibly it could even integrate nicely with libraries like https://www.npmjs.com/package/lit-css:

class MyElement extends LitElement {
  static get styles() {
    return css`
      .foo {
        color: ${cssVar};
      }
      ${sharedStyleModuleA}
      ${sharedStyleModuleB}
    `;
  }  

  render() {
    return html`
     ...HTML content
    `;
  }
}

@web-padawan
Copy link
Contributor

web-padawan commented Dec 18, 2018

My 5 cents to the above comment: here is how <style> tag memoization is implemented in styled-lit-element built on top of the lit-css.

The combination of both and provides good DX so far, especially for extending custom elements and overriding style getter while calling super.style. See the example from my prototyping repo.

It would be nice if the constructable stylesheets could be implemented in the manner compatible with the same API (so that we could only switch internals without forcing users to rewrite their CSS).

@justinfagnani
Copy link
Collaborator

I do think that the simplest solution here is to separate styles from the template and create a StyleSheet object when possible, otherwise auto-inject a <style> tag into the ShadowRoot.

@kevinpschaaf
Copy link
Member Author

To follow up here, I believe we are going to go forward with a static get styles() approach. Thanks for the feedback.

@kevinpschaaf
Copy link
Member Author

Closing, as this was addressed in LitElement instead of shadyRender via lit/lit-element#401.

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

5 participants