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.

Sign up for GitHub

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

Jump to bottom

Fix selector-max-specificity end positions #7685

Merged
merged 7 commits into from
May 11, 2024
Merged

Fix selector-max-specificity end positions #7685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
df247f0
Fix `selector-max-specificity` end positions
romainmenke May 10, 2024
ff462a2
Create kind-apricots-greet.md
romainmenke May 10, 2024
f39fd45
increase coverage
romainmenke May 10, 2024
19b11ee
Merge branch 'fix-selector-max-specificity--adventurous-pelican-112c3…
romainmenke May 10, 2024
9c819ed
increase coverage
romainmenke May 10, 2024
535503a
cleanup
romainmenke May 10, 2024
56269fe
Merge branch 'main' into fix-selector-max-specificity--adventurous-pe…
romainmenke May 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/kind-apricots-greet.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"stylelint": patch
---

Fixed: `selector-max-specificity` end positions
86 changes: 77 additions & 9 deletions lib/rules/selector-max-specificity/__tests__/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ testRule({
{
code: ':nth-child(2n + 1) {}',
},
{
code: '[value] {}',
Copy link
Member

@Mouvedia Mouvedia May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit
non-blocking

It's kind of contradictory.
i.e. that's the attribute name not its value

},
],

reject: [
Expand Down Expand Up @@ -156,6 +159,14 @@ testRule({
endLine: 1,
endColumn: 27,
},
{
code: '.a [value] {}',
message: messages.expected('.a [value]', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 11,
},
],
});

Expand Down Expand Up @@ -260,47 +271,47 @@ testRule({
},
{
code: '.cd { .de { .fg {} } }',
message: messages.expected('.cd .de .fg', '0,2,1'),
message: messages.expected('.fg', '0,2,1'),
line: 1,
column: 13,
endLine: 1,
endColumn: 16,
},
{
Mouvedia marked this conversation as resolved.
Show resolved Hide resolved
code: '.cd { .de { & > .fg {} } }',
message: messages.expected('.cd .de > .fg', '0,2,1'),
message: messages.expected('& > .fg', '0,2,1'),
line: 1,
column: 13,
endLine: 1,
endColumn: 20,
},
{
code: '.cd { .de { &:hover > .fg {} } }',
message: messages.expected('.cd .de:hover > .fg', '0,2,1'),
message: messages.expected('&:hover > .fg', '0,2,1'),
line: 1,
column: 13,
endLine: 1,
endColumn: 26,
},
{
code: '.cd { .de { .fg > & {} } }',
message: messages.expected('.fg > .cd .de', '0,2,1'),
message: messages.expected('.fg > &', '0,2,1'),
line: 1,
column: 13,
endLine: 1,
endColumn: 20,
},
{
code: '.cd { @media print { .de { & + .fg {} } } }',
message: messages.expected('.cd .de + .fg', '0,2,1'),
message: messages.expected('& + .fg', '0,2,1'),
line: 1,
column: 28,
endLine: 1,
endColumn: 35,
},
{
code: '@media print { li { & + .ab, .ef.ef { .cd {} } } }',
message: messages.expected('li .ef.ef .cd', '0,2,1'),
message: messages.expected('.cd', '0,2,1'),
line: 1,
column: 39,
endLine: 1,
Expand All @@ -322,21 +333,46 @@ testRule({
reject: [
{
code: '.thing .thing2 {&.nested {#pop {}}}',
message: messages.expected('.thing .thing2.nested #pop', '0,4,1'),
message: messages.expected('#pop', '0,4,1'),
line: 1,
column: 27,
endLine: 1,
endColumn: 31,
},
{
code: '.thing .thing2 {#here & {}}',
message: messages.expected('#here .thing .thing2', '0,4,1'),
message: messages.expected('#here &', '0,4,1'),
line: 1,
column: 17,
endLine: 1,
endColumn: 24,
},
{
code: '.thing .thing2 .thing3 .thing4 {a.here & {}}',
message: messages.expected('a.here .thing .thing2 .thing3 .thing4', '0,4,1'),
message: messages.expected('a.here &', '0,4,1'),
line: 1,
column: 33,
endLine: 1,
endColumn: 41,
},
{
code: '.a { #b { #c {} } }',
warnings: [
{
message: messages.expected('#b', '0,4,1'),
line: 1,
column: 6,
endLine: 1,
endColumn: 8,
},
{
message: messages.expected('#c', '0,4,1'),
line: 1,
column: 11,
endLine: 1,
endColumn: 13,
},
],
},
],
});
Expand All @@ -363,48 +399,64 @@ testRule({
message: messages.expected('.ab .ab', '0,1,1'),
line: 1,
column: 1,
endLine: 1,
endColumn: 8,
},
{
code: '.a:not(.b) { @include test {} }',
message: messages.expected('.a:not(.b)', '0,1,1'),
line: 1,
column: 1,
endLine: 1,
endColumn: 11,
},
{
code: '.a:not(.b, .c) { @include test {} }',
message: messages.expected('.a:not(.b, .c)', '0,1,1'),
line: 1,
column: 1,
endLine: 1,
endColumn: 15,
},
{
code: ':not(.b, .c.d) { @include test {} }',
message: messages.expected(':not(.b, .c.d)', '0,1,1'),
line: 1,
column: 1,
endLine: 1,
endColumn: 15,
},
{
code: '.a:matches(.b) { @include test {} }',
message: messages.expected('.a:matches(.b)', '0,1,1'),
line: 1,
column: 1,
endLine: 1,
endColumn: 15,
},
{
code: '.a:matches(.b, .c) { @include test {} }',
message: messages.expected('.a:matches(.b, .c)', '0,1,1'),
line: 1,
column: 1,
endLine: 1,
endColumn: 19,
},
{
code: ':matches(.b, .c.d) { @include test {} }',
message: messages.expected(':matches(.b, .c.d)', '0,1,1'),
line: 1,
column: 1,
endLine: 1,
endColumn: 19,
},
{
code: '@include test { .ab .ab {} }',
message: messages.expected('.ab .ab', '0,1,1'),
line: 1,
column: 17,
endLine: 1,
endColumn: 24,
},
],
});
Expand Down Expand Up @@ -470,42 +522,56 @@ testRule({
message: messages.expected('.a:global(.b)', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 14,
},
{
code: '.a:global(.b, .c) {}',
message: messages.expected('.a:global(.b, .c)', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 18,
},
{
code: ':global(.b, .c.d) {}',
message: messages.expected(':global(.b, .c.d)', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 18,
},
{
code: '.a:local(.b) {}',
message: messages.expected('.a:local(.b)', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 13,
},
{
code: '.a:local(.b, .c) {}',
message: messages.expected('.a:local(.b, .c)', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 17,
},
{
code: ':local(.b, .c.d) {}',
message: messages.expected(':local(.b, .c.d)', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 17,
},
{
code: 'my-tag.a.b {}',
message: messages.expected('my-tag.a.b', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 11,
},
],
});
Expand All @@ -531,6 +597,8 @@ testRule({
message: messages.expected('.a:global(.b)', '0,1,0'),
line: 1,
column: 1,
endLine: 1,
endColumn: 14,
},
],
});
36 changes: 13 additions & 23 deletions lib/rules/selector-max-specificity/index.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,11 @@
'use strict';

const selectorSpecificity = require('@csstools/selector-specificity');
const resolvedNestedSelector = require('postcss-resolve-nested-selector');
const selectors = require('../../reference/selectors.cjs');
const validateTypes = require('../../utils/validateTypes.cjs');
const flattenNestedSelectorsForRule = require('../../utils/flattenNestedSelectorsForRule.cjs');
const isStandardSyntaxRule = require('../../utils/isStandardSyntaxRule.cjs');
const isStandardSyntaxSelector = require('../../utils/isStandardSyntaxSelector.cjs');
const optionsMatches = require('../../utils/optionsMatches.cjs');
const parseSelector = require('../../utils/parseSelector.cjs');
const report = require('../../utils/report.cjs');
const ruleMessages = require('../../utils/ruleMessages.cjs');
const validateOptions = require('../../utils/validateOptions.cjs');
Expand Down Expand Up @@ -233,35 +231,27 @@ const rule = (primary, secondaryOptions) => {
const maxSpecificity = { a, b, c };

root.walkRules((ruleNode) => {
if (!isStandardSyntaxRule(ruleNode)) {
return;
}

// Using `.selectors` gets us each selector in the eventuality we have a comma separated set
for (const selector of ruleNode.selectors) {
for (const resolvedSelector of resolvedNestedSelector(selector, ruleNode)) {
// Skip non-standard syntax selectors
if (!isStandardSyntaxSelector(resolvedSelector)) {
continue;
}

const selectorTree = parseSelector(resolvedSelector, result, ruleNode);

if (!selectorTree) continue;
if (!isStandardSyntaxRule(ruleNode)) return;

flattenNestedSelectorsForRule(ruleNode, result).forEach(({ selector, resolvedSelectors }) => {
resolvedSelectors.forEach((resolvedSelector) => {
// Check if the selector specificity exceeds the allowed maximum
if (selectorSpecificity.compare(maxChildSpecificity(selectorTree), maxSpecificity) > 0) {
if (selectorSpecificity.compare(nodeSpecificity(resolvedSelector), maxSpecificity) > 0) {
const index = selector.first?.sourceIndex ?? 0;
const selectorStr = selector.toString().trim();

report({
ruleName,
result,
node: ruleNode,
message: messages.expected,
messageArgs: [resolvedSelector, primary],
word: selector,
messageArgs: [selectorStr, primary],
index,
endIndex: index + selectorStr.length,
});
}
}
}
});
});
});
};
};
Expand Down
36 changes: 13 additions & 23 deletions lib/rules/selector-max-specificity/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import { compare, selectorSpecificity } from '@csstools/selector-specificity';
import resolvedNestedSelector from 'postcss-resolve-nested-selector';

import {
aNPlusBNotationPseudoClasses,
aNPlusBOfSNotationPseudoClasses,
linguisticPseudoClasses,
} from '../../reference/selectors.mjs';
import { assertNumber, isRegExp, isString } from '../../utils/validateTypes.mjs';
import flattenNestedSelectorsForRule from '../../utils/flattenNestedSelectorsForRule.mjs';
import isStandardSyntaxRule from '../../utils/isStandardSyntaxRule.mjs';
import isStandardSyntaxSelector from '../../utils/isStandardSyntaxSelector.mjs';
import optionsMatches from '../../utils/optionsMatches.mjs';
import parseSelector from '../../utils/parseSelector.mjs';
import report from '../../utils/report.mjs';
import ruleMessages from '../../utils/ruleMessages.mjs';
import validateOptions from '../../utils/validateOptions.mjs';
Expand Down Expand Up @@ -234,35 +232,27 @@ const rule = (primary, secondaryOptions) => {
const maxSpecificity = { a, b, c };

root.walkRules((ruleNode) => {
if (!isStandardSyntaxRule(ruleNode)) {
return;
}

// Using `.selectors` gets us each selector in the eventuality we have a comma separated set
for (const selector of ruleNode.selectors) {
for (const resolvedSelector of resolvedNestedSelector(selector, ruleNode)) {
// Skip non-standard syntax selectors
if (!isStandardSyntaxSelector(resolvedSelector)) {
continue;
}

const selectorTree = parseSelector(resolvedSelector, result, ruleNode);

if (!selectorTree) continue;
if (!isStandardSyntaxRule(ruleNode)) return;

flattenNestedSelectorsForRule(ruleNode, result).forEach(({ selector, resolvedSelectors }) => {
resolvedSelectors.forEach((resolvedSelector) => {
// Check if the selector specificity exceeds the allowed maximum
if (compare(maxChildSpecificity(selectorTree), maxSpecificity) > 0) {
if (compare(nodeSpecificity(resolvedSelector), maxSpecificity) > 0) {
const index = selector.first?.sourceIndex ?? 0;
Copy link
Member

Choose a reason for hiding this comment

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

[note] The coverage diff is caused by this line, but I think it's ignorable.

If improving the coverage, we might change it like this, but I'm unsure it's really safe...

Suggested change
const index = selector.first?.sourceIndex ?? 0;
const index = selector.first.sourceIndex;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think it is safe in practice, but I'd rather keep it anyway.

If the AST is mutated without setting sourceIndex or if something clears out a selector it would throw errors.

But maybe we can add a helper so that we can document this better?

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with keeping it as-is, and I agree with adding a helper. 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

I should actually fix this one : postcss/postcss-selector-parser#287

Then we change const index = selector.first?.sourceIndex ?? 0 to const index = selector.sourceIndex

Copy link
Member

Choose a reason for hiding this comment

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

nice👍

const selectorStr = selector.toString().trim();

report({
ruleName,
result,
node: ruleNode,
message: messages.expected,
messageArgs: [resolvedSelector, primary],
word: selector,
messageArgs: [selectorStr, primary],
index,
endIndex: index + selectorStr.length,
});
}
}
}
});
});
});
};
};
Expand Down