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

[jss-vendor-prefixer] Naive use of content prop causes uncaught error in IE #242

Closed
mdarens opened this issue Jun 13, 2016 · 21 comments
Closed
Labels
bug It went crazy and killed everyone.

Comments

@mdarens
Copy link
Member

mdarens commented Jun 13, 2016

When using the content prop (in this case for the usual task of styling pseudo-elements), if escaped quote characters are not passed into the string given as the value, the value ends up getting passed without quotes.

While Chrome and Edge pass it through and simply ignore the invalid value, in IE11, css-vendor/supported-value.js throws an error:

[Main Thread]
supportedValue [Line: 41, Col: 1], supported-value.js
Anonymous function [Line: 37, Col: 1], index.js
run [Line: 49, Col: 1], PluginsRegistry.js
createRule [Line: 213, Col: 1], StyleSheet.js
Anonymous function [Line: 37, Col: 1], index.js
run [Line: 49, Col: 1], PluginsRegistry.js
createRule [Line: 213, Col: 1], StyleSheet.js
StyleSheet [Line: 60, Col: 1], StyleSheet.js
createStyleSheet [Line: 79, Col: 1], Jss.js
attach [Line: 38, Col: 1], index.js
ref [Line: 47, Col: 1], index.js
componentWillMount [Line: 71, Col: 1], index.js
ReactCompositeComponentMixin.mountComponent [Line: 210, Col: 1], ReactCompositeComponent.js
wrapper [Line: 66, Col: 1], ReactPerf.js
ReactReconciler.mountComponent [Line: 37, Col: 1], ReactReconciler.js
ReactCompositeComponentMixin.mountComponent [Line: 225, Col: 1], ReactCompositeComponent.js
wrapper [Line: 66, Col: 1], ReactPerf.js
ReactReconciler.mountComponent [Line: 37, Col: 1], ReactReconciler.js
mountComponentIntoNode [Line: 266, Col: 1], ReactMount.js
Mixin.perform [Line: 136, Col: 1], Transaction.js
batchedMountComponentIntoNode [Line: 282, Col: 1], ReactMount.js
Mixin.perform [Line: 136, Col: 1], Transaction.js
ReactDefaultBatchingStrategy.batchedUpdates [Line: 62, Col: 1], ReactDefaultBatchingStrategy.js
batchedUpdates [Line: 94, Col: 1], ReactUpdates.js
ReactMount._renderNewRootComponent [Line: 476, Col: 1], ReactMount.js
wrapper [Line: 66, Col: 1], ReactPerf.js
ReactMount._renderSubtreeIntoContainer [Line: 550, Col: 1], ReactMount.js
ReactMount.render [Line: 570, Col: 1], ReactMount.js
wrapper [Line: 66, Col: 1], ReactPerf.js
Anonymous function [Line: 9, Col: 1], index.jsx
s [Line: 1, Col: 1], _prelude.js
e [Line: 1, Col: 1], _prelude.js
Anonymous function [Line: 1, Col: 1], _prelude.js
Anonymous function [Line: 1, Col: 1], _prelude.js
Global code [Line: 1, Col: 1], _prelude.js

When jss-vendor-prefixer is disabled, IE behavior is identical to Chrome/Edge.

@kof kof changed the title [jss-vender-prefixer] Naive use of content prop causes uncaught error in IE [jss-vendor-prefixer] Naive use of content prop causes uncaught error in IE Jun 13, 2016
@kof kof added the bug It went crazy and killed everyone. label Jun 13, 2016
@kof
Copy link
Member

kof commented Jun 13, 2016

Can you give me an example of jss code you are using?

@kof
Copy link
Member

kof commented Jun 13, 2016

Is this enough to reproduce?

{
  button: {
    '&:after': {
      content: ''
     }
  }
}

@kof
Copy link
Member

kof commented Jun 13, 2016

I was always wondering whether jss should just simply check the " in the beginning, and if it is not there, add those quotes around automatically.

@mdarens
Copy link
Member Author

mdarens commented Jun 13, 2016

Here is the code I am using to reproduce it now:

index.jsx:

import React from "react";
import ReactDOM from "react-dom";

import Root from "./root";

ReactDOM.render((
        <Root />
), document.getElementById("exampleApp"));

root.jsx:

import React from "react";
import jss from "./jss-preset";
import useSheet from "react-jss";

const styles = {
    foo: {
        "&:after": {
            display: "block",           
            content: "bar"
        }
    }
};

const Root = ({
    sheet: { classes }
}) => {
    return <h1 className={classes.foo}>Hello World</h1>
};

