search

cssinjs / css-vendor

Runtime vendor prefixing based on feature detection.
MIT License
67 stars 24 forks source link

add supportedSelector with at-rule support #1

Closed pstoica closed 9 years ago

pstoica commented 9 years ago

This is in support of the first item at jsstyles/jss-vendor-prefixer#1

A corresponding PR for that library will follow that will update the selector using the result from supportedSelector.

pstoica commented 9 years ago

https://github.com/Modernizr/Modernizr/pull/1132#issuecomment-49923209

This may not be the best solution? Additionally, @-moz-document should work, but DOCUMENT_RULE isn't present.

Some exceptions may need to be made.

kof commented 9 years ago

nice work @pstoica

kof commented 9 years ago

Some questions though, please don't take it the wrong way, I just try to maintain this tiny code base in the highest possibly quality ...

pstoica commented 9 years ago

It's all good! I'll adjust according to feedback later.

Regarding using window.CSSRule, I started out mapping at-rules to similar properties, did some research, and found this repo (also adapted the code in this PR since it's public domain): https://github.com/ryanmorr/is-rule-supported

Modernizr is also doing something similar. The problem is, it's not 100% accurate (IE problems, Mozilla supports @-moz-document but has no document rule). And like you said, we already know the prefix. Should this stick to CSSRule and add any other conditions/exceptions, or would you prefer something else?

kof commented 9 years ago

Regarding using window.CSSRule, I started out mapping at-rules to similar properties, did some research, and found this repo (also adapted the code in this PR since it's public domain): https://github.com/ryanmorr/is-rule-supported

yeah saw it too.

Modernizr is also doing something similar. The problem is, it's not 100% accurate (IE problems, Mozilla supports @-moz-document but has no document rule).

yeah I know :)

Which problems except of @document do we know? I think @document is not an issue for any of us :)

Should this stick to CSSRule and add any other conditions/exceptions,

I would stick to an element if it works. If there are issues, then it might be good to know them and reference them as a comment, otherwise I would use element just for the sake of consistency ... basically it doesn't matter. Another thing is CSSRule is implemented in IE since version 9, so element has a higher versions range ...

pstoica commented 9 years ago

Yeah, so I figured @keyframes is really all we care about.. @document would only go to @-moz-document or be removed if we really care about supporting it.

I think @keyframes can just go off of -[prefix]-animation-name in an element's style. Should we just leave other at-rules untouched?

One more question: does jss-vendor-prefixer leave already prefixed props/values alone? Do I need to account for that here?

kof commented 9 years ago

I think @keyframes can just go off of -[prefix]-animation-name in an element's style. Should we just leave other at-rules untouched?

For now yes, however css-vendor is a generic utility, if we do any specific checks then in jss-vendor-prefixer.

One more question: does jss-vendor-prefixer leave already prefixed props/values alone? Do I need to account for that here?

It only adds prefix if it won't work without. So here you just need to return the supported name or false.

A I mentioned before: test if passed property works as it is -> if yes return it -> if no add vendor prefix for the current runtime, test again -> if yes - return it -> if no - return false.

kof commented 9 years ago

And yes if someone will pass -moz-blah in chrome, it won't work we will add -webkit to it, it won't work either, we will return false

pstoica commented 9 years ago

So it sounds like in jss-vendor-prefixer we can just:

  1. check for @keyframes in rule.selector
  2. check supportedProperty('animation-name')
  3. convert -webkit-animation-name back to @-webkit-keyframes

and then we can leave other at-rules alone :smile:

Sound good? If so, close this and let me know if you want me to make those changes to jss-vendor-prefixer.

kof commented 9 years ago

Not sure I understand what you mean

  1. how do we check if @keyframes supported, we would need your test for this, no?
  2. Don't understand this :

convert -webkit-animation-name back to @-webkit-keyframes

pstoica commented 9 years ago

Sorry, forgot to include some context:

https://developer.mozilla.org/en-US/docs/Web/CSS/@keyframes Mozilla recommends:

To use keyframes, you create a @keyframes rule with a name that is then used by the animation-name property to match an animation to its keyframe list.

We already have your supportedProperty rule working fine for animation-name. It seems better to just reuse that instead of making a new test that 1) isn't that accurate and 2) checks for stuff we don't care about, we just need @keyframes.

kof commented 9 years ago

ok so lets discuss again in jss-vendor-prefixer what needs to be done as a separate issue there .... :)

kof commented 9 years ago

looks like we want to close this pr, right?

pstoica commented 9 years ago

Sounds good!

kof commented 9 years ago

just added keyframes prefixing in https://github.com/jsstyles/jss-vendor-prefixer