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

Exclude all problematic pseudo-classes from rule-merging #392

Merged
merged 3 commits into from
Dec 2, 2014

Conversation

silverwind
Copy link
Contributor

Here's the patch to exclude all seven problematic pseudo-classes from rule-merging as discussed in #390.

@silverwind
Copy link
Contributor Author

Seems the first commit made it into this PR too because I based the second branch on it.

@@ -13,7 +13,7 @@ var DEFAULTS = {
},
selectors: {
ie7Hack: false, // *+html hack
special: /\-(moz|ms|o|webkit)\-/ // special selectors which prevent merging
special: /(\-moz\-|\-ms\-|\-o\-|\-webkit\-|:dir\((ltr|rtl)\)|:first|:fullscreen|:left|:read-only|:read-write|:right)/ // special selectors which prevent merging
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions:

  • Shall we rather just check for :dir in the pattern?
  • Checking for :first may also match :first-child which is mergeable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we rather just check for :dir in the pattern?

Yeah, might be more proof to future changes. Something like /:dir\(.+\)/ will probably be better.

Checking for :first may also match :first-child which is mergeable.

Good point. I think I'd use something like /:first(?!-)/ to avoid this, if you have a better suggestion, go ahead. I'm not 100% sure if this is even an issue, or if the class detection is good enough to avoid such a mismatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these are better. I'm a noob when it comes to negative lookahead. Feel free to improve :)

/:dir\([a-z-]*\)/
/:first(?![a-z-])/

Edit: updates

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silverwind These look good. Could you make a change and also add testcases for :first and :first-child?

@silverwind
Copy link
Contributor Author

Sorry for those extra commits on the tests, now it's done :)

@jakubpawlowicz
Copy link
Collaborator

Feel free to squash the last 3 together. Otherwise I'll do it :)

@silverwind
Copy link
Contributor Author

And they're squashed.

jakubpawlowicz added a commit that referenced this pull request Dec 2, 2014
Exclude all problematic pseudo-classes from rule-merging
@jakubpawlowicz jakubpawlowicz merged commit a5e416d into clean-css:master Dec 2, 2014
@jakubpawlowicz
Copy link
Collaborator

💯 now off to backporting it into 2.2.

@silverwind
Copy link
Contributor Author

👍 looking forward to it!

@jakubpawlowicz
Copy link
Collaborator

2.2.20 is out. Thanks again for the fix.

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

Successfully merging this pull request may close these issues.

None yet

2 participants