Setting css variables by a style object - style.setProperty is needed

[ChrisKuBa]

Hi,

I think an example is not necessary, because it is a raw javascript issue.

inferno/src/DOM/props.ts - patchStyle

The function sets the styles via object access: domStyle[style] = ...

The proplem is, that css variables can only be set by domStyle.setProperty(style, ...)

Please change that, if there isn't a performance or compatibility impact.

Thx

[Havunen]

Hi @chrkulbe thanks for reporting the issue. Fixing this seems trivial, but could you provide test case so we can make sure it does not break in future again please?

[ChrisKuBa]

Hi,

after writing a test its seems, setProperty has different parameters, because it requires the the hyp-hen syntax instead of camelCase keys. To prevent breaking changes its the best that only css vars will be set with setProperty. Maybe this has a performance impact, because an additional condition and two string reads for every changed style are necessary.

function isCssVariable(style) {
  return style[0] === '-' && style[1] === '-'; 
}

// We are assuming here that we come from patchProp routine
// -nextAttrValue cannot be null or undefined
function patchStyle(lastAttrValue, nextAttrValue, dom) {
  const domStyle = dom.style;
  let style;
  let value;

  if (isString(nextAttrValue)) {
    domStyle.cssText = nextAttrValue;
    return;
  }

  if (!isNullOrUndef(lastAttrValue) && !isString(lastAttrValue)) {
    for (style in nextAttrValue) {
      // do not add a hasOwnProperty check here, it affects performance
      value = nextAttrValue[style];
      if (value !== lastAttrValue[style]) {
        if(isCssVariable(style)) {
          domStyle.setProperty(style, value);
        } else {
          domStyle[style] = isNumber(value) ? getNumberStyleValue(style, value) : value;
        }
      }
    }

    for (style in lastAttrValue) {
      if (isNullOrUndef(nextAttrValue[style])) {
        if(isCssVariable(style)) {
          domStyle.removeProperty(style);
        } else {
          domStyle[style] = '';
        }
      }
    }
  } else {
    for (style in nextAttrValue) {
      value = nextAttrValue[style];
        if(isCssVariable(style)) {
          domStyle.setProperty(style, value);
        } else {
          domStyle[style] = isNumber(value) ? getNumberStyleValue(style, value) : value;
        }
    }
  }
}

But the test case will fail, because setProperty is mocked in the test suite :o( The module cssstyle will translate all style keys with dashedToCamelCase so the test will fail. I don't how this could be tested automatically.

Test cases - if cssstyle works correctly with css variables:

//issue-1375.spec.jsx
import { render, Component } from 'inferno';

describe('add css variables to the style object of props', () => {
  let container;

  beforeEach(function() {
    container = document.createElement('div');
    document.body.appendChild(container);
  });

  afterEach(function() {
    render(null, container);
    container.innerHTML = '';
    document.body.removeChild(container);
  });

  it('Should apply the css variable to the cssText - Github #1375', () => {
    let renderCounter = 0;

    class App extends Component {
      render() {
        return (
          <div style={{,'--test': 1}}></div>
        );
      }
    }

    render(<App />, container);

    expect(container.innerHTML).toEqual('<div style="--test:1"></div>');

    const div = container.querySelector('div');
    expect(div.style.cssText).toEqual('--test:1');

  });
});

[Havunen]

Hi @chrkulbe I remember now that we actually had similar issue already last year. We used to do checks for hyphen in patchStyle routine but it has big impact on performance when application uses style as objects. If I remember correctly the issue was resolved by using style as text. In inferno you can write style as plain string and it will then use cssText property. Can you try if that resolves the issue you are having? If so; we could just document it so that css variables are supported through css text.

[ChrisKuBa]

Hi,

I know, that sting based style will change cssText directly. But this is on component level not performant when I use strings all the time (string manuipulation). If I use style objects they must be converted to a string on every render call on each class. :o(

Another idea with an minimum performance impact is to use a reserved key like '--' with a subobject for the variables. If css variables will not be used the perfromance impact will be only one additional if condition per render. I only extend patchstyle function. It's not performance optimized and untested. But an idea.

if (!isNullOrUndef(lastAttrValue) && !isString(lastAttrValue)) {
  for (style in nextAttrValue) {
    // do not add a hasOwnProperty check here, it affects performance
    value = nextAttrValue[style];
    if (value !== lastAttrValue[style) {
      domStyle[style] = isNumber(value) ? getNumberStyleValue(style, value) : value;
    }
  }

  for (style in lastAttrValue) {
    if (isNullOrUndef(nextAttrValue[style])) {
      domStyle[style] = '';
    }
  }

  //extension start 
  if(nextAttrValue['--']) {
    var lastCssVars = lastAttrValue['--'] || {};
    var nextCssVars = nextAttrValue['--'] || {};

    for (style in nextCssVars) {
      value = nextAttrValue[style];
      if (value !== lastAttrValue[style]) {
        domStyle.setAttribute(style, value]);
      }
    } 

    for (style in lastCssVars) {
      if (isNullOrUndef(lastCssVars[style])) {
        domStyle.removeAttribute(style);
      }
    }
  }
  //extension end

} else {
  for (style in nextAttrValue) {
    value = nextAttrValue[style];
    domStyle[style] = isNumber(value) ? getNumberStyleValue(style, value) : value;
  }

  //extension start
  if(nextAttrValue['--']) {
    var nextCssVars = nextAttrValue['--'] || {};
    for (style in nextCssVars) {
      domStyle.setAttribute(style, nextCssVars[style]);
    }
  }
  //extension end
}

[Havunen]

@chrkulbe Did you measure performance difference using string vs object literal? I believe ( didn't test ) setting DOM style property multiple times costs more than string manipulation. However I like this feature request, lets implement it and keep performance as good as possible.

[ChrisKuBa]

@Havunen No I didn't measure it in perfection.

jsperf: style vs cssText vs setAttribute Different runs lead to different winners but all in range of one tenth slower. JQuery performance is bad every time. Tested with latest Chrome and Edge. IE 11: cssText twice faster than style / setProperty FF latest: setAttribute("style", text) is six times faster then style or cssText

cssText and style changes equals in performance at all tested browsers (expect IE) so I think its easier in development to use objects and let inferno merge the diffs between previous and current styles. Should inferno sets styles by cssText or by every key on CSSStyleDeclaration? depends on supporting hyphen or camelCase style by the style object. Because inferno/jsx supports camelCase we should continuing changing the CSSStyleDeclaration.

So keep on going with my suggestion. I will post a tested example on monday.

[trueadm]

Using the Typed CSSOM will be the fastest approach, otherwise strings tend to be faster in real-world applications as they are better on the GC than many object allocations/property accessing. I wouldn't use jsperf benchmarks to compare these sorts of things, they will never be representative of performance in an app's UI.

[Havunen]

We actually recently switched from object literal styles to string text, because there were some customers complaining lag. It improved one of our gantt view performance more than 30%

[Havunen]

In that view there can be hundreds or thousand elements, so it had big impact

[ChrisKuBa]

Interesting.

How do you do that. Configure a style object and join all to a string where the keys are hyphen based and save time to prevent mapping from camelcase to hyphen based style?

Is this an option for inferno core? Only iterate the current style object once and overwrite the cssText (without deleting old style?)

[Havunen]

We actually manually converted all relevant source code to use hyphen styled version strings. I can try to post video or something similar here to show our case. before and after cases

[ChrisKuBa]

@Havunen

Can you try if that resolves the issue you are having? If so; we could just document it so that css variables are supported through css text.

Yes, setting css var through cssText works in generally.

In a perfect world Inferno has an option/flag for a hint that a style object contains only hyphen based keys. Otherwise camelCase will be expected.

Why the camelCase syntax is currently the preferred style when an object is used? Compatibliity to react?

[Havunen]

@chrkulbe yeah its for React compatibility, we should also consider changing it. In my opinion Inferno should take its own path in this kind of things. Supporting CSS variables sounds big enough reason to me. What do you think? We can add transformation to inferno-compat pkg to supporting React way

[ChrisKuBa]

@Havunen I think Inferno can become a precursor :o) Inferno is build for speed. If cssText is up to 30% faster we should support it with a hyphen based style object only. Alternatively setAttribute('style, ...) if this is faster. Css varaiables will become secondary, but supported.

But I dont know if a migration time span is necessary, so other developer can switch slowly if they use the camelcase style in inferno without react. In this case we have a breaking change and force other developer to use react compat layer even if they don't use react.

Suggestion Introduction in version 5.5.0 with a flag to activte it. (global and / or local for migration) Switch default flag to hyphen style in 6.0.0 Breaking change / remove flag in 7.0.0

[ChrisKuBa]

What will be the next step?

• switching to hyphen based style in style object with react compat fallback?
• special key in the style object für css variables?

[Havunen]

@chrkulbe I'm not sure yet; We need to do measurements so we have black on white which is fastest method.

[Havunen]

I think we could switch to hyphen based styles in v6.0.0 and use style.setProperty to me it looks much more web compatible and future proof compared to directly assigning style properties. For inferno-compat we could transform hyphens to camelCase runtime. I will implement this now with test case, unless something unexpected shows up

[Havunen]

I'm closing this ticket as Inferno v6 is now released! :tada:

https://github.com/infernojs/inferno/releases/tag/v6.0.0