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.

Sign up for GitHub

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/inject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ type ProcessedStyleDefinitions = {
// faster.
let styleTag = null;

// The current CSS contents of the style tag. This is an optimization for IE,
// as using `styleTag.styleSheet.cssText` is significantly faster than altering
// the text content of the `styleTag`, but `cssText` doesn't retain media queries,
// causing them to be blown away by subsequent additions.
let styleString /* : string */ = '';

// Inject a string of styles into a <style> tag in the head of the document. This
// will automatically create a style tag and then continue to use it for
// multiple injections. It will also use a style tag with the `data-aphrodite`
Expand All @@ -41,13 +47,17 @@ const injectStyleTag = (cssContents /* : string */) => {
styleTag.type = 'text/css';
styleTag.setAttribute("data-aphrodite", "");
head.appendChild(styleTag);
} else if (styleTag.styleSheet) {
// $FlowFixMe: legacy Internet Explorer compatibility
styleString = styleTag.innerText;
}
}


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

@lencioni lencioni May 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
styleTag.appendChild(document.createTextNode(cssContents));
}
Expand Down Expand Up @@ -190,6 +200,7 @@ export const reset = () => {
alreadyInjected = {};
isBuffering = false;
styleTag = null;
styleString = '';
};

export const startBuffering = () => {
Expand Down
18 changes: 18 additions & 0 deletions tests/inject_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,24 @@ describe('injection', () => {
});
});

it('properly adds multiple styles to .styleSheet.cssText if available', done => {
const styleTag = global.document.createElement("style");
styleTag.setAttribute("data-aphrodite", "");
document.head.appendChild(styleTag);
styleTag.styleSheet = { cssText: "" };

injectStyleOnce("x", ".x", [{ color: "red" }], false);
injectStyleOnce("y", ".y", [{ color: "blue" }], false);

asap(() => {
assert.include(styleTag.styleSheet.cssText, ".x{");
assert.include(styleTag.styleSheet.cssText, "color:red");
assert.include(styleTag.styleSheet.cssText, ".y{");
assert.include(styleTag.styleSheet.cssText, "color:blue");
done();
});
});

it('uses document.getElementsByTagName without document.head', done => {
Object.defineProperty(global.document, "head", {
value: null,
Expand Down