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

Allowing number values in CSSResults #471

Closed
daKmoR opened this issue Jan 23, 2019 · 4 comments
Closed

Allowing number values in CSSResults #471

daKmoR opened this issue Jan 23, 2019 · 4 comments

Comments

@daKmoR
Copy link
Contributor

daKmoR commented Jan 23, 2019

hey,

we have some default number values for our styling.
Reason:

  • We want to keep them the same across multiple elements
  • These values should not be changeable (e.g. we don't want to use css variables)
    • as we don't want them as part of our "api"
    • as we want to do javascript calculations
    • as we want to keep the door open to optimize these variables "away" inside css via a build step

Live Demo

https://stackblitz.com/edit/ems7at?file=custom-greeting.js

Steps to Reproduce

Use a number

import { defaultPadding } from './css-constants.js';
// const defaultPadding = 5;

static get styles() {
  return [css`
    :host {
      padding: ${defaultPadding*2}px;
    }
  `];
}

Expected Results

Renders with padding of 10px.

imho Numbers should be allowed as they "are save" and can not be used for any form of css attack right?

Actual Results

Error thrown

Error: Value passed to 'css' function must be a 'css' function result: 10.

Browsers Affected

  • all

Versions

  • lit-element: v2.0.0-rc.x
@kenchris
Copy link
Contributor

CSS environment variables is supposed to handle this: https://drafts.csswg.org/css-env-1/#env-function

But it isn't specced yet how to add custom environment variables from either CSS or JS. Maybe @justinfagnani can push internally for moving that forward instead of the above idea

@sorvell
Copy link
Member

sorvell commented Jan 23, 2019

We've decided to add an unsafeCss function to help accommodate this. It's named "unsafe" so users are aware this must be done with care to avoid security issues.

@sorvell sorvell self-assigned this Jan 23, 2019
@sorvell sorvell added this to the 2.0.0 milestone Jan 23, 2019
sorvell pushed a commit that referenced this issue Jan 23, 2019
Fixes #451 and #471. Non-literal values can now be included in styling with `css` by using the `unsafeCss` function. This is named "unsafe" so users know this they must use this carefully to avoid security issues.
@daKmoR
Copy link
Contributor Author

daKmoR commented Jan 23, 2019

nice 💪 makes it definitely more useful 👍 especially for import.meta.url 🤗

I'm still unsure why such a wrapper would be needed for a value of type number?
I assumed a number is always safe? I feel like I am missing something if that is not the case 🙈

padding: ${unsafeCss( 5*2 )}px;

feels kinda strange...

dfreedm pushed a commit that referenced this issue Jan 24, 2019
* Adds `unsafeCss` for composing values into `css`

Fixes #451 and #471. Non-literal values can now be included in styling with `css` by using the `unsafeCss` function. This is named "unsafe" so users know this they must use this carefully to avoid security issues.

* Address review feedback

* add docs
* refine error message
@sorvell
Copy link
Member

sorvell commented Jan 24, 2019

Fixed via #474.

@sorvell sorvell closed this as completed Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants