Use `insertRule` to inject styles by reklawnos · Pull Request #240 · Khan/aphrodite

reklawnos · 2017-05-12T18:33:45Z

Instead of appending a new text node to a style tag or appending to the cssText property of the stylesheet (which broke media queries in IE as documented in #238), use insertRule to add CSS rules to the style element.

This change requires styles to be generated as an array of strings with one rule per string, rather than a single large string that contains multiple rules.

This was (at least provisionally) implemented in #83 and #157 and discussed in #76, but it didn't seem like those were going anywhere.

khanbot · 2017-05-12T18:33:48Z

CLA signature looks good 👍

lencioni · 2017-05-13T17:37:09Z

        // Generate the styles normally, and then wrap them in the media query.
        const generated = generateSubtreeStyles(baseSelector);
-        return `${selector}{${generated}}`;
+        return [`${selector}{${generated}}`];


I wonder if we actually need to create a new array here or if we can continue returning a string and handle it properly elsewhere. Also, do we need to change pseudoSelectors above to also return an array?

generateSubtreeStyles should have the same return type as generateCSS so pseudoSelectors should already return an array. Would it make sense to make the Flow type for generateSubtreeStyles more specific?

We could have generateCSS handle a string being returned here, but we never want a selector handler to return strings containing multiple rules, as insertRule expects a single rule to be passed. My thought here was that selector handlers should always return an array of strings with one rule per string.

This would be a breaking change to the current API, so perhaps we could, say, wrap strings returned by a selector handler in a @media all {...} query and warn about that return type being deprecated. Or just bump by a major version.

Ah, I see. The alternative would be to do something like this on line 173:

if (Array.isArray(result)) { generatedStyles.push(...result); } else { generatedStyles.push(result); }

I think making the breaking change is okay. @xymostech do you have any thoughts on this?

Also, yes I think it would be good to improve the flow types where we can.

Hmmm. I think the funky @media all{} with a warning solution sounds the best to me, but I'm not that attached to it. I don't know how many people are using extensions right now, but I'd prefer if we didn't break it already?

(and as a ping, the psuedoSelectors function above needs to be changed too, right?)

lencioni · 2017-05-13T17:42:33Z

+    if (sheet.insertRule) {
+        cssContents.forEach((rule) => {
+            try {
+                sheet.insertRule(rule, sheet.cssRules.length);


It might be worth moving this out into a variable that we increment, to avoid having to read this value every time.

let numberOfRules = sheet.cssRules.length; cssContents.forEach((rule) => { try { sheet.insertRule(rule, numberOfRules); numberOfRules += 1; } catch(e) { ... } });

lencioni · 2017-05-13T17:42:46Z

-        // $FlowFixMe: legacy Internet Explorer compatibility
-        styleTag.styleSheet.cssText += cssContents;
+    if (sheet.insertRule) {
+        cssContents.forEach((rule) => {


I wonder if we should make this a regular for loop for perf

Will check how this affects performance.

Ran both versions through your benchmark several times. There was no noticeable difference between the forEach version and the for loop version, but a small speedup when storing sheet.cssRules.length in a variable.

xymostech

Ahh! Thank you so much for doing this! I've been meaning to get this out for a while but haven't had the time!

I left a bunch of comments about things to look at! Overall though, this change looks great. :)

xymostech · 2017-05-15T23:11:49Z

+        const currentStyleTag = styleTag;
+        cssContents.forEach((rule) => {
+            currentStyleTag.innerText = (currentStyleTag.innerText || '') + rule;
+        });


We probably want to batch update this, like currentStyleTag.innerText = (currentStyleTag.innerText || '') + cssContents.join('');.

xymostech · 2017-05-15T23:12:59Z

 // `document.querySelector("style[data-aphrodite"])`, but holding onto it is
 // faster.
-let styleTag = null;
+let styleTag /* : ?HTMLStyleElement */ = null;


xymostech · 2017-05-15T23:15:19Z


-    if (styleTag.styleSheet) {
-        // $FlowFixMe: legacy Internet Explorer compatibility
-        styleTag.styleSheet.cssText += cssContents;


Could you test this change in IE? Looks like this "IE compatibility" branch got changed.

insertRule is supported on ≥ IE 9. I tested it on IE 9 and AFAICT it worked just fine. What browsers does Aphrodite guarantee support for? The innerText fallback should work for browsers without insertRule, but I just want to get idea of how far back I should test this.

Testing in IE 9 sounds good enough to me! Thanks!

xymostech · 2017-05-15T23:16:34Z


 // This is the buffer of styles which have not yet been flushed.
-let injectionBuffer = "";
+let injectionBuffer = [];


Could you give this a : string[] type to help flow make sure that these strings can't be null?

ping on this?

xymostech · 2017-05-15T23:19:28Z

        // Generate the styles normally, and then wrap them in the media query.
        const generated = generateSubtreeStyles(baseSelector);
-        return `${selector}{${generated}}`;
+        return [`${selector}{${generated}}`];


Hmmm. I think the funky @media all{} with a warning solution sounds the best to me, but I'm not that attached to it. I don't know how many people are using extensions right now, but I'd prefer if we didn't break it already?

(and as a ping, the psuedoSelectors function above needs to be changed too, right?)

xymostech · 2017-05-15T23:20:40Z

 // tag on it if that exists in the DOM. This could be used for e.g. reusing the
 // same style tag that server-side rendering inserts.
-const injectStyleTag = (cssContents /* : string */) => {
+const injectStyleTag = (cssContents /* : string[] */) => {


Could this argument name and docstring be updated?

reklawnos · 2017-05-16T23:15:00Z

@lencioni @xymostech PTAL, addressed comments and updated to use the @media all solution I suggested.

xymostech

This looks great to me! I'll let @lencioni do the final approve and merge to make sure that their concerns were addressed.

xymostech · 2017-05-16T23:33:12Z


 // This is the buffer of styles which have not yet been flushed.
-let injectionBuffer = "";
+let injectionBuffer = [];


ping on this?

xymostech · 2017-05-16T23:34:42Z

+            '^': {
+                color: 'red',
+            },
+        }], ['@media all {.foo::before{color:red;} .foo::after{color:red;}}'], [handler], {}, false);


lencioni · 2017-05-18T04:23:28Z

I don't think I'll be able to look for a while so don't hold it up for me.

…

On Tue, May 16, 2017, 4:41 PM Emily Eisenberg ***@***.***> wrote: ***@***.**** approved this pull request. This looks great to me! I'll let @lencioni <https://github.com/lencioni> do the final approve and merge to make sure that their concerns were addressed. ------------------------------ In src/inject.js <#240 (comment)>: > @@ -137,7 +146,7 @@ const stringHandlers = { let alreadyInjected = {}; // This is the buffer of styles which have not yet been flushed. -let injectionBuffer = ""; +let injectionBuffer = []; ping on this? ------------------------------ In tests/generate_test.js <#240 (comment)>: > + + it('supports selector handlers that return strings containing multiple rules', () => { + const handler = (selector, baseSelector, callback) => { + if (selector[0] !== '^') { + return null; + } + const generatedBefore = callback(baseSelector + '::before'); + const generatedAfter = callback(baseSelector + '::after'); + return `${generatedBefore} ${generatedAfter}`; + }; + + assertCSS('.foo', [{ + '^': { + color: 'red', + }, + }], ***@***.*** all {.foo::before{color:red;} .foo::after{color:red;}}'], [handler], {}, false); <3 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#240 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAL7zpm2o3F5qLNsuPJqkiHkHsSlXXzhks5r6jQOgaJpZM4NZoqT> .

Trolleymusic · 2017-07-25T10:47:40Z

Hey @reklawnos @xymostech, any plans to merge this soon?

Instead of appeding a new text node to a style tag or appending to the `cssText` property of the stylesheet (which broke media queries in IE as documented in Khan#238), use `insertRule` to add CSS rules to the `style` element. This change requires styles to be generated as an array of strings with one rule per string, rather than a single large string that contains multiple rules.

lencioni · 2018-01-31T21:29:41Z

-            assertStylesInclude('0%{opacity:0;-webkit-transform:scale(0.75) translate3d(1px, 2px, 0);-ms-transform:scale(0.75) translate3d(1px, 2px, 0);transform:scale(0.75) translate3d(1px, 2px, 0);}');
-            assertStylesInclude('100%{opacity:1;-webkit-transform:scale(1) translate3d(1px, 2px, 0);-ms-transform:scale(1) translate3d(1px, 2px, 0);transform:scale(1) translate3d(1px, 2px, 0);}');
-            assertStylesInclude('animation-name:keyframe_d35t13');
+            assertStylesInclude('0% {opacity: 0; -webkit-transform: scale(0.75) translate3d(1px,2px,0); -ms-transform: scale(0.75) translate3d(1px,2px,0); transform: scale(0.75) translate3d(1px,2px,0);}');


Looks like some of the spaces you removed here are still there in the test

lencioni · 2018-01-31T21:36:08Z

I think this looks good. There is one test that has a minor failure on it. I'll follow up with a fix for that after merging this. Thanks for your work on this!

lencioni · 2018-01-31T22:01:37Z

+                        'Returning a string containing multiple rules is deprecated.',
+                        handler,
+                    );
+                    generatedStyles.push(`@media all {${result}}`);


I think this may be a semver major change, since it could theoretically cause some styles to stop working in browsers that don't support media queries. For IE, this applies to IE8 and older.

Practically speaking, however, I would be surprised if this actually breaks anything for anyone, so I am tempted to go with semver minor.

What do you think @xymostech?

Just my 2c, but if I understand correctly, if someone was not using media queries before this change, and needed to support IE8 for some reason, then it seems like this should likely be considered a breaking change, as all styles would no longer work?

Is there a concern that bumping semver major would cause an unnecessary burden to consumers?

Yeah I think you are correct. My main concern is that I don't want to do a major release if there is no practical benefit to it, but I guess there's no way to know so probably best to stay on the safe side.

Wondering if opening a new PR to bump the version of inline-style-prefixer https://github.com/rofrischmann/inline-style-prefixer/blob/master/Changelog.md, which contains a few bug fixes and reported 30% performance improvement since , along with noting some potential internal perf benefits would be of practical benefit?

Yes that sounds great to me!

lencioni reviewed May 13, 2017

View reviewed changes

xymostech reviewed May 15, 2017

View reviewed changes

xymostech approved these changes May 16, 2017

View reviewed changes

Trolleymusic mentioned this pull request Jul 25, 2017

Temp fix for nav double animation nudj/web#58

Merged

reklawnos added 7 commits January 30, 2018 14:40

Cache sheet.cssRules.length

3805992

Use @media all {...} to combine multiple rules into one

2b736a1

Rename cssContents -> cssRules

1cab4d2

Batch update innerText

60ef3bf

Add Flow type to injectionBuffer

f39889b

Fix broken tests

5606f41

reklawnos force-pushed the use-insert-rule branch from 10b4772 to 5606f41 Compare January 30, 2018 22:56

lencioni reviewed Jan 31, 2018

View reviewed changes

lencioni merged commit d3a0fbf into Khan:master Jan 31, 2018

lencioni reviewed Jan 31, 2018

View reviewed changes

lencioni mentioned this pull request Jan 31, 2018

Fix bug where styles have their media queries removed on IE #238

Closed

lencioni added the semver-major: breaking change label Feb 5, 2018

lencioni mentioned this pull request Feb 5, 2018

Build with rollup instead of webpack #281

Merged

This was referenced Mar 16, 2018

Bump aphrodite from 1.2.5 to 2.1.0 in /frontend uclapi/uclapi#385

Closed

Bump aphrodite from 1.2.5 to 2.1.0 tmhn/react#89

Closed

ghost mentioned this pull request Mar 19, 2018

Update aphrodite in / from 1.2.5 to 2.1.0 severest/retrobot#67

Merged

mbensch mentioned this pull request Jul 30, 2018

Edit generated styles in dev tools #314

Closed

Conversation

reklawnos commented May 12, 2017

Uh oh!

khanbot commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xymostech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reklawnos commented May 16, 2017

Uh oh!

xymostech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lencioni commented May 18, 2017 via email

Uh oh!

Trolleymusic commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lencioni commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants