Skip to content

Commit

Permalink
Fix false negatives for where, is, nth-child and nth-last-child in se…
Browse files Browse the repository at this point in the history
…lector-max-* (except selector-max-type) (#4842)

* updates keywordSets

* replaces functionality of isLogicalCombination with isContextFunctionalPseudoClass

note: possible logic error in selector-max-type, will investigate further

* adds a check for nonstandard syntax type selectors to selector-max-type

* Apply suggestions from code review

minor documentation fixes!

Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>

Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
  • Loading branch information
mattxwang and jeddy3 committed Jul 23, 2020
1 parent c935d0a commit cba8dcf
Show file tree
Hide file tree
Showing 12 changed files with 101 additions and 85 deletions.
12 changes: 6 additions & 6 deletions lib/reference/keywordSets.js
Expand Up @@ -208,9 +208,7 @@ keywordSets.pseudoElements = uniteSets(
);

keywordSets.aNPlusBNotationPseudoClasses = new Set([
'nth-child',
'nth-column',
'nth-last-child',
'nth-last-column',
'nth-last-of-type',
'nth-of-type',
Expand All @@ -220,6 +218,10 @@ keywordSets.linguisticPseudoClasses = new Set(['dir', 'lang']);

keywordSets.atRulePagePseudoClasses = new Set(['first', 'right', 'left', 'blank']);

keywordSets.logicalCombinationsPseudoClasses = new Set(['has', 'is', 'matches', 'not', 'where']);

keywordSets.aNPlusBOfSNotationPseudoClasses = new Set(['nth-child', 'nth-last-child']);

keywordSets.otherPseudoClasses = new Set([
'active',
'any-link',
Expand All @@ -241,19 +243,15 @@ keywordSets.otherPseudoClasses = new Set([
'focus-visible',
'fullscreen',
'future',
'has',
'host',
'host-context',
'hover',
'indeterminate',
'in-range',
'invalid',
'is',
'last-child',
'last-of-type',
'link',
'matches',
'not',
'only-child',
'only-of-type',
'optional',
Expand Down Expand Up @@ -302,6 +300,8 @@ keywordSets.webkitProprietaryPseudoClasses = new Set([
keywordSets.pseudoClasses = uniteSets(
keywordSets.aNPlusBNotationPseudoClasses,
keywordSets.linguisticPseudoClasses,
keywordSets.logicalCombinationsPseudoClasses,
keywordSets.aNPlusBOfSNotationPseudoClasses,
keywordSets.otherPseudoClasses,
);

Expand Down
6 changes: 3 additions & 3 deletions lib/rules/selector-max-attribute/index.js
Expand Up @@ -3,7 +3,7 @@
'use strict';

const _ = require('lodash');
const isLogicalCombination = require('../../utils/isLogicalCombination');
const isContextFunctionalPseudoClass = require('../../utils/isContextFunctionalPseudoClass');
const isStandardSyntaxRule = require('../../utils/isStandardSyntaxRule');
const optionsMatches = require('../../utils/optionsMatches');
const parseSelector = require('../../utils/parseSelector');
Expand Down Expand Up @@ -49,8 +49,8 @@ function rule(max, options) {

function checkSelector(selectorNode, ruleNode) {
const count = selectorNode.reduce((total, childNode) => {
// Only traverse inside actual selectors and logical combinations
if (childNode.type === 'selector' || isLogicalCombination(childNode)) {
// Only traverse inside actual selectors and context functional pseudo-classes
if (childNode.type === 'selector' || isContextFunctionalPseudoClass(childNode)) {
checkSelector(childNode, ruleNode);
}

Expand Down
6 changes: 3 additions & 3 deletions lib/rules/selector-max-class/index.js
Expand Up @@ -2,7 +2,7 @@

'use strict';

const isLogicalCombination = require('../../utils/isLogicalCombination');
const isContextFunctionalPseudoClass = require('../../utils/isContextFunctionalPseudoClass');
const isStandardSyntaxRule = require('../../utils/isStandardSyntaxRule');
const parseSelector = require('../../utils/parseSelector');
const report = require('../../utils/report');
Expand Down Expand Up @@ -34,8 +34,8 @@ function rule(max) {

function checkSelector(selectorNode, ruleNode) {
const count = selectorNode.reduce((total, childNode) => {
// Only traverse inside actual selectors and logical combinations
if (childNode.type === 'selector' || isLogicalCombination(childNode)) {
// Only traverse inside actual selectors and context functional pseudo-classes
if (childNode.type === 'selector' || isContextFunctionalPseudoClass(childNode)) {
checkSelector(childNode, ruleNode);
}

Expand Down
6 changes: 3 additions & 3 deletions lib/rules/selector-max-compound-selectors/index.js
Expand Up @@ -2,7 +2,7 @@

'use strict';

const isLogicalCombination = require('../../utils/isLogicalCombination');
const isContextFunctionalPseudoClass = require('../../utils/isContextFunctionalPseudoClass');
const isStandardSyntaxRule = require('../../utils/isStandardSyntaxRule');
const parseSelector = require('../../utils/parseSelector');
const report = require('../../utils/report');
Expand Down Expand Up @@ -39,8 +39,8 @@ function rule(max) {
let compoundCount = 1;

selectorNode.each((childNode) => {
// Only traverse inside actual selectors and logical combinations
if (childNode.type === 'selector' || isLogicalCombination(childNode)) {
// Only traverse inside actual selectors and context functional pseudo-classes
if (childNode.type === 'selector' || isContextFunctionalPseudoClass(childNode)) {
checkSelector(childNode, rule);
}

Expand Down
6 changes: 3 additions & 3 deletions lib/rules/selector-max-id/index.js
Expand Up @@ -3,7 +3,7 @@
'use strict';

const _ = require('lodash');
const isLogicalCombination = require('../../utils/isLogicalCombination');
const isContextFunctionalPseudoClass = require('../../utils/isContextFunctionalPseudoClass');
const isStandardSyntaxRule = require('../../utils/isStandardSyntaxRule');
const optionsMatches = require('../../utils/optionsMatches');
const parseSelector = require('../../utils/parseSelector');
Expand Down Expand Up @@ -47,10 +47,10 @@ function rule(max, options) {

function checkSelector(selectorNode, ruleNode) {
const count = selectorNode.reduce((total, childNode) => {
// Only traverse inside actual selectors and logical combinations that are not part of ignored functional pseudo-classes
// Only traverse inside actual selectors and context functional pseudo-classes that are not part of ignored functional pseudo-classes
if (
childNode.type === 'selector' ||
(isLogicalCombination(childNode) &&
(isContextFunctionalPseudoClass(childNode) &&
!isIgnoredContextFunctionalPseudoClass(childNode, options))
) {
checkSelector(childNode, ruleNode);
Expand Down
6 changes: 3 additions & 3 deletions lib/rules/selector-max-pseudo-class/index.js
Expand Up @@ -2,7 +2,7 @@

'use strict';

const isLogicalCombination = require('../../utils/isLogicalCombination');
const isContextFunctionalPseudoClass = require('../../utils/isContextFunctionalPseudoClass');
const isStandardSyntaxRule = require('../../utils/isStandardSyntaxRule');
const keywordSets = require('../../reference/keywordSets');
const parseSelector = require('../../utils/parseSelector');
Expand Down Expand Up @@ -35,8 +35,8 @@ function rule(max) {

function checkSelector(selectorNode, ruleNode) {
const count = selectorNode.reduce((total, childNode) => {
// Only traverse inside actual selectors and logical combinations
if (childNode.type === 'selector' || isLogicalCombination(childNode)) {
// Only traverse inside actual selectors and context functional pseudo-classes
if (childNode.type === 'selector' || isContextFunctionalPseudoClass(childNode)) {
checkSelector(childNode, ruleNode);
}

Expand Down
11 changes: 8 additions & 3 deletions lib/rules/selector-max-type/index.js
Expand Up @@ -3,11 +3,12 @@
'use strict';

const _ = require('lodash');
const isContextFunctionalPseudoClass = require('../../utils/isContextFunctionalPseudoClass');
const isKeyframeSelector = require('../../utils/isKeyframeSelector');
const isLogicalCombination = require('../../utils/isLogicalCombination');
const isOnlyWhitespace = require('../../utils/isOnlyWhitespace');
const isStandardSyntaxRule = require('../../utils/isStandardSyntaxRule');
const isStandardSyntaxSelector = require('../../utils/isStandardSyntaxSelector');
const isStandardSyntaxTypeSelector = require('../../utils/isStandardSyntaxTypeSelector');
const optionsMatches = require('../../utils/optionsMatches');
const parseSelector = require('../../utils/parseSelector');
const report = require('../../utils/report');
Expand Down Expand Up @@ -56,8 +57,8 @@ function rule(max, options) {

function checkSelector(selectorNode, ruleNode) {
const count = selectorNode.reduce((total, childNode) => {
// Only traverse inside actual selectors and logical combinations
if (childNode.type === 'selector' || isLogicalCombination(childNode)) {
// Only traverse inside actual selectors and context functional pseudo-classes
if (childNode.type === 'selector' || isContextFunctionalPseudoClass(childNode)) {
checkSelector(childNode, ruleNode);
}

Expand All @@ -81,6 +82,10 @@ function rule(max, options) {
return total;
}

if (childNode.type === 'tag' && !isStandardSyntaxTypeSelector(childNode)) {
return total;
}

return total + (childNode.type === 'tag');
}, 0);

Expand Down
48 changes: 48 additions & 0 deletions lib/utils/__tests__/isContextFunctionalPseudoClass.test.js
@@ -0,0 +1,48 @@
'use strict';

const isContextFunctionalPseudoClass = require('../isContextFunctionalPseudoClass');
const parseSelector = require('postcss-selector-parser');
const postcss = require('postcss');

function selector(css, cb) {
postcss.parse(css).walkRules((rule) => {
parseSelector((selectorAST) => {
selectorAST.walkPseudos(cb);
}).processSync(rule.selector);
});
}

describe('isContextFunctionalPseudoClass', () => {
it('handles non-context-functional pseudo-classes, like hover', () => {
selector('a:hover {}', (selector) => {
expect(isContextFunctionalPseudoClass(selector)).toBe(false);
});
});

it('handles logical combinations, ', () => {
selector('a:has(.foo) {}', (selector) => {
expect(isContextFunctionalPseudoClass(selector)).toBe(true);
});
selector('a:is(.foo) {}', (selector) => {
expect(isContextFunctionalPseudoClass(selector)).toBe(true);
});
selector('a:matches(.foo) {}', (selector) => {
expect(isContextFunctionalPseudoClass(selector)).toBe(true);
});
selector('a:not(.foo) {}', (selector) => {
expect(isContextFunctionalPseudoClass(selector)).toBe(true);
});
selector('a:where(.foo) {}', (selector) => {
expect(isContextFunctionalPseudoClass(selector)).toBe(true);
});
});

it('handles tree structural/NPlusBOfSNotationPseudoClasses classes', () => {
selector('a:nth-child(n+7) {}', (selector) => {
expect(isContextFunctionalPseudoClass(selector)).toBe(true);
});
selector('a:nth-last-child(n+7) {}', (selector) => {
expect(isContextFunctionalPseudoClass(selector)).toBe(true);
});
});
});
39 changes: 0 additions & 39 deletions lib/utils/__tests__/isLogicalCombination.test.js

This file was deleted.

23 changes: 23 additions & 0 deletions lib/utils/isContextFunctionalPseudoClass.js
@@ -0,0 +1,23 @@
'use strict';

const keywordSets = require('../reference/keywordSets');

/**
* Check whether a node is a context-functional pseudo-class (i.e. either a logical combination
* or a 'aNPlusBOfSNotationPseudoClasses' / tree-structural pseudo-class)
*
* @param {import('postcss-selector-parser').Pseudo} node postcss-selector-parser node (of type pseudo)
* @return {boolean} If `true`, the node is a context-functional pseudo-class
*/
module.exports = function isContextFunctionalPseudoClass(node) {
if (node.type === 'pseudo') {
const normalisedParentName = node.value.toLowerCase().replace(/:+/, '');

return (
keywordSets.logicalCombinationsPseudoClasses.has(normalisedParentName) ||
keywordSets.aNPlusBOfSNotationPseudoClasses.has(normalisedParentName)
);
}

return false;
};
22 changes: 0 additions & 22 deletions lib/utils/isLogicalCombination.js

This file was deleted.

1 change: 1 addition & 0 deletions lib/utils/isStandardSyntaxTypeSelector.js
Expand Up @@ -27,6 +27,7 @@ module.exports = function (node) {
if (
parentType === 'pseudo' &&
(keywordSets.aNPlusBNotationPseudoClasses.has(normalisedParentName) ||
keywordSets.aNPlusBOfSNotationPseudoClasses.has(normalisedParentName) ||
keywordSets.linguisticPseudoClasses.has(normalisedParentName) ||
keywordSets.shadowTreePseudoElements.has(normalisedParentName))
) {
Expand Down

0 comments on commit cba8dcf

Please sign in to comment.