Fix bug where styles have their media queries removed on IE #238
Conversation
|
Hey @reklawnos, Thanks for the PR! Mind signing our Contributor License Agreement? When you've done so, go ahead and comment Yours truly, |
466b29f to
6fb6555
Compare
|
[clabot:check] |
|
CLA signature looks good 👍 |
src/inject.js
Outdated
| @@ -47,7 +47,7 @@ const injectStyleTag = (cssContents /* : string */) => { | |||
|
|
|||
| if (styleTag.styleSheet) { | |||
| // $FlowFixMe: legacy Internet Explorer compatibility | |||
| styleTag.styleSheet.cssText += cssContents; | |||
| styleTag.innerText += cssContents; | |||
There was a problem hiding this comment.
Did you do any profiling to see what the performance impact this might be?
Also, I wonder if we should be using insertRule() instead?
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule
In IE 9, appending two styles with media queries to a `StyleSheet`'s
`cssText` property results in the first style's media query being
removed and that first style will always apply without the query.
For example:
```
var styleTag = document.createElement('style');
document.body.appendChild(styleTag);
styleTag.styleSheet.cssText +=
'@media (min-width: 500px) { body { background-color: blue; } }';
styleTag.styleSheet.cssText +=
'@media (min-width: 700px) { body { background-color: red; } }';
```
Causes the body element to be red for window widths ≥ 700px, but it is
blue otherwise, even at widths < 500px.
This change adds a `styleString` variable that keeps track of the
contents of the current `styleTag`, and updates the `cssText` content
with `styleString` instead of appending to it with `+=`.
6fb6555 to
1e18722
Compare
|
@lencioni I changed the approach a bit so that we're still using I found that There's a fair amount of work that'd need to be done to get |
| // $FlowFixMe: legacy Internet Explorer compatibility | ||
| styleTag.styleSheet.cssText += cssContents; | ||
| styleTag.styleSheet.cssText = styleString; |
There was a problem hiding this comment.
I wonder if you could avoid managing styleString by doing this instead?
styleTag.styleSheet.cssText = styleTag.innerText + cssContents;With your current approach I mostly worry about something else modifying the stylesheet and then that getting blown away (in addition to the extra bookkeeping complexity). Doesn't seem super likely, but if we can trivially avoid it might as well, right?
Need to see if that is still okay for performance.
There was a problem hiding this comment.
I agree that we should avoid having to keep track of styleString if possible, but that solution won't quite work because innerText doesn't get updated when cssText is altered.
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.
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.
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.
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 #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.
|
Closing, as I believe this was fixed by #240. |
In IE 9, appending two styles with media queries to a
StyleSheet'scssTextproperty results in the first style's media query being removed and that first style will always apply without the query.For example:
Causes the body element to be red for window widths ≥ 700px, but it is blue otherwise, even at widths < 500px.
This change adds a
styleStringvariable that keeps track of the contents of the currentstyleTag, and updates thecssTextcontent withstyleStringinstead of appending to it with+=.Here's a JS Bin that demonstrates the issue, which includes a commented-out solution. Note that the JS Bin won't work outside of IE. If you don't want to open up IE you can take a look at these gifs, recorded on IE 9:
Using
styleSheet.cssTextUsing
innerText