csstools/postcss-focus-visible

preserve option minification issues

Closed this issue · 5 comments

jrwebdev commented

I recently upgraded the plugin and it broke the focus-visible styling in our app. I tracked it down to the preserve option being added along with our existing minification combining the two selectors. It appears that if browsers (tested Chrome, Safari, Firefox) encounter a pseudo-class they don't recognise, it just ignores the whole block. Example:

https://codepen.io/jrwebdev/pen/EpGbBx

So maybe it'd be an idea to either remove the preserve option or at least set the default to false?

Happy to provide a PR if needed.

Thanks

jonathantneal commented

The whole rule is cloned, not just the selector.

jonaskuske commented

Yes, the whole rule is cloned – but if you're using CSS minification (which isn't unlikely if you're using a build step w/ postcss), those two rules will be merged into one again, which breaks the polyfill.
So having preserve default to false would make a lot of sense.

Also, I'd add a warning to the section in the README covering the preserve option, as this issue can be hard to spot before it's too late: you usually only minify for production, so the polyfill works perfectly while running your development server, but once you build for production and host it, it suddenly breaks.

So can we maybe reopen this issue? Could also submit a PR if you want.

jonathantneal commented

Hi, @jonaskuske.

  1. In CSS, any unknown selector disables the entire rule. If you are using a CSS minifier that fails to account for this, please stop using it immediately and/or file a bug with that project. That would be a critical failing.

  2. I will not change the default functionality, because focus-within is a well-supported property (see https://caniuse.com/#feat=css-focus-within). Once Edge is on board, the fallbacks generated by this plugin should be phased out.

jonaskuske commented

Hi @jonathantneal ,

  1. Yep, the issue is caused by the minifier (I've deactivated mergeRules in CSSnano for now) but still, pointing out the potential danger of enabled preserve in combination with a minifier would be fair in my opinion.

  2. This is the polyfill for :focus-visible, which isn't supported anywhere unfortunately: https://caniuse.com/#search=focus-visible

  • 👍1
feibyte commented

I'm with @jonaskuske When I encounter this bug, I have no idea what happens. It looks all good on developing environment but failed on production.
It could lead to a potential bug for everyone.
If you won't change your mind, could you consider adding some Notice on the ReadMe page?

  • 👍2