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

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

Closed
chrkulbe opened this Issue Aug 16, 2018 · 18 comments

Comments

Projects
None yet
3 participants
@chrkulbe

chrkulbe commented Aug 16, 2018

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

This comment has been minimized.

Member

Havunen commented Aug 16, 2018

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?

@chrkulbe

This comment has been minimized.

chrkulbe commented Aug 17, 2018

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

This comment has been minimized.

Member

Havunen commented Aug 18, 2018

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.

@chrkulbe

This comment has been minimized.

chrkulbe commented Aug 18, 2018

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

This comment has been minimized.

Member

Havunen commented Aug 18, 2018

@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.

@chrkulbe

This comment has been minimized.

chrkulbe commented Aug 18, 2018

@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

This comment has been minimized.

Collaborator

trueadm commented Aug 18, 2018

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

This comment has been minimized.

Member

Havunen commented Aug 18, 2018

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

This comment has been minimized.

Member

Havunen commented Aug 18, 2018

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

@chrkulbe

This comment has been minimized.

chrkulbe commented Aug 18, 2018

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

This comment has been minimized.

Member

Havunen commented Aug 18, 2018

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

@chrkulbe

This comment has been minimized.

chrkulbe commented Aug 18, 2018

@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

This comment has been minimized.

Member

Havunen commented Aug 18, 2018

@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

@chrkulbe

This comment has been minimized.

chrkulbe commented Aug 19, 2018

@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

@chrkulbe

This comment has been minimized.

chrkulbe commented Aug 20, 2018

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

This comment has been minimized.

Member

Havunen commented Aug 20, 2018

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

@Havunen

This comment has been minimized.

Member

Havunen commented Sep 2, 2018

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 added a commit that referenced this issue Sep 2, 2018

Move to hyphen-case styles from camelCase, this adds support for inli…
…ne CSS variables and removed white-list of properties where "px" suffix was added. Fixes Github #1375
@Havunen

This comment has been minimized.

Member

Havunen commented Oct 13, 2018

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

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

@Havunen Havunen closed this Oct 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment