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

fix(postcss-merge-rules): prevent breaking rule merges #1072

Merged
merged 5 commits into from May 12, 2021

Conversation

ludofischer
Copy link
Collaborator

This PR fixes two related issues.

First commit, fixes: #730 by not merging the :host selector with others.
I have rebased #731 as it seems to me it fixes a legitimate problem (angular/angular-cli#18672).
Although this comment #876 (comment) says the PR is already done, the test fails on master.
There's a comment #730 (comment) mentioning the fix is not safe if the browser does not support :host but it seems the current situation is what would caus the bug.

As #999 states, merging unknown and known selectors causes the browser to ignore the whole rule, so not merging should be the safe path.
Second commit fixes #999. If the pseudo-selector is unknown and does not start with a vendor prefix, do not merge.

@ludofischer ludofischer changed the title fix(postcss-fix-rules): prevent merging of :host(tag) and tag fix(postcss-merge-rules): prevent breaking rule merges May 11, 2021
MapTo0 and others added 3 commits May 12, 2021 00:16
Move all checks together.
Do not merge if the pseudo-selector is not in the list of well-known pseudo-selectors
and does not start with a vendor prefix. We let through vendor prefixes as they are
handled elsewhere in the code.

Fix #999
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2021

Codecov Report

Merging #1072 (5e4a3b5) into master (15235a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1072   +/-   ##
=======================================
  Coverage   96.37%   96.38%           
=======================================
  Files         115      115           
  Lines        3588     3594    +6     
  Branches     1058     1060    +2     
=======================================
+ Hits         3458     3464    +6     
  Misses        121      121           
  Partials        9        9           
Impacted Files Coverage Δ
packages/postcss-merge-rules/src/index.js 97.57% <ø> (-0.18%) ⬇️
...postcss-merge-rules/src/lib/ensureCompatibility.js 98.63% <100.00%> (+0.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15235a9...5e4a3b5. Read the comment docs.

@ludofischer
Copy link
Collaborator Author

I think the pseudo-selector list https://github.com/cssnano/cssnano/blob/master/packages/postcss-merge-rules/src/lib/ensureCompatibility.js is incomplete, which causes deoptimizations after this pull request. For example, the list is missing visited, so we get:

blockquote cite a, blockquote cite a:visited {color: #555} 

becomes

blockquote cite a { color: #255 }blockquote cite a:visited {color: #555}

That's because before it sees :visited as an unknown pseudo-selector, so it does not merge it anymore.

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