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

Set style attributes directly instead of using cssText for inline styles #455

Closed
ccampbell opened this issue Apr 5, 2017 · 30 comments
Closed

Comments

@ccampbell
Copy link

I am working on an app where there is a lot of motion and positions that are updated dynamically based on the state.

I threw together a very crude demo here:
https://svelte.technology/repl?version=1.13.2&gist=ac946b7b6802e16d0e963d3043c5d1d2

Anyway, I noticed thate the generated code uses element.style.cssText. I didn’t really think much of it, but I thought I would throw together a jsperf test to compare it to setting the styles attributes directly. (I found a few existing jsperf tests, but they were just setting the same property/value over and over, so I didn’t really trust the results)

https://jsperf.com/csstext-vs-style-moving-element

It looks like cssText is approximately 15-30% slower in Chrome (in Safari it is around 50% slower).

Also I threw a test in there using cssText with unchanged styles and that makes the performance quite a bit worse. I am not 100% sure about what svelte is doing, but I believe it is rendering out the full style attribute string every time it re-renders an element.

I think it would be nice to update svelte so that it sets style properties directly. Not only would that be a bit faster, but it would allow the generated code to only update the styles that have changed vs. resetting the entire style attribute string each time.

I am sure that is quite a bit of work, but perhaps this could be considered a performance optimization for the future!

@evs-chris
Copy link
Contributor

Just wanted to note that the ever lovely CSSStyleDeclaration has a setProperty method that takes the guesswork out of converting css property names to js camel-cased-with-some-special-cases properties. It also lets you set a priority (!important) so you can cover all of the cases of inline style attributes.

I don't know how it benchmarks in comparison to setting the properties directly though, but I'd hope as well as, if not better.

@PaulBGD
Copy link
Member

PaulBGD commented Apr 5, 2017

The most performent thing to do would be to know at compile time what CSS styles we're setting, however we can't quite statically analyze the value of a styles property since it's dynamic. What I'm thinking is having us declare each CSS style like so:

<div style:top="{{top}}" style:position="relative"/>

which then would tell us what styles we're setting at compile time. I might be way off, but that's just my idea.

@Conduitry
Copy link
Member

Ooh, that might be a good idea. My initial thought was that we'd have to use an object of keys+values for styles, as in React. But I do like the idea of knowing at compile time what all the CSS properties are. I think both mechanisms (style="{{some dynamic string}} and style:property="{{value}}") would be able to live alongside one another.

@ccampbell
Copy link
Author

Oooh that is interesting. Just for my own understanding, the only way the style attribute would not be able to be statically analyzed is if a style property name itself is set dynamically, right? Like this:

Not possible to analyze

style="{{ someProp }}: {{ someValue}};"

Possible to analyze

style="left: {{ someValue}}px;"

I guess it is possible someValue in the second example could be set to 123px; right: 0 for the value. I think that is pretty unlikely though.

I know for me personally, I am never setting any style properties dynamically. I have done things like this:

style={{ isNew ? `perspective: ${width}px;` : '' }}

That also has the property itself available at build time though.

@evs-chris
Copy link
Contributor

evs-chris commented Apr 5, 2017

Ractive supports individual style properties as attributes e.g. <div style-top="{{foo}}px" style-left="{{bar}}px"> in addition to parsing out individual styles from an inline style attribute. You could probably support some level of parsing for inline style attributes in svelte, but it may not be worth the corner cases if attributes are addressable individually. Ractive does have the luxury/unfortunate overhead of a full runtime.

It may be worth noting that having class attributes get similar treatment is handy for not blowing away externally contributed classes e.g. <div class-foo="bar != 'whatever'"> only ever adds or removes a foo class to the element. Unforunately classList browser support isn't perfect, so it may be caveated or require a larger helper.

@PaulBGD
Copy link
Member

PaulBGD commented Apr 5, 2017

I know for me personally, I am never setting any style properties dynamically. I have done things like this:

style={{ isNew ? `perspective: ${width}px;` : '' }}

@ccampbell But you're still setting it dynamically, that ternary is ran at runtime and as well the value of width could be anything.

So I've been thinking about the syntax and I'm definitely wondering if along with defining styles we could mix in the shorthand attributes implemented with #384 (linking to PR since there's no documentation I found yet.)
Could look something like:

<div :style:top :style:left/>

<script>
export default {
  data() {
    return { top: '5px', left: '15px' }
  }
}
</script>

@ccampbell
Copy link
Author

ccampbell commented Apr 5, 2017

Ah okay I gotcha. Technically that could be compiled to something like this if that string was parsed also:

isNew ? setStyleAttribute(element, 'perspective', width + 'px') : '';

but probably that is a lot more effort than it is worth.


I like the style:top="{{ top }}px" syntax. Binding directly to properties is cool too, but I find that more often than not the properties I am setting are integers, so having to add the px every time to do the :style:top shorthand I don’t think would really be super helpful for me.

EDIT: Maybe you could use computed to add the px automatically?

@Rich-Harris
Copy link
Member

Yeah, the style:top='{{foo}}px' syntax seems reasonable (slightly confusing mismatch between the need for tags there with the prohibition on tags in the similar-looking bind:value='foo' directive, but I think we can live with it), would be pretty easy to handle. As long as style='{{bigBlobOfText}}' still works, then there doesn't seem to be any compelling reason not to do it.

I'd be interested in exploring whether we can infer key-value pairs from an attribute like this:

<div style='top: {{top}}px; left: {{left}}px;'></div>

As others have pointed out, you could definitely do something wacky that would break expectations, but I think in 99% of cases you could correctly translate that to...

<div style:top='{{top}}px' style:left='{{left}}px'></div>

...i.e., read a key, check it's a valid CSS key, then treat everything up to the next ; or the end of the attribute as the value. You'd probably want to have a compiler option that disabled that, just in case, but if it speeds apps up then let's do it.

I'd probably vote against doing the same thing with class because a) there's the browser support thing @evs-chris mentions, and b) classes don't typically change with the same frequency, unlike styles which could easily change 60 times a second.

I find that more often than not the properties I am setting are integers, so having to add the px every time

Maybe (and I'm just throwing this out there) we stipulate that style:top is equivalent to style:top='{{top}}px' rather than style:top='{{top}}'?

@PaulBGD
Copy link
Member

PaulBGD commented Apr 5, 2017

Maybe (and I'm just throwing this out there) we stipulate that style:top is equivalent to style:top='{{top}}px' rather than style:top='{{top}}'?

@Rich-Harris The issue is we don't know if the value is an integer or string, so it would break if someone passes '12pt'.

@Rich-Harris
Copy link
Member

Right, I'm saying we stipulate that if you choose the style:top syntax, you're agreeing that it's a number of pixels. Maybe that's taking things too far, and we should go the less magical route (I often use % values with top and left etc)

@PaulBGD
Copy link
Member

PaulBGD commented Apr 5, 2017

I definitely think we shouldn't do that for users, but how much code would it be to add it to the runtime?

prop.top = top + isNaN(top) ? '' : 'px';

doesn't seem too bad, especially if we threw it into a helper function. Also we'd only need to add it if someone uses the style: syntax.

@ccampbell
Copy link
Author

@PaulBGD @Rich-Harris just to clarify you are talking about using the proposed shorthand of :style:top to bind the top property directly to the style right? For the style:top="" i would vote that there shouldn’t be any magic involved there.

@evs-chris
Copy link
Contributor

evs-chris commented Apr 5, 2017

Regarding the distinction between having mustaches and not, the approach I've taken for explaining it is that things that are inherently strings, like the values in an inline style, attribute values, and interpolators amongst text nodes, require the mustaches. Things that are inherently values, like variable bindings/mappings, references inside a condition (e.g. it's {{#if foo + 2 > bar}} rather than {{#if {{foo}} + 2 > {{bar}} }}), and directive values, are already in a sort of mustache context and don't require additional mustaches. That mostly seems to make sense of when mustaches are required.

For classes, yeah, individual/independent access doesn't really make much sense until you get into decorators.

@PaulBGD
Copy link
Member

PaulBGD commented Apr 5, 2017

I'm just talking generally, I don't think the shorthand should act any differently.

@ccampbell
Copy link
Author

ccampbell commented Apr 5, 2017

The issue is if you do style:top="{{ top }}px" or style:top="{{ top }}%" then those make sense, but if you did style:top="{{ top }}" then I think it would be weird to make an assumption about the unit you want. Could be em too or other things.

@PaulBGD
Copy link
Member

PaulBGD commented Apr 5, 2017

@ccampbell Could be, but I think the reasoning is that if you have an invalid number for a CSS property, most browsers won't show the value in the inspect panel.

@Rich-Harris
Copy link
Member

This convo has persuaded me that we definitely shouldn't try to guess what units the user meant, and should always require that it be explicit — too much opportunity for confusion. Forget I mentioned it 😀

if you have an invalid number for a CSS property, most browsers won't show the value in the inspect panel.

We can always add dev mode checks that tell the user if the value is invalid CSS

@evs-chris
Copy link
Contributor

Regarding key-value pair inference, it seems to me that having a parser specifically for reading inline styles would be fairly reasonable, as parsing out style pairs isn't too terrible if you aren't concerned with validating the values. I think you could safely allow conditional blocks and more complex value mustaches that way. The failure mode on the parser when running across e.g. style="top: {{top}}px; {{morestyles}}" could just be to maybe throw out a warning and fall back to just setting cssText. There's also style:left="{{x}}px" style:top="{{y}}px" style="{{morestyles}}" that would be an issue if there's no runtime handling.

@PaulBGD
Copy link
Member

PaulBGD commented Apr 5, 2017

I just don't see the need to write a complex CSS parser if you can just specify what styles you're using, which could even support the existing shorthand to make it even easier.

@Rich-Harris
Copy link
Member

@PaulBGD it's about making it easy for people to get the benefits of directly setting styles without having to learn new syntax. I'm probably still going to do style='this: {{that}}' and so on just because it feels more natural and my colleagues already understand it (if you can use Svelte at all, you can grok that syntax). I don't think it'd be a complex parser — it's basically [key][colon][tag][semi?] until you run out of characters

There's also style:left="{{x}}px" style:top="{{y}}px" style="{{morestyles}}" that would be an issue if there's no runtime handling.

I reckon we'd allow one or the other for any given element, but not both.

@ccampbell
Copy link
Author

@Rich-Harris 👍

That was what I was originally thinking. Also cool with either/or. How do you think conditional styles would work? I have some things right now there are like

<div style="{{ playing ? `transform: translateX(${getWidth(time)}px);` : '' }}">

I think that would be the equivalent of this in the new proposed syntax:

<div style:transform="{{ playing ? `translateX(${getWidth(time)}px);` : '' }}">

I’m just not 100% sure how that would be handled. If the attribute is empty probably it should be reset to null since I am pretty sure that is what clears it out?

@Rich-Harris
Copy link
Member

Rich-Harris commented Apr 5, 2017

If the attribute is empty probably it should be reset to null since I am pretty sure that is what clears it out?

For most (all?) style properties I think setting it to the empty string resets it (and allows it to inherit styles: https://jsfiddle.net/wbLchk0q/)

(Edit: null works the same way! undefined does not)

@PaulBGD
Copy link
Member

PaulBGD commented Apr 5, 2017

But what about cases such as this one, which was suggested up above:

style="left: {{ someValue}}px;"

I guess it is possible someValue in the second example could be set to 123px; right: 0 for the value. I think that is pretty unlikely though.

@Rich-Harris
Copy link
Member

@PaulBGD combination of dev mode warning and compiler option should be enough, I reckon. Seems unfair to penalise the 99% just in case the 1% do something bonkers!

Rich-Harris added a commit that referenced this issue Sep 3, 2017
optimize style attributes
@Rich-Harris
Copy link
Member

As of 1.36, style attributes will be optimized to setProperty calls where possible:

<!-- setProperty('color', state.color) -->
<div style='color: {{color}}'></div>

<!-- setProperty('transform', "translate(" + state.x + "px," + state.y + "px)") -->
<div style='transform: translate({{x}}px,{{y}}px)'></div>

<!-- bails out -->
<div style='{{key}}: {{value}}'></div>
<div style='{{style}}'></div>

Unchanged (or static) properties are ignored when the DOM updates. I haven't implemented style:top etc as I'm not convinced it's necessary with the automatic optimizations.

@sayhicoelho
Copy link

sayhicoelho commented Sep 1, 2019

I would like to pass an object to style property. Like this:

<script>
  const myElementStyle = {
    width: 100,
    height: 100,
    display: 'block',
    color: 'red',
  }
</script>

<div style={myElementStyle}></div>

Instead of:

<script>
  const width = 100,
  const height = 100,
  const display = 'block',
  const color = 'red',
</script>

<div style="width: {width}px; height: {height}px; display: {display}; color: {color};"></div>

Or instead of:

<script>
  const width = 100,
  const height = 100,
  const display = 'block',
  const color = 'red',

  const myElementStyle = `width: ${width}px; height: ${height}px; display: ${display}; color: ${color};`
</script>

<div style={myElementStyle}></div>

Yes, I know I can make use of <style> css, but if we need to change these properties dynamically?

I would like to do the following:

<script>
  let pos ={
    x: 0,
    y: 0
  }

  $: myElementStyle = {
    top: pos.y,
    left: pos.x
  }

  function handleMousemove(e) {
    pos.x = e.clientX
    pos.y = e.clientY
  }
</script>

<style>
  .viewport {
    width: 400px;
    height: 200px;
    border: 1px solid gray;
    position: relative;
  }

  .viewport > div {
    width: 50px;
    height: 50px;
    position: absolute;
  }
</style>

<div class="viewport">
  <div
    on:mousemove={handleMousemove}
    style={myElementStyle}
  >
    {pos.x} x {pos.y}
  </div>
</div>

There is an approach relative to this?

UPDATE

Currently I've made a helper to format a javascript object to style format:

// utils.js
export function formatStyle(style) {
  const appends = {
    width: 'px',
    height: 'px',
    maxWidth: 'px',
    maxHeight: 'px',
    top: 'px',
    bottom: 'px',
    left: 'px',
    right: 'px',
    fontSize: 'px',
  }

  let result = ''

  for (let property in style) {
    result += `${property}: ${style[property]}${appends[property] || ''};`
  }

  return result
}
<!-- My component -->
<script>
  import { formatStyle } from './utils.js'

  const myElementStyle = formatStyle({
    width: 100,
    height: 100,
    display: 'block',
  })
</script>

<div style={myElementStyle}>...</div>

@gotofritz
Copy link

To save future wanderer time: the syntax discussed here doesn't work in 2020. Only standard HTML <div style="color: red"> works nowadays

@Conduitry
Copy link
Member

<div style="color: {color};"> also works. In version 2.0, the syntax for template expressions changed from {{double braces}} to {single braces}.

@gotofritz
Copy link

Sure. But what definitively doesn't work (and I wasted enough time on it...) is the <div style:color="red"> stuff

@Nantris
Copy link

Nantris commented Jun 26, 2020

I came across this issue from Google and wanted to mention that every individual style set causes a reflow. Using cssText should result in the styles being applied in a single reflow.

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

8 participants