export default useSheet(Root, styles);

jss-preset.js:

import jss from "jss";
import jssExtend from "jss-extend";
import jssNested from "jss-nested";
import jssCamelCase from "jss-camel-case";
import jssDefaultUnit from "jss-default-unit";
import jssVendorPrefixer from "jss-vendor-prefixer";

jss.use(jssExtend());
jss.use(jssNested());
jss.use(jssCamelCase());
jss.use(jssDefaultUnit());
jss.use(jssVendorPrefixer());

export default jss;

The nesting/pseudo-class is not necessary to reproduce it, just used as it reflects the typical usage pattern. Other properties on the nested selector work fine, and using content without the escaped quotes directly on the className key reproduces the error as well.

@mdarens
Copy link
Member Author

mdarens commented Jun 13, 2016

@kof
Copy link
Member

kof commented Jun 13, 2016

Oh now I recall again why I didn't auto insert them. This would break other cases. There are values for content that can be put there without quotes. https://developer.mozilla.org/de/docs/Web/CSS/content

Barium is broken by design.

@kof
Copy link
Member

kof commented Jun 13, 2016

The only thing I can optimize there is when content value is an empty string, in this case I can auto insert double quotes.

Now back to the original issue. I am really surprised that IE throws a js error in such case.

@mdarens
Copy link
Member Author

mdarens commented Jun 14, 2016

Perhaps a warning when the environment is debug to make sure content will produce a valid value, and/or some validation:

  • one ofinherit, initial, none, normal, unset
  • contains () (for url,counter,attr)
  • contains \ (for escapes)
  • contains quote

@kof
Copy link
Member

kof commented Jun 14, 2016

I think someone who is writing it, will test and see if it doesn't work?

@kof
Copy link
Member

kof commented Jun 14, 2016

I mean it won't work in any browser, just won't throw error in others than IE.

kof added a commit to cssinjs/css-vendor that referenced this issue Jun 14, 2016
@kof
Copy link
Member

kof commented Jun 14, 2016

please confirm that the error in IE has been fixed.

@kof
Copy link
Member

kof commented Jun 14, 2016

you need to remove css-vendor package and install a new version or simply remove everything and install again.

@kof kof closed this as completed Jun 14, 2016
@mdarens
Copy link
Member Author

mdarens commented Jun 15, 2016

Verified fixed in IE11, thanks! Interestingly enough, while trying different values, I was still able to get IE to throw an exception when surrounding single quotes with double (e.g. { content: "'bar'" } as IE gives a different value to the prefixless comparison. IE escapes the quotes when accessing the style from the DOM, so el.style[property] is "\"bar\"", whereas Chome returns it as it was passed in and results in a valid value.

@kof
Copy link
Member

kof commented Jun 16, 2016

Interesting ... wonder what we are currently generating on IE ... never had a problem, so it must be working somehow.

Gopal2bn pushed a commit to Gopal2bn/coreui1 that referenced this issue Jul 18, 2016
@oliviertassinari
Copy link
Contributor

oliviertassinari commented Nov 19, 2016

I have got a similar issue with this line content: '\'\'',.
IE11 is throwing right here.
Using content: '""', solves the issue.

@kof
Copy link
Member

kof commented Nov 20, 2016

Hmm, do we need to fix this?

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Nov 20, 2016

Hmm, do we need to fix this?

Your call, I have a white screen when using content: "''", with no warnings.
My previous comment is for anyone facing the same issue, so they get a quick workaround.

@kof
Copy link
Member

kof commented Nov 20, 2016

It is fixable in theory, but a bit too much hustle considered this spec: https://developer.mozilla.org/en-US/docs/Web/CSS/content

@TerrySlack
Copy link

I wish this was fixed. I just spent a full day wondering why my css was not working. I wrote normal css and inserted it into a style tag and my css worked. But using the content: "" and trying content: '', after compilation, the content attribute was removed from my class by React-jss. I have no idea why, but wanted to give you a heads up. I'm using Firefox 70 FYI, but it's the compilation process in jss, that seems to remove this. Using the fix mentioned above content: "''" solved the issue.

@kof
Copy link
Member

kof commented Nov 4, 2019

@TerrySlack it's not broken, it's just not intuitive, content can contain values without quotes. The only thing I know we can do is to provide warnings in dev mode.

@TerrySlack
Copy link

TerrySlack commented Nov 4, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It went crazy and killed everyone.
Projects
None yet
Development

No branches or pull requests

4 participants