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

Ponyfill doesn't work with CSSStylesheet.insertRule #19

Closed
vladbicu opened this issue Aug 8, 2018 · 7 comments
Closed

Ponyfill doesn't work with CSSStylesheet.insertRule #19

vladbicu opened this issue Aug 8, 2018 · 7 comments

Comments

@vladbicu
Copy link

vladbicu commented Aug 8, 2018

Hi,

I'm working on a project that use emotion-css library + CSS vars. Because we had to support IE11, I tried to use this ponyfill. All things were great until we did a production build and the ponyfill didn't work.

After some research I discovered that emotion library use insertRule API, which means that <style> tag is empty and the ponyfill has nothing to parse and replace.

There's someone with this issue? Any ideas?

@jhildenbiddle
Copy link
Owner

jhildenbiddle commented Aug 9, 2018

Hi @vladbicu.

This is a relatively easy fix, but it comes with caveats.

Currently, CSS data is collected from a stylesheet's Node.textContent value. As you've mentioned, this won't be reliable if a stylesheet has been modified using the deleteRule() or insertRule() methods because these modifications aren't reflected in the Node.textContent value.

The fix requires iterating over the CSSRuleList of each stylesheet and concatenating all CSSRule.cssText values into a single string. This works, but there are two things to consider:

  1. Iterating over potentially thousands of CSS rules is significantly slower than simply reading a textContent value. This is especially true with legacy browsers. For example, a Windows 10 VM on a 2009 Mac Pro @ 2.66GHz is able to generate CSS from the CSSRuleList of bootstrap.min.css in ~10ms running Chrome 69 and ~30ms with IE11. I intentionally chose a large block of CSS to illustrate the point: the fix is significantly slower than the ~0ms it takes to read the textContent property.

    The way to mitigate this performance hit is to limit processing to only the CSS that needs to be processed. This can be done using either the include or exclude options. It looks like emotion.js adds a data-emotion attribute to the inserted <style> element, which makes this pretty easy (see below).

  2. The fix will (potentially) produce different CSS output for each browser. This is because browsers can generate different runtime rules from the same CSS input based on how they handle unrecognized and vendor-prefixed properties and values. This is not a big deal functionally speaking, but it is different than how the ponyfill works today and could be an issue if you are (for some reason) trying to use ponyfill-generated CSS created in one browser in a different browser.

    Example CSS:

    body {
     -moz-appearance: none;
     -webkit-appearance: none;
    }
    

    Chrome / Safari CSSRule.cssText:

    body { -webkit-appearance: none; }
    

    Firefox CSSRule.cssText:

    body { -moz-appearance: none; }
    

    Edge / IE CSSRule.cssText:

    body { -moz-appearance: none; -webkit-appearance: none; }
    

With those caveats understood, I think it makes sense to add an option for this scenario. I'm not familiar enough with the various CSS-in-JS libraries to know if this is a common issue, but it seems anyone modifying CSS using the deleteRule() or insertRule() methods would benefit.

The ponyfill will continue to work as it does today by default (reading CSS data from the textContent value), but the new option will allow data to be generated from runtime values. Your final implementation would look something like this:

cssVars({
  // Only process emotion.js <style> nodes
  include: 'style[data-emotion]' // 
  // Get <style> CSS from runtime values
  parseRuntime: true
}) ;

I'll put something together on a separate branch later on tonight. You can give it a test, see how it works, and if all is well I'll publish the changes to npm.

@jhildenbiddle
Copy link
Owner

I've created a runtime branch with the fix described above. To install from this branch on GitHub using NPM:

npm i -D jhildenbiddle/css-vars-ponyfill#runtime

You can view the options.parseRuntime docs in the same branch. Let me know if this addresses your issue and if you have any feedback.

@jhildenbiddle
Copy link
Owner

jhildenbiddle commented Aug 9, 2018

Well, that didn't go as planned.

Turns out the fix doesn't work in legacy browsers. I was testing in the latest version of Chrome which worked fine. Once I ran all automated tests in legacy browers I noticed that the new options.parseRuntime test was failing.

The reason for the failure is due to the browser behavior I described above:

[...] browsers can generate different runtime rules from the same CSS input based on how they handle unrecognized and vendor-prefixed properties and values.

Legacy browsers don't understand CSS custom properties. When a rule is inserted that contains either a custom property declaration or var() function, that declaration or function is dropped from the CSSRule.cssText value. For example:

// Insert two rules into an empty stylesheet (like emotions.js)
styleElm.sheet.insertRule(':root { --color: red; }', 0); // 1
styleElm.sheet.insertRule('p { color: var(--color); }', 0); // 2

// Modern browser CSSRuleList
// 1: ':root { --color: red; }'
// 2: 'p { color: var(--color); }'

// Legacy browser CSSRuleList (e.g. IE)
// 1: ':root {  }'
// 2: 'p {  }'

Given this, I don't think it is possible to have the ponyfill work with stylesheets that use insertRule() to add rules that contain CSS custom properties. Bummer.

One last thought...

You mentioned that everything was working fine until you did a production build. I assume this means emotion.js behaves differently in development mode vs production mode by modifying the <style> elements text content for development but insertRule() for production (it seems that similar libraries also take this approach for performance reasons). Perhaps there is a way to force emotion.js to update CSS using the same process used in development mode even while in production. Ideally, this would be something that you could enable/disable conditionally on the client, which would allow you to enable this behavior only for legacy browsers. If this option isn't available, consider creating an GitHub issue for emotion. It may be a trivial option to add, and it could enable emotion.js to work with other libs that collect CSS data by reading a stylesheet's textContent value.

Hope this helps!

@vladbicu
Copy link
Author

Hi @jhildenbiddle,

Thanks for the effort! Maybe will help some people that use insertRule/deleteRule API.
Emotion library use the .insertRule API in production because it's fast and i don't think they will change that. At this point i have two options:

  1. Drop IE11 support
  2. Replace emotion CSS with something else

@jhildenbiddle
Copy link
Owner

Sorry we couldn't find a solution, @vladbicu. Best of luck with your project and finding a solution for IE11.

@chenyulun
Copy link

Can proxy insetRule method?
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet

// Object.keys(CSSStyleSheet.prototype)
// (9) ['ownerRule', 'cssRules', 'rules', 'addRule', 'deleteRule', 'insertRule', 'removeRule', 'replace', 'replaceSync']
// Some methods are supported only in higher versions and can be done without a proxy
var insertRuleFn = CSSStyleSheet.prototype.insertRule
CSSStyleSheet.prototype.insertRule =function(ruleStr,index) {
// console.log(ruleStr);
var legacyRuleStr = window.cssVarsSync ?  window.cssVarsSync(ruleStr) : ruleStr;
// console.log(legacyRuleStr);
 insertRuleFn.call(this, legacyRuleStr, index) }

@jhildenbiddle
Copy link
Owner

jhildenbiddle commented Mar 17, 2022

@chenyulun --

The challenge with an insertRule proxy (or any CSSOM proxy, for that matter) is that there is no indication of how "ready" the CSS is for processing. You either have to treat each rule as if it is the last and process it using what you know, or find some indicator to serve as a "ready" event like waiting some amount of time since the last CSSOM method call (addRule, deleteRule, insertRule, etc).

If rules are inserted in a very specific order, this is not an issue:

styleElm.sheet.insertRule(':root { --color: red; }', 0);
styleElm.sheet.insertRule('p { color: var(--color); }', 0);
p { color: red; }

But if rules are inserted out of order...

styleElm.sheet.insertRule('p { color: var(--color); }', 0);
styleElm.sheet.insertRule(':root { --color: red; }', 0);

... or override previous values...

styleElm.sheet.insertRule(':root { --color: red; }', 0);
styleElm.sheet.insertRule('p { color: var(--color); }', 0);
styleElm.sheet.insertRule(':root { --color: blue; }', 0);

... or invalidate previous values ...

styleElm.sheet.insertRule(':root { --color: red; }', 0);
styleElm.sheet.insertRule(':root { --color: blue; }', 0);
styleElm.sheet.insertRule('p { color: var(--color); }', 0);
styleElm.sheet.deleteRule(1);

... things get complicated very quickly.

The ponyfill doesn't suffer from these problems because it doesn't begin its work until the DOMContentLoaded event fires. This is a "good enough" signal that all CSS is ready for processing:

  1. Get all CSS data from <link> and <style> elements
  2. Parse CSS data
  3. Store all custom property values
  4. Replace all var() functions with custom property values
  5. Inject <style> elements with transformed CSS after each <link> and <style> element processed

Could CSSOM method proxies work? Possibly, but it would be a significant amount of work and has the likely potential to perform poorly if/when previously processed rules need to be re-processed (see examples above). If someone were motivated, they could attempt it using the individual libraries in the src directory of the repo.

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