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

[RRFC] Add ability to set dynamic styles programatically after render #13

Open
1 task done
andy-blum opened this issue Nov 27, 2022 · 6 comments
Open
1 task done

Comments

@andy-blum
Copy link

andy-blum commented Nov 27, 2022

  • I searched for an existing RRFC which might be relevant to my RRFC

Motivation

In the documentation for styles, there is a brief section dynamic styles and classes. In this section it provides the below example (modified for clarity):

constructor() {
    super();
    this.styles = {color: 'lightgreen', fontFamily: 'Roboto'};
  }
  render() {
    return html`
      <div style=${styleMap(this.styles)}>
        Some content
      </div>
    `;
  }

The stylemap directive simply iterates over Object.entries(this.styles) and uses Array.prototype.reduce() to form a string that can be rendered as part of the component's own lifecycle. Styles added this way, however, are blocked when using a CSP with the style-src directive (MDN)

From the MDN page above:

Inline style attributes are also blocked:

<div style="display:none">Foo</div>

As well as styles that are applied in JavaScript by setting the style attribute directly, or by setting cssText:

document.querySelector('div').setAttribute('style', 'display:none;');
document.querySelector('div').style.cssText = 'display:none;';

However, styles properties that are set directly on the element's style property will not be blocked, allowing users to safely manipulate styles via JavaScript:

document.querySelector('div').style.display = 'none';

Example

On a page with the CSP header Content-Security-Policy: style-src https://example.com/

This code adds the styles to the markup, but the browser does not apply those styles, blocking them per the content security policy.

constructor() {
    super();
    this.styles = {color: 'lightgreen', fontFamily: 'Roboto'};
  }
  render() {
    return html`
      <div style=${styleMap(this.styles)}>
        Some content
      </div>
    `;
  }

How

I propose adding a new @ expression or similar, as we would for attaching event listeners, to programatically attach stylemap-ready objects post-render:

constructor() {
    super();
    this.styles = {color: 'lightgreen', fontFamily: 'Roboto'};
  }
  render() {
    return html`
      <div @style=${this.styles}>
        Some content
      </div>
    `;
  }

In this situation, the html tagged literal wouldn't render style="color: lightgreen; font-family: 'Roboto';", but instead render the div without a style element and then post-render run:

div.style.color = lightgreen;
div.style.fontFamily = 'Roboto';

References

@justinfagnani
Copy link
Contributor

justinfagnani commented Nov 28, 2022

The core of lit-html has no knowledge of classes and styles, they're just attributes, and classMap() and styleMap() are just directives. We don't want to add code to lit-html's core that deals directly in these concepts, so if we want to change the behavior of styleMap() we have to do it in that directive.

One way to do this would be to have styleMap() take a second options argument to tell it to skip the string rendering and go directly to updating the style object, which would happen around here: https://github.com/lit/lit/blob/main/packages/lit-html/src/directives/style-map.ts#L72

We'd need a name for this option. I'll call it directUpdate for now. It would look like:

constructor() {
    super();
    this.styles = {color: 'lightgreen', fontFamily: 'Roboto'};
  }
  render() {
    return html`
      <div style=${styleMap(this.styles, {directUpdate: true})}>
        Some content
      </div>
    `;
  }

@andy-blum
Copy link
Author

How would that integrate with the html tagged string? Doesn't styleMap() currently evaluate before render() does?

I was thinking the stylemap stuff would need evaluated here, along side .property, ?boolean-attribute, and @event.

Similar to how lit-html doesn't have knowledge of the event's name, callback, or options, but still adds it:

    if (shouldAddListener) {
      this.element.addEventListener(
        this.name,
        this,
        newListener as EventListenerWithOptions
      );
    }

Functionality could be added to apply styles to an element:

    if (shouldAddStyles) {
      Object.entries(styleMap).forEach((property, value) => {
        this.element.style[property] = value;
      }
    }

@justinfagnani
Copy link
Contributor

justinfagnani commented Nov 28, 2022

@andy-blum styleMap() is a directive, not just a function, so it's render() and update() methods are called by lit-html at the right times in the rendering pass.

The only thing that needs to be done here is to put a condition around the few lines that prime the previousStyleProperties set and return the result of render(). Something like:

    if (this._previousStyleProperties === undefined) {
      this._previousStyleProperties = new Set();
      if (this.directUpdate !== false) {
        for (const name in styleInfo) {
          this._previousStyleProperties.add(name);
        }
        return this.render(styleInfo);
      }
    }

You should be able to try this out by commenting out lines 69-72 locally and seeing that it works with CSP style-src.

@andy-blum
Copy link
Author

I've been poking at this again, and it seems that styleMap handles inline styles under a CSP just fine - except for the initial render.

See this codepen: https://codepen.io/andy-blum/pen/JjZQrwj

Notice how the styles property sets the background, padding, and fontsize.

If you remove the requestUpdate in firstUpdated, it'll fail as the styles added in a way the CSP blocks.

@justinfagnani
Copy link
Contributor

@kevinpschaaf I think this may be a reason to have update() not call render(), at least as an option.

@kevinpschaaf
Copy link
Member

kevinpschaaf commented Dec 14, 2022

Given that this is a pretty core directive with CSP implications, we could just bite the bullet and make a node build for styleMap where the string render is only done under the node condition. Then it'd just work with no DX implications.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 10, 2023
…first render r=mtigley

lit calls `element.setAttribute("style", "<our styles>")` on first render of an
element so that it will have the style property set in the markup if it is being
server-side rendered. We don't need that, and it causes CSP errors in
about:logins so this patches out the functionality.

lit/rfcs#13

Differential Revision: https://phabricator.services.mozilla.com/D165240
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jan 13, 2023
…first render r=mtigley

lit calls `element.setAttribute("style", "<our styles>")` on first render of an
element so that it will have the style property set in the markup if it is being
server-side rendered. We don't need that, and it causes CSP errors in
about:logins so this patches out the functionality.

lit/rfcs#13

Differential Revision: https://phabricator.services.mozilla.com/D165240
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

3 participants