cssinjs / css-vendor

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

Test suite for supportedProperty #11

Closed cvle closed 7 years ago

cvle commented 7 years ago

Test cases are taken from http://shouldiprefix.com/ marked as use prefixes. Additionally the transition property is tested.

The following issues have been discovered and fixed:

cvle commented 7 years ago

We will need to do this for supportedValue as well.

cvle commented 7 years ago

Changed fixture to be generated from all available properties and using prefixes from autoprefixer. I think we can rely on autoprefixer to be correct, wdyt? Now the tests are all failing due to various wrong prefixes. We have to deal with the issues one by one.

kof commented 7 years ago

I think thats the right way to go. Are there many issues?

cvle commented 7 years ago

I'll make a list.

cvle commented 7 years ago

Some of the issues can be sorted out by correctly ignoring unsupported properties by the autoprefixer.

kof commented 7 years ago

Why does auto detection fail in those cases?

cvle commented 7 years ago

I think we can solve some cases for Edge by prefering ms prefix over the webkit prefix to align with the autoprefixer. Some issues from Opera can probably be solved by falling back to the webkit prefix. Others I don't know yet. It could possibly be like the filter property, where there is support for the same property without a prefix which behaves different than with a prefix.

kof commented 7 years ago

where there is support for the same property without a prefix which behaves different than with a prefix.

prefix has been always used for experimental implementations, if it works without we should use without.

cvle commented 7 years ago

Not always. I noticed that for the filter property, browser supports only the url() value when not using a prefix, but support others like grayscale() when using the prefix. That's why it failed in the tests. So it happens that for some properties there is a non-prefixed version and a prefixed-version that behaves differently. I guess some of the issues above could be caused by that.

kof commented 7 years ago

That makes sense, looks like prefixed version just implements more features which haven't been unprefixed yet. Bad for auto detection though. Maybe we should leave such props out for user to decide?

kof commented 7 years ago

Actually we need to do exactly the same like static auto prefixer does in such cases. Because in SSR a static prefixer will be used and we need a consistency.

kof commented 7 years ago

I was thinking to use https://github.com/rofrischmann/inline-style-prefix-all for the static version.

cvle commented 7 years ago

All static prefixers follow the caniuse data, I will try to stick to that. For the filter property I already have a solution by checking whether the unprefixed property accepts a grayscale value and if not use the prefixed property if available.

I'm currently using the data from autoprefixer here. It contains a list of properties and browsers that needs a prefix which I did manually before. But I cannot tell using this data whether the property is fully supported or not supported at all. I will work on a PR to expose the feature name so we can query the caniuse db by ourself.

I removed the Todo list above, it was inaccurate. I'll do another one when it's ready.

kof commented 7 years ago

Wondering if this will also prefix the gradient…

cvle commented 7 years ago

The PR here is a test suite for supportedProperty. Gradient is a case for supportedValue. Though I think the prefix should work, but we can only know for sure when we add a test case. What makes you wonder?

kof commented 7 years ago

Yeah, wrong pr, because this is a long standing issue https://github.com/cssinjs/jss/issues/90

kof commented 7 years ago

Can I help with this? What needs to be done?

cvle commented 7 years ago

I almost finished generating a better test fixture, then we can see which test cases are failing.

cvle commented 7 years ago

Related: https://github.com/Fyrd/caniuse/issues/3070

cvle commented 7 years ago

Pending PR: https://github.com/postcss/autoprefixer/pull/758

kof commented 7 years ago

@cvle how can I help here? we recently released http://cssinjs.org/ Now I can start with the next thing, I would probably start https://github.com/cssinjs/jss/issues/356 if you are going to finish this. Otherwise I need to do this, because its important.

cvle commented 7 years ago

I'm a bit occupied with some projects, but I am still determined to get this done. Now that autoprefixer merged my PR, I can update this.

cvle commented 7 years ago

Sorry for letting this stale for so long. I scheduled some time to work on this for the coming week. I'm finishing one of my projects soon so I'll have more time.

kof commented 7 years ago

Looking forward to it! By the way @rofrischmann mentioned in chat that he is happy to help. He built inline-style-prefixer

robinweser commented 7 years ago

True story. I'd love to help. Also got a lot of knowledge in that field actually.

cvle commented 7 years ago

Ok I started working on this again. This is now my main focus. I appreciate your help @rofrischmann, great!

cvle commented 7 years ago

Please review my commit. It adds the autoprefixer test suite, but doesn't fix any of the failing tests. Let's deal with the issues one by one until we have full compliance.

cvle commented 7 years ago

Wait before review, I'm not ready.

cvle commented 7 years ago

Ok ready. Now I give an unique string for each test for each browser so karma-mocha-reporter gives a good summary 👍

kof commented 7 years ago

nice!

kof commented 7 years ago

merged

cvle commented 7 years ago

Related: https://github.com/postcss/autoprefixer/pull/792

cvle commented 7 years ago

Related: https://github.com/postcss/autoprefixer/pull/793

cvle commented 7 years ago

Related: https://github.com/Fyrd/caniuse/pull/3189

cvle commented 7 years ago

Related: https://github.com/Fyrd/caniuse/pull/3190