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

mergeRules should not merge unknown pseudo selectors #642

Closed
fracmak opened this issue Oct 18, 2018 · 12 comments · Fixed by #882
Closed

mergeRules should not merge unknown pseudo selectors #642

fracmak opened this issue Oct 18, 2018 · 12 comments · Fixed by #882
Labels
Milestone

Comments

@fracmak
Copy link

fracmak commented Oct 18, 2018

It appears that if a browser (tested in Chrome 69, Firefox 60, Safari 12, Edge 16) does not recognize a pseudo selector, it drops the entire selector, including any combined selectors.

Example:

https://codepen.io/anon/pen/ZqxNEq

image

@saulhardman
Copy link

I've recently come across this issue myself and I'm wondering if it's currently possible to disable optimisations on a line-by-line basis? E.g. like ESLint's // eslint-disable-line comment format.

@alexander-akait
Copy link
Member

@saulhardman no, it is hard to implement using comments, but PR welcome

@saulhardman
Copy link

@evilebottnawi I figured as much, thanks for your response 👍

@alexander-akait alexander-akait added this to the 4.1 milestone Jan 31, 2019
@ben-eb
Copy link
Collaborator

ben-eb commented Jun 7, 2019

@evilebottnawi We have a system for handling mergeable pseudo elements here:

https://github.com/cssnano/cssnano/blob/master/packages/postcss-merge-rules/src/lib/ensureCompatibility.js#L13

Perhaps this should be changed; currently this list prevents a merge for known pseudo selectors, but allows unknown ones through. If we only allow a merge on known pseudo selectors, we will have less bug reports on broken behaviour and hopefully more reports of acceptable pseudo selector merging.

In any case this is a matter of adding ':focus-visible': 'css-focus-visible' to that list.

@alexander-akait
Copy link
Member

Good idea, can you take care about this?

@ben-eb
Copy link
Collaborator

ben-eb commented Jun 7, 2019

@evilebottnawi Probably easier if we fix the immediate bug and then can look into changing behaviour later?

@alexander-akait
Copy link
Member

Sounds good, need open issue about this or update currently

@felixfbecker
Copy link

:focus-visible is now on the list (so rules using it will be merged), but not supported by any browsers other than Firefox, meaning CSSNano currently breaks Chrome when e.g. using a focus-visible polyfill that creates duplicate rules for :focus-visible and a polyfilled .focus-visible class. Is there a way to instruct CSSNano to not consider :focus-visible "supported"?

@anikethsaha
Copy link
Member

are you using the nightly ?

cause this has been fixed in #882 but not released yet.

@anikethsaha
Copy link
Member

:focus-visible is now on the list (so rules using it will be merged)

no it wont (in the master). ref this https://github.com/cssnano/cssnano/blob/a4b38cbfaf/packages/postcss-merge-rules/src/__tests__/index.js#L790

@felixfbecker
Copy link

Ah, I was confused, I understand the issue now (before all unknown selectors were merged, and :focus-visible was unknown, but now both unknown selectors are no longer merged and :focus-visible is known now, but will still not be merged depending on CanIUse/browserslist data). Thanks.
Is there an ETA for the release?

@anikethsaha
Copy link
Member

once #954 is done.

In the meantime use cssnano@nightly. that is up to date with the master and pretty stable.

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 a pull request may close this issue.

6 participants