search

cssinjs / css-vendor

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

-webkit-backdrop-filter required on Microsoft Edge #84

Closed felthy closed 5 years ago

felthy commented 5 years ago

In the current release supportedProperty('backdrop-filter') returns 'backdrop-filter' on Edge, but that browser requires -webkit-backdrop-filter.

This case is handled by the prefixed plugin, which tries the webkit prefix as a fallback if the browser’s own prefix (ms in this case) is not supported. The fallback works correctly in this case, but it returns the original value of prop. This line of code used to be `-webkit-${prop}` but the -webkit part was removed in PR #26.

This PR restores the -webkit prefix, which works for me to get backdrop-filter working in MS Edge. There are a couple of things I'm unsure of though:

  1. This probably needs a unit test, but I'm not sure how to go about adding one. I don't have a BrowserStack account to be able try the USE_CLOUD configuration and see what happens when it runs on Edge, or whether the backdrop-filter style is already included by generateFixture().
  2. I don’t know why the -webkit prefix was removed from the fallback in #26. That PR was about transition/transform, both of which have their own plugins, so I don't think the prefixed plugin would be executed at all for those styles. This comment was also deleted at the same time: // E.g. appearance in Edge & IE Mobile needs a -webkit- prefix.. Do you remember the context here @AleshaOleg ?
AleshaOleg commented 5 years ago

Hey @felthy, you're absolutely right!

The problem is that we build our tests on autoprefixer base. And seems it have a bug with proper prefix defining for backdrop-filter property. I just made a PR to fix that: https://github.com/postcss/autoprefixer/pull/1208. After this things will be merged and released, I'll test again your code locally, and most likely will merge it! I already test it and can say that your changes only touch this property.

I think I removed those line of code because I trusted to autoprefixer, how this property should be prefixed and tests fail to me.

I want to say sorry, that your tests on Travis failed because we're using BrowserStack for testing and Travis will work only if some people which is authorized to run it will commit some changes.

Thanks!

AleshaOleg commented 5 years ago

@felthy Can you please update autoprefixer to 9.5.1 for this PR? It was just released and contains info about proper prefixing backdrop-filter property. Just checked locally, your code working as expected after an update. Thanks!

AleshaOleg commented 5 years ago

As you can see update to autoprefixer without your fix, breaks a build :D

felthy commented 5 years ago

Thanks very much for reviewing so quickly and for making the upstream fixes @AleshaOleg! I've updated autoprefixer in this PR so I expect the tests would pass on Travis if they were run by an authorized user.

AleshaOleg commented 5 years ago

@felthy 👍

AleshaOleg commented 5 years ago

Released in 2.0.2.