postcss / postcss-safe-parser

Fault tolerant CSS parser for PostCSS
MIT License
120 stars 12 forks source link

`*color:red` : the `*` ends up in `raws.before` #4

Closed pygy closed 8 years ago

pygy commented 8 years ago

Hello fine folks, in the following code, I'd expect the asterisk to be part of the property name, but it isn't...

var root = postcss().process('*color:red;', {parser: require('postcss-safe-parser'}).root
root.nodes[0].prop === '*color' // false, it is just 'color'
root.nodes[0].raws.before === '*' // true

What gives?

The README states that the examples present in browserhacks.com should parse fine, but the parser treats *color and e.g. %color differently, even though they are part of the same hack.

ai commented 8 years ago

*-hack do not change property name. *color will be parsed in IE like color. This is why PostCSS move * to .raws.before, so Autoprefixer and any other PostCSS plugins will work with this property without any special checks.

ai commented 8 years ago

I add only * case to parser, because I never saw % and other symbols. If we will check only for * symbol, it will make our parser much faster.

pygy commented 8 years ago

My use case is a CSS to j2c converter, where property hacks need special treatment, but they need to be preserved, obviously...

This family of hacks applies to IE <= 7, I didn't even know these browsers supported any kind of prefix. Are you sure it is relevant? *foo will be ignored everywhere else...

BTW, here's how PostCSS handles the full list:

"! $ & * ( ) = % + @ , . / ` [ ] # ~ ? : < > |".split(' ').map(function (c) {
    return  c + ' ' + p().process('a{' + c + 'w:w;}',
        {parser: require('postcss-safe-parser')}).root.nodes[0].nodes[0].prop
})
[ '! !w',
  '$ $w',
  '& &w',
  '* w',
  '( w',
  ') w',
  '= =w',
  '% %w',
  '+ +w',
  '@ undefined',
  ', ,w',
  '. .w',
  '/ /w',
  '` `w',
  '[ [w',
  '] ]w',
  '# #w',
  '~ ~w',
  '? ?w',
  ': w',
  '< <w',
  '> >w',
  '| |w' ]

I added the a{} wrapper because I wanted to be sure the @ undefined wasn't due to a lack of context.

So, beside *, (, ), and : are not treated as part of the property name.

I could work around the problem by checking for a / *[\*\(\):]$/ match in raws.before, so, if you don't plan to fix this, it's not a big deal...

Edit: actually, /(?:^|\s+)([\*\(\):])$/ reliably extracts the missing character.

ai commented 8 years ago

BTW, do you see postcss-js. It can be useful for CSS-in-JS solutions.

ai commented 8 years ago

where property hacks need special treatment, but they need to be preserved, obviously...

You need hasck in prop for your case, but many others cases will need different behaviour.

This code will help:

if ( node.raws.before.slice(-1) === '*' ) {
    prop = '*' + node.prop;
}
pygy commented 8 years ago

These hacks are in practice conditional comments.

PostCSS currently leaves most of them alone, which is fine, but treats some of them as unconditional non-comments, which is wrong. You may want to document that inconsistent behavior somewhere, and stop claiming that you support the full browserhacks set, since @zoom: 1; causes a misparse.

You talk about "many other cases", could you point to some?

ai commented 8 years ago

many other cases

https://github.com/postcss/postcss-parser-tests/blob/master/cases/no-selector.css https://github.com/postcss/postcss-parser-tests/blob/master/cases/escape.css

stop claiming that you support the full browserhacks set

I told only that we can parse that CSS, not that we will works properly with them ;)

ai commented 8 years ago

These hacks are in practice conditional comments.

Yeap, this is why PostCSS supports * and move it to raws.before, because it is necessary for hack support. If * will be in property, many PostCSS plugins will not work properly with this escaped property.

ai commented 8 years ago

If you have any real CSS with @ hack we can add it to parser. It is not so complicated.

ben-eb commented 8 years ago

If it helps, I wrote this for leading character hacks:

https://github.com/ben-eb/stylehacks/blob/master/src/plugins/leadingStar.js

pygy commented 8 years ago

@ai I don't understand how the no-selectors and escape test cases are relevant, but since you do, I suppose I'm missing some broader context that I don't have the time to investigate.

I don't have any real code that relies on the @, no... I'm bitching a bit here, but I must say that, overall, PostCSS is the best CSS parser I found, and I thank you for writing it. I was surprised when I found out that it didn't match my expectations (as you said, the docs isn't completely honest re. browserhacks), but I'm still very happy to use it as is, especially since my issue can be worked around.

Is the raws.before field guaranteed to only contain white space (in the broadest sense) and one of *, (, ), and :?

@ben-eb, thanks, I was thinking of writing a plugin that puts back the problem characters in the prop name so that I don't have to worry about it in client code... I'll use yours as a baseline.

ai commented 8 years ago

@pygy Thanks =^_^=

Safe Parser move all unknown symbols to raws.before. ut right now seems like in will be only *, (, ) and :.

pygy commented 8 years ago

I'm adding this for the sake of completeness for anyone reading this later on: _ is also put in raws.before, but a leading - isn't, which makes sense in the age of vendor prefixes. \9 is kept as part of the value.

All of this as of PostCSS v5.0.13 / SafeParser v1.0.4.

ai commented 8 years ago

We can add \9 to raws.before too, if it is used somewhere.

Or, for example, we can change Safe Parser and move to raws.before any non-alphabet first symbol. Safe Parser is not so critical as standard parser.

pygy commented 8 years ago

\9 is actually tacked after values foo:bar\9... As far as I'm concerned, things can stay the way they are... If they were to change, a semver major bump would be nice.