Test suite for supportedProperty #11

cvle · 2016-12-01T08:25:49Z

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:

Fixes issue when prefixing the filter property
Fallback to ms prefix on edge

cvle · 2016-12-01T08:49:29Z

We will need to do this for supportedValue as well.

kof · 2016-12-01T09:24:04Z

test/fixtures/property-prefix.js

+  full: 2,
+}
+
+const cases = [


can we somehow avoid maintaining this list, but instead generating it?

I wouldn't know how. There is no list anywhere connecting the caniuse feature name with the actual css properties. And then there are some edge cases, where we have to specifically follow the caniuse footnotes, explaining the differences in implementation and adhere to those. I think this is the best we can get.

You see here, I specifically target browsers with the note number 1.
https://github.com/cssinjs/css-vendor/pull/11/files/e335d5c0b13677d9b09859ff7eb4207a99597c89#diff-4bed91943a5c241cca1ff0b7ddc3fe7bR84

I think I can try relying on https://github.com/postcss/postcss-js for this. Let me see if it works.

cvle · 2016-12-01T11:09:03Z

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 · 2016-12-01T11:17:19Z

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

cvle · 2016-12-01T11:48:48Z

I'll make a list.

cvle · 2016-12-01T14:07:17Z

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

kof · 2016-12-01T14:12:57Z

Why does auto detection fail in those cases?

cvle · 2016-12-01T17:12:30Z

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 · 2016-12-01T17:59:16Z

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 · 2016-12-02T02:51:48Z

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 · 2016-12-02T08:40:14Z

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 · 2016-12-02T08:52:18Z

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 · 2016-12-02T08:53:29Z

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

cvle · 2016-12-02T09:39:15Z

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 · 2016-12-21T19:58:40Z

Wondering if this will also prefix the gradient…

cvle · 2016-12-23T01:19:17Z

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 · 2016-12-23T10:27:18Z

Yeah, wrong pr, because this is a long standing issue cssinjs/jss#90

kof · 2016-12-23T12:21:41Z

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

cvle · 2016-12-24T01:56:10Z

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

cvle · 2016-12-24T02:56:01Z

Related: Fyrd/caniuse#3070

cvle · 2016-12-24T03:22:39Z

Pending PR: postcss/autoprefixer#758

kof · 2017-01-13T15:43:42Z

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

cvle · 2017-01-13T16:34:20Z

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 · 2017-02-11T16:20:49Z

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 · 2017-02-11T16:22:38Z

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

robinweser · 2017-02-11T20:03:59Z

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

cvle · 2017-02-19T13:40:45Z

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

cvle · 2017-02-20T05:44:19Z

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.

kof · 2017-02-20T10:01:25Z

src/index.test.js

+      if (failMsgs.length > 0) {
+        failMsgs.forEach(err => {
+          const logMsg = `${currentBrowser.id} ${currentBrowser.version} ${err}`
+          console.log(logMsg) // eslint-disable-line no-console


in other repos we have started to use the warning module

Why do we catch the assertions and log the messages anyways? Shouldn't we just let let them throw and fix the code to support everything correctly?

If you throw immediately, you will only see the first non matching property prefix. This way we have a whole list and an overview of what needs to be done. We also can easily spot related property prefix errors.

The logging is meant for the CI log, not for end users, so I think the warning module is unnecessary.

It is also needed because the karma mocha reporter screws up the summary when running with multiple browsers..

I will add comments to clarify this

do we need it just for now to see an overview? after we fix them all, we can go back to normal assertion?

by the way, we could generate it() statements witha n assertion together, this should fix the reporting, it exits basically because one it is one test case, by design one it shouldn't have lots of assertions actually.

True lets do that. But we will still need to log to the console, because of the karma-mocha-reporter issue. The problem is that when two browsers fail the same test but outputs a different message, only the first message is reported in the summary.

cvle · 2017-02-20T12:23:01Z

Wait before review, I'm not ready.

cvle · 2017-02-20T12:31:01Z

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

kof · 2017-02-20T20:19:30Z

nice!

kof · 2017-02-20T20:19:52Z

merged

cvle · 2017-02-21T05:01:53Z

Related: postcss/autoprefixer#792

cvle · 2017-02-23T16:29:48Z

Related: postcss/autoprefixer#793

cvle · 2017-02-23T16:29:51Z

Related: Fyrd/caniuse#3189

cvle · 2017-02-23T17:06:46Z

Related: Fyrd/caniuse#3190

cvle mentioned this pull request Dec 1, 2016

[WIP] Support transition prefix #6

Closed

cvle force-pushed the property-prefix-test-suite branch 2 times, most recently from 6914564 to e335d5c Compare December 1, 2016 09:20

kof reviewed Dec 1, 2016

View reviewed changes

cvle changed the title ~~Test suite for supportedProperty~~ [WIP] Test suite for supportedProperty Dec 1, 2016

cvle force-pushed the property-prefix-test-suite branch from e335d5c to 0feb6f4 Compare December 1, 2016 11:01

cvle mentioned this pull request Dec 15, 2016

Expose feature name in prefixes data postcss/autoprefixer#754

Closed

cvle force-pushed the property-prefix-test-suite branch from 0feb6f4 to f7e20bf Compare February 20, 2017 05:38

kof reviewed Feb 20, 2017

View reviewed changes

cvle mentioned this pull request Feb 20, 2017

Support for different failure messages for the same test case litixsoft/karma-mocha-reporter#92

Closed

cvle force-pushed the property-prefix-test-suite branch from f7e20bf to 83eb9d5 Compare February 20, 2017 12:16

cvle force-pushed the property-prefix-test-suite branch from 83eb9d5 to f8c3c42 Compare February 20, 2017 12:29

cvle force-pushed the property-prefix-test-suite branch from f8c3c42 to ab5619d Compare February 20, 2017 12:35

Add test suite for property prefixes

690f7d6

cvle force-pushed the property-prefix-test-suite branch from ab5619d to 690f7d6 Compare February 20, 2017 12:36

cvle changed the title ~~[WIP] Test suite for supportedProperty~~ Test suite for supportedProperty Feb 20, 2017

kof merged commit f872ce3 into cssinjs:master Feb 20, 2017

Test suite for supportedProperty #11

Test suite for supportedProperty #11

Conversation

cvle commented Dec 1, 2016 • edited Loading

cvle commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvle commented Dec 1, 2016

kof commented Dec 1, 2016

cvle commented Dec 1, 2016

cvle commented Dec 1, 2016 • edited Loading

kof commented Dec 1, 2016

cvle commented Dec 1, 2016

kof commented Dec 1, 2016

cvle commented Dec 2, 2016

kof commented Dec 2, 2016

kof commented Dec 2, 2016

kof commented Dec 2, 2016

cvle commented Dec 2, 2016

kof commented Dec 21, 2016

cvle commented Dec 23, 2016

kof commented Dec 23, 2016

kof commented Dec 23, 2016

cvle commented Dec 24, 2016

cvle commented Dec 24, 2016

cvle commented Dec 24, 2016

kof commented Jan 13, 2017

cvle commented Jan 13, 2017

cvle commented Feb 11, 2017

kof commented Feb 11, 2017

robinweser commented Feb 11, 2017

cvle commented Feb 19, 2017

cvle commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvle Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvle commented Feb 20, 2017

cvle commented Feb 20, 2017 • edited Loading

kof commented Feb 20, 2017

kof commented Feb 20, 2017

cvle commented Feb 21, 2017

cvle commented Feb 23, 2017

cvle commented Feb 23, 2017

cvle commented Feb 23, 2017

cvle commented Dec 1, 2016 •

edited

Loading

cvle commented Dec 1, 2016 •

edited

Loading

cvle Feb 20, 2017 •

edited

Loading

cvle commented Feb 20, 2017 •

edited

Loading