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

Adds static get styles() #401

Merged
merged 15 commits into from
Jan 10, 2019
Merged

Adds static get styles() #401

merged 15 commits into from
Jan 10, 2019

Conversation

sorvell
Copy link
Member

@sorvell sorvell commented Dec 22, 2018

Fixes #391.

This should be implemented to return an array of styles defined using the css tag function. These styles will be applied with adoptedStyleSheets where available and shimmed when not.

This should be implemented to return an array of styles defined using the `css` tag function. These styles will be applied with `adoptedStyleSheets` where available and shimmed when not.

WIP: needs tests.
@sorvell sorvell added this to the 1.1.0 milestone Dec 22, 2018
@sorvell sorvell requested a review from kevinpschaaf December 22, 2018 03:19
@daKmoR
Copy link
Contributor

daKmoR commented Dec 22, 2018

Questions: why is it an array? and not just

return css`
  h2 { color: orange; }
`;

using only a css tagged template literal you can do more advanced stuff like duplicate css.

e.g.

// somewhere
const headerColor = css`h2 { color: orange; }`;

// myBase.js
static get styles() {
  return css`
     ${headerColor}
     .footer { border-top: 1px solid green; }
  `;
}

// myEl.js
static get styles() {
  return css`
     ${super.styles}
     ${headerColor}
     .some { color: yellow; }
  `;
}

Output of myEl css

h2 { color: orange; }
.footer { border-top: 1px solid green; }
h2 { color: orange; }
.some { color: yellow; }

Output of myEl css WITH dedupe

h2 { color: orange; }
.footer { border-top: 1px solid green; }
.some { color: yellow; }

I am telling this as we have been using a BaseClass that supports style deduping for a while now and this is a really powerful feature and it makes the life of the css parser/shadycss just simpler by not needing to process as much css.

In this simple example it's not much but really if you have huge css blocks then deduping css improves performance a lot.

So I am really interested what the reasoning for using an array is.

PS: lit-css already supports this see https://github.com/lit-styles/lit-styles/blob/master/packages/lit-css/test/lit-css.spec.js#L29

@bashmish
Copy link

I'm happy to see that you folks take a step forward to support this. Would be nice if you take lit-css as an inspiration for your implementation. I personally always thought this should be part of the LitElement core. I do understand that tagged templates are not designed for such a use case or at least this use case with deduping is uncommon, but so far there were some users happy with it, for example, @web-padawan who prototypes new vaadin components.

@bashmish
Copy link

bashmish commented Dec 22, 2018

Also some ideas behind such a design I presented in this talk a few weeks ago https://youtu.be/wNKyEtLRTe4?t=2373 (will start at the most interesting part of the talk).

Between the lines it explains the motivation for lit-css and why the getter for styles in LitElement is static (we agree here anyway which is a good sign).

* Array of styles to apply to the element. The styles should be defined
* using the `css` tag function.
*/
static get styles() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add type of Array<CSSStyleSheet|LiteralString>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added specific type for this and used it here.

*/
export const supportsAdoptedStyleSheets = ('adoptedStyleSheets' in Document.prototype);

class LiteralString {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name this CSSLiteral

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
};

export const css = (strings: string[], ...values: any[]) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider having this return an object with an optional .styleSheet and .cssText so that there's a consistent return type for the css tag and so the static style property has a single item type.

This would change below to just:

   const cssText = values.reduce((acc, v, idx) =>
       acc + literalValue(v) + strings[idx + 1], strings[0]);
   return new CSSResult(cssText);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return value.value;
} else {
throw new Error(
`non-literal value passed to 'css' function: ${value}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to allow user-provided values in safe contexts with context-aware auto-escaping. Not sure how far we can go purely client-side. We may be able to allow directive-like functions that return a sentinel value that parses correctly for a particular context, then replace it, lit-html style. Soy does interesting things here at compile time. Maybe @mikesamuel has ideas.

styles.forEach((s) => {
const style = document.createElement('style');
style.textContent = s;
shadowRoot.appendChild(style);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to match Constructible StyleSheet ordering these have to come first in the ShadowRoot. Is that happening because you're directly waiting _updatePromise? Comment if so.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, comment added. Note, they come last in the shadowRoot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't they have to come first in case there are <style> tags in the rest of the shadow content?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -462,7 +472,25 @@ export abstract class UpdatingElement extends HTMLElement {
* @returns {Element|DocumentFragment} Returns a node into which to render.
*/
protected createRenderRoot(): Element|ShadowRoot {
return this.attachShadow({mode : 'open'});
const shadowRoot = this.attachShadow({mode : 'open'});
const styles = (this.constructor as typeof UpdatingElement).styles;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I (weakly) feel like all this should be in LitElement, since it does the actual rendering.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the shadowRoot rendering (creation + styles) should go together, but I think it's reasonable that this go into LitElement and not UpdatingElement. Let's discuss separately.

return this.attachShadow({mode : 'open'});
const shadowRoot = this.attachShadow({mode : 'open'});
const styles = (this.constructor as typeof UpdatingElement).styles;
if (window.ShadyCSS !== undefined && !(window.ShadyCSS as any).nativeShadow) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment the three cases a little

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@justinfagnani
Copy link
Contributor

@daKmoR styles is an array because elements may be sharing styles, and in a Constrctible StyleSheet supporting browser the css tag will return an actual StyleSheet object. We don't want to (and can't easily in all cases) merge these into each other with interpolations because adoptedStyleSheets is an array - we want to pass a reference to each StyleSheet object individually.

A modified version of your example:

const sharedStyles = css`
  h2 { color: orange; }
`;

class Base extends LitElement {
  static get styles() {
    return [
      sharedStyles,
      css`
        .footer { border-top: 1px solid green; }
      `
    ];
  }
}

class El extends LitElement {
  static get styles() {
    return [
      ...super.styles,
      // you probably know this is included in the parent, so this is dubious
      // we can still dedupe the arrays before setting `adoptedStyleSheets` but that
      // can change the cascade
      sharedStyles,
      css`
        .some { color: yellow; }
      `
    ];
  }
}

@b-strauss
Copy link

Is createRenderRoot the right place for this code?

Having a custom implementation for createRenderRoot for example here with custom options for attachShadow will require you to duplicate all the code.

@daKmoR
Copy link
Contributor

daKmoR commented Dec 22, 2018

// you probably know this is included in the parent, so this is dubious

unfortunately, you can not know that always... for example if you are a mixin.

So sometimes you have shared stylings for two mixins but each needs it to work on its own. However, you can also apply both mixins and then you will get duplicate css.

const srOnlyClass = css`.sr-only { ... }`;

const DescriptionMixin = ... // applies srOnlyClass so
class foo extends DescriptionMixin(BaseElement) // works
const DetailsMixin = ... // applies srOnlyClass so
class foo extends DetailsMixin(BaseElement) // works

// now this will apply srOnlyClass 2 times
class bar extends DescriptionMixin(DetailsMixin(BaseElement)) {}

we have this actually quite often in our codebase that there is shared stylings.

// we can still dedupe the arrays before setting adoptedStyleSheets but that
// can change the cascade

true so for us we always import the (parent) stylings at the top... which worked really nicely for us.

So overall arrays should work as well... I do find it a little bit more cumbersome but if it's more future proof then it's worth it.

But deduping should still be done... and for a css tagged template it was nicely part of the css tag itself... for array we either have to do return dedupeStyles([...super.styles, myStyleB]) or do "magic" with the array...

PS: I am off work till next year so the above example I made up... if need be I could see if I can look up some of our real examples

@LarsDenBakker
Copy link
Contributor

LarsDenBakker commented Dec 29, 2018

I really like this direction!

I think it's important to consider how to compose style modules:

const typographyStyles = css`...`;
const buttonStyles = css`...`;

// a standalone style module
const styleModuleA = css`...`;
// a style module that composes with another
const styleModuleB = [typographyStyles, css`...`];

class MyElement extends LitElement {
  static get styles() {
    return [
      typographyStyles,
      buttonStyles,
      styleModuleA,
      ...styleModuleB,
    ];
  }
}

I think this creates two challenges:

  1. Deduplication: Like @daKmoR mentioned you don't want to include the same styles twice. In this case typographyStyles will be included twice. You don't want to need to know within MyElement how styleModuleB is constructed internally so that you can leave out typographyStyles. Also if styleModuleB every changes internally, it will break your element.
  2. Composition: You don't want to have to know in MyElement which style modules are arrays and should be destructured, it's a cumbersome process and it exposes an extra implementation detail. Perhaps the same approach as Polymer 1 behaviors can be used here, where the final styles array is flattened. On modern browsers Array.prototype.flat is enough.

result.styleSheet = new CSSStyleSheet() as ConstructableStyleSheet;
result.styleSheet.replaceSync(cssText);
} else {
result.cssText = new CSSLiteral(cssText);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can always assign this field

@sorvell sorvell modified the milestones: 1.1.0, 1.0.0 Jan 7, 2019
Removes duplicate styles in the `static get styles` array. This is useful when styles are cmposed via mixins/subclasses. To try to preserve cascade ordering, the last duplicate style in the list is preserved.
@sorvell sorvell changed the title WIP: Adds static get styles() Adds static get styles() Jan 7, 2019
@blikblum
Copy link

blikblum commented Jan 8, 2019

Having a custom implementation for createRenderRoot for example here with custom options for attachShadow will require you to duplicate all the code.

Not only that, but just disabling shadowDom by returning this in createRenderRoot, as recommended in documentation, will break styles.

@justinfagnani
Copy link
Contributor

Not only that, but just disabling shadowDom by returning this in createRenderRoot, as recommended in documentation, will break styles.

That's just going to be true of any styles. If you have styles that assume ShadowDOM scoping, they're going to do the wrong thing if you don't render to a ShadowRoot. In this case at least nothing will happen with the styles unless you do something yourself, which is probably better than them being applied globally by default.


class CSSLiteral {

value: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

value: string;

constructor(value: string) {
this.value = value.toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the .toString()? If value can be something other than a string, update the type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

* property. Styling will apply using `shadowRoot.adoptedStyleSheets` where
* available and will fallback otherwise.
*/
protected createRenderRootStyles(shadowRoot: ShadowRoot) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is too generic, since this doesn't work with any render root, but only ShadowRoots. How about adoptStyles or applyStyles?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to adoptStyles

*/
protected createRenderRootStyles(shadowRoot: ShadowRoot) {
let styles = (this.constructor as typeof UpdatingElement).styles;
// de-duplicate styles preserving the last item in the list.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add why this is important

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added note, for performance.

protected createRenderRootStyles(shadowRoot: ShadowRoot) {
let styles = (this.constructor as typeof UpdatingElement).styles;
// de-duplicate styles preserving the last item in the list.
const stylesSet = new Set();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is better, but it gets rid of the deletes (which involve hashes) in exchange for a reverse:

const stylesSet = new Set();
for (const i = styles.length - 1; i >= 0; i==) {
  s.add(styles[i]);
}
styles = [...styleSet].reverse();

or:

styles = [...new Set([...styles].reverse())].reverse();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used reduceRight + reverse

styles.forEach((s) => {
const style = document.createElement('style');
style.textContent = s;
shadowRoot.appendChild(style);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/**
* Applies styling to the element shadowRoot using the `static get styles`
* property. Styling will apply using `shadowRoot.adoptedStyleSheets` where
* available and will fallback otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's describe the fallback. Specifically that it appends <style> elements to the ShadowRoot after the first render. This will be important for other rendering libraries.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

export * from './css-tag.js';

// Augment existing types with bleeding edge API.
declare global {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added types to env.d.ts

*/
export const supportsAdoptedStyleSheets = ('adoptedStyleSheets' in Document.prototype);

interface ConstructableStyleSheet extends CSSStyleSheet {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to w3c/csswg-drafts#3433 these additions are going to be merged into CSSStyleSheet, so add them globally as with adoptedStyleSheets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
};

export type CSSStyleSheetOrCssText = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Or is the best naming anymore. It's really specific to this operation, so something like CSSResult seems fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

* available and will fallback otherwise.
*/
protected createRenderRootStyles(shadowRoot: ShadowRoot) {
let styles = (this.constructor as typeof UpdatingElement).styles;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if styles is undefined and return to avoid extra works on elements that don't declare styles?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

@@ -0,0 +1,52 @@
/**
@license
Copyright (c) 2017 The Polymer Project Authors. All rights reserved.
Copy link
Contributor

@kenchris kenchris Dec 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 2019 I assume

* simplified `css` function and removed `cssLiteral`; values must be `css` results
* updated types and put into `env.d.ts`
* memoized unique-ifying `styles`
* `adoptStyles` now does the style adoption and is called separately from `createRenderRoot` and only if the root is a ShadowRoot.
* updated changelog.
@@ -415,6 +444,12 @@ export abstract class UpdatingElement extends HTMLElement {
*/
protected initialize() {
this.renderRoot = this.createRenderRoot();
// Note, tf renderRoot is not a shadowRoot, styles would/could apply to the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tf" should be "if"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ghost ghost mentioned this pull request Jan 8, 2019
Copy link
Contributor

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (when the ShadyCSS change is in)


_styleSheet?: CSSStyleSheet|null;

constructor(public readonly cssText: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not to use parameter properties, because it's non-standard JS outside of the type system. I collected guidance in the lit-html repo: https://github.com/Polymer/lit-html/blob/master/CONTRIBUTING.md#typescript

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (this._styleSheet === undefined) {
// Note, assume that if adoptedStyleSheets is supported,
// constructable stylesheets are as well.
if (supportsAdoptedStyleSheets) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename to supportsCSSShadowParts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to supportsAdoptingStyleSheets and added explicit check for construct-able stylesheet support.

*/
protected adoptStyles() {
const styles = (this.constructor as typeof UpdatingElement)._uniqueStyles;
if (!styles.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer if (style.length === 0)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Note, if renderRoot is not a shadowRoot, styles would/could apply to the
// element's getRootNode(). While this could be done, we're choosing not to
// support this now since it would require different logic around de-duping.
if (this.renderRoot instanceof window.ShadowRoot) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fail in a env that does not supports shadow dom and not is pollyfied. Such env would be supported at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

* moves `renderRoot` and all styling features from UpdatingElement to LitElement
* remove unncessary checking for constractable stylesheet support
})();

export const supportsAdoptingStyleSheets = supportsConstructableStyleSheets &&
('adoptedStyleSheets' in Document.prototype);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adoptedStylesheets is sufficient because it's unlikely a browser would ever ship that without constructable stylesheets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// This matches the spec'd behavior of `adoptedStyleSheets`.
// Ensure an updated is requested and then render styling directly after the update.
this.requestUpdate();
this._updatePromise.then(() => {
Copy link
Member

@kevinpschaaf kevinpschaaf Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing lit-html to have a render option that allows the user to specify a "before reference" to create the part before would allow us to remove this timing code here and instead put the <style>s in synchronously and just say render(result, this.renderRoot, {beforeRef: this.renderRoot.firstChild})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since part is cached off the container, avoiding this. However, moved this case to update to avoid the fancy waiting here.

// preserving the last item in the list. The last item is kept to
// try to preserve cascade order with the assumption that it's most
// important that last added styles override previous styles.
const styleSet = styles.reduceRight((set, s) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can just be styles.reduceRight((set, s) => set.add(s), new Set())

(set.add returns the set)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
## Unreleased

### Added
* Added `static get styles()` to allow defining element styling separate from `render` method.
This takes advantage of [`adoptedStyleSheets`](https://wicg.github.io/construct-stylesheets/#using-constructed-stylesheets) when possible ([#391](https://github.com/Polymer/lit-element/issues/391)).
* Added the `performUpdate` method to allow control of update timing ([#290](https://github.com/Polymer/lit-element/issues/290)).
* Updates deferred until first connection ([#258](https://github.com/Polymer/lit-element/issues/258)).
### Changed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add [breaking] change notice about moving createRenderRoot and related functionality from UpdatingElement to LitElement

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// (1) shadowRoot polyfilled: use ShadyCSS
// (2) shadowRoot.adoptedStyleSheets available: use it.
// (3) shadowRoot.adoptedStyleSheets polyfilled: append styles after rendering.
if (window.ShadyCSS !== undefined && !window.ShadyCSS.nativeShadow) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth moving more of the prep steps to the static one-time spot (_uniqueStyles)? Basically, move the map calls out of instance-time work, and possibly also create a template of styles to clone for the final case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem worth it.

Steven Orvell added 4 commits January 9, 2019 11:20
Cleans up the Shadow DOM + shim adoptedStyles case:
* avoids making the `updatePromise` protected
* handles stamping styles into shadowRoot after render.
@sorvell sorvell merged commit 22eca24 into master Jan 10, 2019
@sorvell sorvell deleted the static-styles branch January 10, 2019 02:42
@motss
Copy link

motss commented Jan 19, 2019

<style> tags no longer showing inside SD of a CE in DevTools on Chrome 73. Is that expected behavior with native support of Constructible Stylesheet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.