Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Apr 8, 2019
Merged

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

merged 2 commits into from
Apr 8, 2019

Conversation

felthy
Copy link
Member

@felthy felthy commented Apr 4, 2019

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 transition/transform fix, CI fix (secure value added) #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 ?

@felthy felthy requested a review from AleshaOleg April 4, 2019 06:20
@AleshaOleg AleshaOleg self-assigned this Apr 6, 2019
@AleshaOleg AleshaOleg added the bug label Apr 6, 2019
@AleshaOleg
Copy link
Member

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: postcss/autoprefixer#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
Copy link
Member

@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
Copy link
Member

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

@felthy
Copy link
Member Author

felthy commented Apr 7, 2019

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
Copy link
Member

@felthy 👍

@AleshaOleg AleshaOleg merged commit 6784477 into cssinjs:master Apr 8, 2019
@AleshaOleg
Copy link
Member

Released in 2.0.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants