From cba8dcfa4ff3a5733016c34007cb64a19d863662 Mon Sep 17 00:00:00 2001 From: Matt Wang Date: Thu, 23 Jul 2020 02:28:39 -0700 Subject: [PATCH] Fix false negatives for where, is, nth-child and nth-last-child in selector-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 Co-authored-by: Richard Hallows --- lib/reference/keywordSets.js | 12 ++--- lib/rules/selector-max-attribute/index.js | 6 +-- lib/rules/selector-max-class/index.js | 6 +-- .../selector-max-compound-selectors/index.js | 6 +-- lib/rules/selector-max-id/index.js | 6 +-- lib/rules/selector-max-pseudo-class/index.js | 6 +-- lib/rules/selector-max-type/index.js | 11 +++-- .../isContextFunctionalPseudoClass.test.js | 48 +++++++++++++++++++ .../__tests__/isLogicalCombination.test.js | 39 --------------- lib/utils/isContextFunctionalPseudoClass.js | 23 +++++++++ lib/utils/isLogicalCombination.js | 22 --------- lib/utils/isStandardSyntaxTypeSelector.js | 1 + 12 files changed, 101 insertions(+), 85 deletions(-) create mode 100644 lib/utils/__tests__/isContextFunctionalPseudoClass.test.js delete mode 100644 lib/utils/__tests__/isLogicalCombination.test.js create mode 100644 lib/utils/isContextFunctionalPseudoClass.js delete mode 100644 lib/utils/isLogicalCombination.js diff --git a/lib/reference/keywordSets.js b/lib/reference/keywordSets.js index 0fdf90ffc2..52f4a718d5 100644 --- a/lib/reference/keywordSets.js +++ b/lib/reference/keywordSets.js @@ -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', @@ -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', @@ -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', @@ -302,6 +300,8 @@ keywordSets.webkitProprietaryPseudoClasses = new Set([ keywordSets.pseudoClasses = uniteSets( keywordSets.aNPlusBNotationPseudoClasses, keywordSets.linguisticPseudoClasses, + keywordSets.logicalCombinationsPseudoClasses, + keywordSets.aNPlusBOfSNotationPseudoClasses, keywordSets.otherPseudoClasses, ); diff --git a/lib/rules/selector-max-attribute/index.js b/lib/rules/selector-max-attribute/index.js index c344139628..f0a4f9c3c6 100644 --- a/lib/rules/selector-max-attribute/index.js +++ b/lib/rules/selector-max-attribute/index.js @@ -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'); @@ -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); } diff --git a/lib/rules/selector-max-class/index.js b/lib/rules/selector-max-class/index.js index b48a9a5b8a..8e08e1a4ed 100644 --- a/lib/rules/selector-max-class/index.js +++ b/lib/rules/selector-max-class/index.js @@ -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'); @@ -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); } diff --git a/lib/rules/selector-max-compound-selectors/index.js b/lib/rules/selector-max-compound-selectors/index.js index febfddf17a..92ed6145ec 100644 --- a/lib/rules/selector-max-compound-selectors/index.js +++ b/lib/rules/selector-max-compound-selectors/index.js @@ -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'); @@ -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); } diff --git a/lib/rules/selector-max-id/index.js b/lib/rules/selector-max-id/index.js index bf608bb3dc..91555fe03d 100644 --- a/lib/rules/selector-max-id/index.js +++ b/lib/rules/selector-max-id/index.js @@ -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'); @@ -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); diff --git a/lib/rules/selector-max-pseudo-class/index.js b/lib/rules/selector-max-pseudo-class/index.js index 0d73785815..fc8d581a98 100644 --- a/lib/rules/selector-max-pseudo-class/index.js +++ b/lib/rules/selector-max-pseudo-class/index.js @@ -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'); @@ -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); } diff --git a/lib/rules/selector-max-type/index.js b/lib/rules/selector-max-type/index.js index 05bc10ab5a..0b4502c660 100644 --- a/lib/rules/selector-max-type/index.js +++ b/lib/rules/selector-max-type/index.js @@ -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'); @@ -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); } @@ -81,6 +82,10 @@ function rule(max, options) { return total; } + if (childNode.type === 'tag' && !isStandardSyntaxTypeSelector(childNode)) { + return total; + } + return total + (childNode.type === 'tag'); }, 0); diff --git a/lib/utils/__tests__/isContextFunctionalPseudoClass.test.js b/lib/utils/__tests__/isContextFunctionalPseudoClass.test.js new file mode 100644 index 0000000000..f3f506128b --- /dev/null +++ b/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); + }); + }); +}); diff --git a/lib/utils/__tests__/isLogicalCombination.test.js b/lib/utils/__tests__/isLogicalCombination.test.js deleted file mode 100644 index 33e64ef749..0000000000 --- a/lib/utils/__tests__/isLogicalCombination.test.js +++ /dev/null @@ -1,39 +0,0 @@ -'use strict'; - -const isLogicalCombination = require('../isLogicalCombination'); -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('isLogicalCombination', () => { - it('hover pseudo class is NOT logical combination', () => { - selector('a:hover {}', (selector) => { - expect(isLogicalCombination(selector)).toBe(false); - }); - }); - - it('not pseudo class is logical combination', () => { - selector('a:not(.foo) {}', (selector) => { - expect(isLogicalCombination(selector)).toBe(true); - }); - }); - - it('has pseudo class is logical combination', () => { - selector('a:has(.foo) {}', (selector) => { - expect(isLogicalCombination(selector)).toBe(true); - }); - }); - - it('matches pseudo class is logical combination', () => { - selector('a:matches(.foo) {}', (selector) => { - expect(isLogicalCombination(selector)).toBe(true); - }); - }); -}); diff --git a/lib/utils/isContextFunctionalPseudoClass.js b/lib/utils/isContextFunctionalPseudoClass.js new file mode 100644 index 0000000000..4fcca055bb --- /dev/null +++ b/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; +}; diff --git a/lib/utils/isLogicalCombination.js b/lib/utils/isLogicalCombination.js deleted file mode 100644 index cd92885eec..0000000000 --- a/lib/utils/isLogicalCombination.js +++ /dev/null @@ -1,22 +0,0 @@ -'use strict'; - -/** - * Check whether a node is logical combination (`:not`, `:has`, `:matches`) - * - * @param {import('postcss-selector-parser').Pseudo} node postcss-selector-parser node (of type pseudo) - * @return {boolean} If `true`, the combination is logical - */ -module.exports = function isLogicalCombination(node) { - if (node.type === 'pseudo') { - switch (node.value) { - case ':not': - case ':has': - case ':matches': - return true; - default: - return false; - } - } - - return false; -}; diff --git a/lib/utils/isStandardSyntaxTypeSelector.js b/lib/utils/isStandardSyntaxTypeSelector.js index 1275676272..3bf35e4bf5 100644 --- a/lib/utils/isStandardSyntaxTypeSelector.js +++ b/lib/utils/isStandardSyntaxTypeSelector.js @@ -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)) ) {