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 selector-attribute-quotes false positives for "never" #6571

Merged
merged 10 commits into from Jan 27, 2023
5 changes: 5 additions & 0 deletions .changeset/chilly-ties-trade.md
@@ -0,0 +1,5 @@
---
"stylelint": patch
---

Fixed: false positives for "never" in `selector-attribute-quotes`
mattxwang marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 8 additions & 18 deletions lib/rules/selector-attribute-quotes/__tests__/index.js
Expand Up @@ -182,6 +182,14 @@ testRule({
code: "[href=\\'test\\'] { }",
description: 'escaped single-quotes are not considered as framing quotes',
},
{
code: `[href="te'st"] { }`,
description: 'removing quotes would produce invalid CSS', // see: https://github.com/stylelint/stylelint/issues/4300
},
{
code: `[href='te"st'] { }`,
description: 'removing quotes would produce invalid CSS', // see: https://github.com/stylelint/stylelint/issues/4300
},
],

reject: [
Expand Down Expand Up @@ -266,24 +274,6 @@ testRule({
endLine: 1,
endColumn: 20,
},
{
code: `[href="te'st"] { }`,
fixed: "[href=te\\'st] { }",
message: messages.rejected("te'st"),
line: 1,
column: 7,
endLine: 1,
endColumn: 14,
},
{
code: `[href='te"st'] { }`,
fixed: '[href=te\\"st] { }',
message: messages.rejected('te"st'),
line: 1,
column: 7,
endLine: 1,
endColumn: 14,
},
{
code: "[href='te\\'s\\'t'] { }",
fixed: "[href=te\\'s\\'t] { }",
Expand Down
12 changes: 12 additions & 0 deletions lib/rules/selector-attribute-quotes/index.js
Expand Up @@ -2,6 +2,7 @@

const getRuleSelector = require('../../utils/getRuleSelector');
const isStandardSyntaxRule = require('../../utils/isStandardSyntaxRule');
const isValidIdentifier = require('../../utils/isValidIdentifier');
const parseSelector = require('../../utils/parseSelector');
const report = require('../../utils/report');
const ruleMessages = require('../../utils/ruleMessages');
Expand Down Expand Up @@ -64,6 +65,17 @@ const rule = (primary, _secondaryOptions, context) => {
}

if (quoted && primary === 'never') {
// some selectors require quotes to be valid;
// we pass in the raw string value, which contains the escape characters
// necessary to check if escaped characters are valid
// see: https://github.com/stylelint/stylelint/issues/4300
if (
!attributeNode.raws.value ||
!isValidIdentifier(attributeNode.raws.value.slice(1, -1))
) {
ybiquitous marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (context.fix) {
selectorFixed = true;
attributeNode.quoteMark = null;
Expand Down
24 changes: 24 additions & 0 deletions lib/utils/__tests__/isValidIdentifier.test.js
@@ -0,0 +1,24 @@
'use strict';

const isValidIdentifier = require('../isValidIdentifier');

it('isValidIdentifier', () => {
expect(isValidIdentifier('foo')).toBeTruthy();
expect(isValidIdentifier('foo1')).toBeTruthy();
expect(isValidIdentifier('1')).toBeFalsy();
expect(isValidIdentifier('-1')).toBeFalsy();
expect(isValidIdentifier('--foo')).toBeTruthy();
expect(isValidIdentifier('has a space')).toBeFalsy();
expect(isValidIdentifier(null)).toBeFalsy();
expect(isValidIdentifier('')).toBeFalsy();
expect(isValidIdentifier(' ')).toBeFalsy();
expect(isValidIdentifier('foö')).toBeFalsy();
expect(isValidIdentifier('\\26 B')).toBeTruthy(); // ISO 10646 character
expect(isValidIdentifier('\\000026B')).toBeTruthy(); // ISO 10646 character
expect(isValidIdentifier("te'st")).toBeFalsy(); // unescaped quotation mark
expect(isValidIdentifier('te"st')).toBeFalsy(); // unescaped quotation mark
expect(isValidIdentifier("te\\'st")).toBeTruthy(); // escaped (in CSS) quotation mark
expect(isValidIdentifier('te\\"st')).toBeTruthy(); // escaped (in CSS) quotation mark
expect(isValidIdentifier("te\\'s\\'t")).toBeTruthy(); // escaped (in CSS) quotation mark
expect(isValidIdentifier('te\\"s\\"t')).toBeTruthy(); // escaped (in CSS) quotation mark
});
41 changes: 41 additions & 0 deletions lib/utils/isValidIdentifier.js
@@ -0,0 +1,41 @@
'use strict';

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be its own file? And hopefully, I haven't missed an existing version of this util!

/**
* returns whether a string is a valid CSS identifier
mattxwang marked this conversation as resolved.
Show resolved Hide resolved
* (i.e. only alphanumeric characters, `-`, and `_`;
* does not have a leading digit, leading dash followed by digit, or two leading dashes)
* furthermore, any escaped or ISO 10646 characters are allowed.
* see: https://www.w3.org/TR/CSS2/syndata.html#value-def-identifier
mattxwang marked this conversation as resolved.
Show resolved Hide resolved
* @param {string} ident
* @returns {boolean}
*/
module.exports = function isValidIdentifier(ident) {
Copy link
Contributor

@Mouvedia Mouvedia Jan 18, 2023

Choose a reason for hiding this comment

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

question

I dunno the stylelint conventions, but did you check if an npm package already existed?
Even if it does maybe we recommend not relying on dependencies whenever possible.
Iv done an id assert package for HTML/XML so I wouldn't be surprised if something similar existed for your case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, to be honest I didn't look too in-depth; I did a quick site:npmjs.com valid css identifier and found nothing on the first page or so of Google (much more support for valid JS identifiers or CSS selectors). If you're aware of one and we think we should use it though, happy to refactor!

if (!ident || ident.trim() === '') {
return false;
}

const hasDigitAt = (/** @type {number} */ i) => !isNaN(parseInt(ident.charAt(i)));

// trims, removes ISO 10646 characters, and singly-escaped characters
const trimmedIdent = ident
.trim()
.replace(/\\[0-9a-f]{1,6}(\\r\\n|[ \t\r\n\f])?/gi, '')
.replace(/\\./g, '');

// characters that are not alphanumeric, hyphen or underscore
if (trimmedIdent.match(/[^\w-]/)) {
mattxwang marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

// ident has a leading digit
if (hasDigitAt(0)) {
return false;
}

// ident has a leading dash followed by a digit
if (trimmedIdent.charAt(0) === '-' && hasDigitAt(1)) {
return false;
}
mattxwang marked this conversation as resolved.
Show resolved Hide resolved

return true;
};