Skip to content

Commit

Permalink
New: Allow ranged locations in CSS
Browse files Browse the repository at this point in the history 
Fix #3130
Close #3138
  • Loading branch information
@sarvaje @molant
sarvaje authored and molant committed Oct 25, 2019
1 parent c1fd229 commit de43df9
Show file tree
Hide file tree
Showing 14 changed files with 302 additions and 84 deletions.
9 changes: 7 additions & 2 deletions packages/extension-vscode/src/utils/problems.ts
View file
Expand Up @@ -17,18 +17,23 @@ const webhintToDiagnosticServerity = (severity: Severity): DiagnosticSeverity =>
// Translate a webhint problem into the VSCode diagnostic format. // Translate a webhint problem into the VSCode diagnostic format.
export const problemToDiagnostic = (problem: Problem): Diagnostic => { export const problemToDiagnostic = (problem: Problem): Diagnostic => {


let { column: character, line } = problem.location; let { column: character, endColumn, endLine, line } = problem.location;


// Move (-1, -1) or similar to (0, 0) so VSCode underlines the start of the document. // Move (-1, -1) or similar to (0, 0) so VSCode underlines the start of the document.
if (character < 0 || line < 0) { if (character < 0 || line < 0) {
character = 0; character = 0;
line = 0; line = 0;
} }


if (!endColumn || !endLine) {
endColumn = character;
endLine = line;
}

return { return {
message: `${problem.message}\n(${problem.hintId})`, message: `${problem.message}\n(${problem.hintId})`,
range: { range: {
end: { character, line }, end: { character: endColumn, line: endLine },
start: { character, line } start: { character, line }
}, },
severity: webhintToDiagnosticServerity(problem.severity), severity: webhintToDiagnosticServerity(problem.severity),
Expand Down
29 changes: 29 additions & 0 deletions packages/extension-vscode/tests/utils/problems.ts
View file
Expand Up @@ -8,6 +8,8 @@ import { problemToDiagnostic } from '../../src/utils/problems';
test('It translates a basic problem correctly', (t) => { test('It translates a basic problem correctly', (t) => {
const location = { const location = {
column: 5, column: 5,
endColumn: 10,
endLine: 8,
line: 7 line: 7
}; };


Expand All @@ -26,6 +28,8 @@ test('It translates a basic problem correctly', (t) => {
t.is(diagnostic.severity, DiagnosticSeverity.Warning); t.is(diagnostic.severity, DiagnosticSeverity.Warning);
t.is(diagnostic.range.start.line, location.line); t.is(diagnostic.range.start.line, location.line);
t.is(diagnostic.range.start.character, location.column); t.is(diagnostic.range.start.character, location.column);
t.is(diagnostic.range.end.character, location.endColumn);
t.is(diagnostic.range.end.line, location.endLine);
}); });


test('It translates missing location correctly', (t) => { test('It translates missing location correctly', (t) => {
Expand All @@ -47,3 +51,28 @@ test('It translates missing location correctly', (t) => {
t.is(diagnostic.range.start.line, 0); t.is(diagnostic.range.start.line, 0);
t.is(diagnostic.range.start.character, 0); t.is(diagnostic.range.start.character, 0);
}); });

test('It translates missing endColumn and endLine properties correctly', (t) => {
const location = {
column: 5,
line: 7
};

const problem = {
hintId: 'test-id-1',
location,
message: 'Test Message 1',
severity: Severity.warning
} as Problem;

const diagnostic = problemToDiagnostic(problem);

t.is(diagnostic.source, 'webhint');
t.true(diagnostic.message.indexOf(problem.message || '') !== -1);
t.true(diagnostic.message.indexOf(problem.hintId || '') !== -1);
t.is(diagnostic.severity, DiagnosticSeverity.Warning);
t.is(diagnostic.range.start.line, location.line);
t.is(diagnostic.range.start.character, location.column);
t.is(diagnostic.range.end.character, location.column);
t.is(diagnostic.range.end.line, location.line);
});
20 changes: 6 additions & 14 deletions packages/hint-compat-api/src/css.ts
View file
Expand Up @@ -6,10 +6,10 @@ import intersection = require('lodash/intersection');
import { vendor, AtRule, Rule, Declaration, ChildNode, ContainerBase } from 'postcss'; import { vendor, AtRule, Rule, Declaration, ChildNode, ContainerBase } from 'postcss';


import { HintContext } from 'hint/dist/src/lib/hint-context'; import { HintContext } from 'hint/dist/src/lib/hint-context';
import { IHint, ProblemLocation } from 'hint/dist/src/lib/types'; import { IHint } from 'hint/dist/src/lib/types';
import { StyleEvents } from '@hint/parser-css/dist/src/types'; import { StyleEvents } from '@hint/parser-css/dist/src/types';
import { getUnsupportedDetails, UnsupportedBrowsers } from '@hint/utils-compat-data'; import { getUnsupportedDetails, UnsupportedBrowsers } from '@hint/utils-compat-data';
import { getCSSCodeSnippet } from '@hint/utils/dist/src/report'; import { getCSSCodeSnippet, getCSSLocationFromNode } from '@hint/utils/dist/src/report';


import { formatAlternatives } from './utils/alternatives'; import { formatAlternatives } from './utils/alternatives';
import { filterBrowsers, joinBrowsers } from './utils/browsers'; import { filterBrowsers, joinBrowsers } from './utils/browsers';
Expand All @@ -22,6 +22,7 @@ import { getMessage } from './i18n.import';
type ReportData = { type ReportData = {
feature: string; feature: string;
formatFeature?: (name: string) => string; formatFeature?: (name: string) => string;
isValue?: boolean;
node: ChildNode; node: ChildNode;
unsupported: UnsupportedBrowsers; unsupported: UnsupportedBrowsers;
}; };
Expand All @@ -35,15 +36,6 @@ type Context = {
walk: (ast: ContainerBase, context: Context) => void; walk: (ast: ContainerBase, context: Context) => void;
}; };


const getLocationFromNode = (node: ChildNode): ProblemLocation | undefined => {
const start = node.source && node.source.start;

return start && {
column: start.column - 1,
line: start.line - 1
};
};

const validateAtSupports = (node: AtRule, context: Context): void => { const validateAtSupports = (node: AtRule, context: Context): void => {
const supported = filterSupports(node.params, context.browsers); const supported = filterSupports(node.params, context.browsers);


Expand Down Expand Up @@ -82,7 +74,7 @@ const validateDeclValue = (node: Declaration, context: Context): ReportData | nu
return `${node.prop}: ${value}`; return `${node.prop}: ${value}`;
}; };


return { feature: formatFeature(node.value), formatFeature, node, unsupported }; return { feature: formatFeature(node.value), formatFeature, isValue: true, node, unsupported };
} }


return null; return null;
Expand Down Expand Up @@ -248,13 +240,13 @@ export default class CSSCompatHint implements IHint {
context.on('parse::end::css', ({ ast, element, resource }) => { context.on('parse::end::css', ({ ast, element, resource }) => {
const browsers = filterBrowsers(context.targetedBrowsers); const browsers = filterBrowsers(context.targetedBrowsers);


const report = ({ feature, formatFeature, node, unsupported }: ReportData) => { const report = ({ feature, formatFeature, isValue, node, unsupported }: ReportData) => {
const message = [ const message = [
getMessage('featureNotSupported', context.language, [feature, joinBrowsers(unsupported)]), getMessage('featureNotSupported', context.language, [feature, joinBrowsers(unsupported)]),
...formatAlternatives(context.language, unsupported, formatFeature) ...formatAlternatives(context.language, unsupported, formatFeature)
].join(' '); ].join(' ');
const codeSnippet = getCSSCodeSnippet(node); const codeSnippet = getCSSCodeSnippet(node);
const location = getLocationFromNode(node); const location = getCSSLocationFromNode(node, { isValue });


context.report(resource, message, { codeLanguage: 'css', codeSnippet, element, location }); context.report(resource, message, { codeLanguage: 'css', codeSnippet, element, location });
}; };
Expand Down
24 changes: 12 additions & 12 deletions packages/hint-compat-api/tests/css.ts
View file
Expand Up @@ -42,27 +42,27 @@ testHint(hintPath,
reports: [ reports: [
{ {
message: `'appearance' is not supported by Internet Explorer.`, message: `'appearance' is not supported by Internet Explorer.`,
position: { match: 'appearance: button; /* Report 1 */' } position: { match: 'appearance: button; /* Report 1 */', range: 'appearance' }
}, },
{ {
message: `'appearance' is not supported by Internet Explorer.`, message: `'appearance' is not supported by Internet Explorer.`,
position: { match: 'appearance: button; /* Report 2 */' } position: { match: 'appearance: button; /* Report 2 */', range: 'appearance' }
}, },
{ {
message: `'-webkit-appearance' is not supported by Internet Explorer.`, message: `'-webkit-appearance' is not supported by Internet Explorer.`,
position: { match: '-webkit-appearance: button; /* Report 3 */' } position: { match: '-webkit-appearance: button; /* Report 3 */', range: '-webkit-appearance' }
}, },
{ {
message: `'-moz-appearance' is not supported by Internet Explorer.`, message: `'-moz-appearance' is not supported by Internet Explorer.`,
position: { match: '-moz-appearance: button; /* Report 4 */' } position: { match: '-moz-appearance: button; /* Report 4 */', range: '-moz-appearance' }
}, },
{ {
message: `'-webkit-appearance' is not supported by Firefox, Internet Explorer. Add '-moz-appearance' to support Firefox.`, message: `'-webkit-appearance' is not supported by Firefox, Internet Explorer. Add '-moz-appearance' to support Firefox.`,
position: { match: '-webkit-appearance: button; /* Report 5 */' } position: { match: '-webkit-appearance: button; /* Report 5 */', range: '-webkit-appearance' }
}, },
{ {
message: `'appearance' is not supported by Chrome, Edge, Firefox, Internet Explorer. Add '-webkit-appearance' to support Chrome, Edge 12+. Add '-moz-appearance' to support Firefox.`, message: `'appearance' is not supported by Chrome, Edge, Firefox, Internet Explorer. Add '-webkit-appearance' to support Chrome, Edge 12+. Add '-moz-appearance' to support Firefox.`,
position: { match: 'appearance: button; /* Report 6 */' } position: { match: 'appearance: button; /* Report 6 */', range: 'appearance' }
} }
], ],
serverConfig: generateCSSConfig('properties') serverConfig: generateCSSConfig('properties')
Expand All @@ -86,7 +86,7 @@ testHint(hintPath,
reports: [ reports: [
{ {
message: `'display: grid' is not supported by Edge < 16. Add 'display: -ms-grid' to support Edge 12+.`, message: `'display: grid' is not supported by Edge < 16. Add 'display: -ms-grid' to support Edge 12+.`,
position: { match: 'display: grid; /* Report */' } position: { match: 'grid; /* Report */', range: 'grid' }
} }
], ],
serverConfig: generateCSSConfig('supports') serverConfig: generateCSSConfig('supports')
Expand All @@ -96,19 +96,19 @@ testHint(hintPath,
reports: [ reports: [
{ {
message: `'display: grid' is not supported by Internet Explorer.`, message: `'display: grid' is not supported by Internet Explorer.`,
position: { match: 'display: grid; /* Report 1 */' } position: { match: 'grid; /* Report 1 */', range: 'grid' }
}, },
{ {
message: `'display: grid' is not supported by Internet Explorer.`, message: `'display: grid' is not supported by Internet Explorer.`,
position: { match: 'display: grid; /* Report 2 */' } position: { match: 'grid; /* Report 2 */', range: 'grid' }
}, },
{ {
message: `'display: -ms-grid' is not supported by Chrome, Firefox, Internet Explorer < 10. Add 'display: grid' to support Chrome 57+, Firefox 52+.`, message: `'display: -ms-grid' is not supported by Chrome, Firefox, Internet Explorer < 10. Add 'display: grid' to support Chrome 57+, Firefox 52+.`,
position: { match: 'display: -ms-grid; /* Report 3 */' } position: { match: '-ms-grid; /* Report 3 */', range: '-ms-grid' }
}, },
{ {
message: `'display: grid' is not supported by Edge < 16, Internet Explorer. Add 'display: -ms-grid' to support Edge 12+, Internet Explorer 10+.`, message: `'display: grid' is not supported by Edge < 16, Internet Explorer. Add 'display: -ms-grid' to support Edge 12+, Internet Explorer 10+.`,
position: { match: 'display: grid; /* Report 4 */' } position: { match: 'grid; /* Report 4 */', range: 'grid' }
} }
], ],
serverConfig: generateCSSConfig('values') serverConfig: generateCSSConfig('values')
Expand Down Expand Up @@ -140,7 +140,7 @@ testHint(hintPath,
reports: [ reports: [
{ {
message: `'appearance' is not supported by Internet Explorer.`, message: `'appearance' is not supported by Internet Explorer.`,
position: { match: 'appearance: none; /* unprefixed */' } position: { match: 'appearance: none; /* unprefixed */', range: 'appearance' }
} }
], ],
serverConfig: generateCSSConfig('ignore') serverConfig: generateCSSConfig('ignore')
Expand Down
27 changes: 6 additions & 21 deletions packages/hint-css-prefix-order/src/hint.ts
View file
Expand Up @@ -6,14 +6,15 @@
import { vendor, Declaration, Rule } from 'postcss'; import { vendor, Declaration, Rule } from 'postcss';


import { HintContext } from 'hint/dist/src/lib/hint-context'; import { HintContext } from 'hint/dist/src/lib/hint-context';
import { IHint, ProblemLocation } from 'hint/dist/src/lib/types'; import { IHint } from 'hint/dist/src/lib/types';
import { debug as d } from '@hint/utils/dist/src/debug'; import { debug as d } from '@hint/utils/dist/src/debug';
import { getCSSCodeSnippet } from '@hint/utils/dist/src/report/get-css-code-snippet'; import { getCSSCodeSnippet } from '@hint/utils/dist/src/report/get-css-code-snippet';


import { StyleEvents, StyleParse } from '@hint/parser-css'; import { StyleEvents, StyleParse } from '@hint/parser-css';


import meta from './meta'; import meta from './meta';
import { getMessage } from './i18n.import'; import { getMessage } from './i18n.import';
import { getCSSLocationFromNode } from '@hint/utils/dist/src/report';


const debug: debug.IDebugger = d(__filename); const debug: debug.IDebugger = d(__filename);


Expand All @@ -22,32 +23,15 @@ type DeclarationPair = {
unprefixed: Declaration; unprefixed: Declaration;
}; };


/** Convert `NodeSource` details to a `ProblemLocation`. */
const getLocation = (decl: Declaration): ProblemLocation => {
const start = decl.source && decl.source.start;

if (start) {
return {
column: start.column - 1,
line: start.line - 1
};
}

return {
column: 0,
line: 0
};
};

/** Determine if the order of a prefixed/unprefixed pair is valid. */ /** Determine if the order of a prefixed/unprefixed pair is valid. */
const validatePair = (pair: Partial<DeclarationPair>): boolean => { const validatePair = (pair: Partial<DeclarationPair>): boolean => {
// Valid if only prefixed or only unprefixed versions exist. // Valid if only prefixed or only unprefixed versions exist.
if (!pair.lastPrefixed || !pair.unprefixed) { if (!pair.lastPrefixed || !pair.unprefixed) {
return false; return false;
} }


const prefixedLocation = getLocation(pair.lastPrefixed); const prefixedLocation = getCSSLocationFromNode(pair.lastPrefixed) || { column: 0, line: 0 };
const unprefixedLocation = getLocation(pair.unprefixed); const unprefixedLocation = getCSSLocationFromNode(pair.unprefixed) || { column: 0, line: 0 };


// Valid if last prefixed line is before unprefixed line. // Valid if last prefixed line is before unprefixed line.
if (prefixedLocation.line < unprefixedLocation.line) { if (prefixedLocation.line < unprefixedLocation.line) {
Expand Down Expand Up @@ -126,7 +110,8 @@ export default class CssPrefixOrderHint implements IHint {
ast.walkRules((rule) => { ast.walkRules((rule) => {
for (const invalidPair of validateRule(rule)) { for (const invalidPair of validateRule(rule)) {
const message = formatMessage(invalidPair); const message = formatMessage(invalidPair);
const location = getLocation(invalidPair.unprefixed); const isValue = invalidPair.lastPrefixed.prop === invalidPair.unprefixed.prop;
const location = getCSSLocationFromNode(invalidPair.unprefixed, { isValue });
const codeSnippet = getCSSCodeSnippet(invalidPair.unprefixed); const codeSnippet = getCSSCodeSnippet(invalidPair.unprefixed);


context.report(resource, message, { codeLanguage: 'css', codeSnippet, element, location }); context.report(resource, message, { codeLanguage: 'css', codeSnippet, element, location });
Expand Down
22 changes: 11 additions & 11 deletions packages/hint-css-prefix-order/tests/tests.ts
View file
Expand Up @@ -42,7 +42,7 @@ const tests: HintTest[] = [
name: `Some prefixed properties listed first, but others last fail`, name: `Some prefixed properties listed first, but others last fail`,
reports: [{ reports: [{
message: `'appearance' should be listed after '-webkit-appearance'.`, message: `'appearance' should be listed after '-webkit-appearance'.`,
position: { match: 'appearance: none; /* Report */' } position: { match: 'appearance: none; /* Report */', range: 'appearance' }
}], }],
serverConfig: generateConfig('interleaved-prefixes') serverConfig: generateConfig('interleaved-prefixes')
}, },
Expand All @@ -54,7 +54,7 @@ const tests: HintTest[] = [
name: 'Prefixed properties listed last with other properties mixed in pass', name: 'Prefixed properties listed last with other properties mixed in pass',
reports: [{ reports: [{
message: `'appearance' should be listed after '-webkit-appearance'.`, message: `'appearance' should be listed after '-webkit-appearance'.`,
position: { match: 'appearance: none; /* Report */' } position: { match: 'appearance: none; /* Report */', range: 'appearance' }
}], }],
serverConfig: generateConfig('mixed-with-prefixes-last') serverConfig: generateConfig('mixed-with-prefixes-last')
}, },
Expand All @@ -63,11 +63,11 @@ const tests: HintTest[] = [
reports: [ reports: [
{ {
message: `'appearance' should be listed after '-webkit-appearance'.`, message: `'appearance' should be listed after '-webkit-appearance'.`,
position: { match: 'appearance: none; /* Report 1 */' } position: { match: 'appearance: none; /* Report 1 */', range: 'appearance' }
}, },
{ {
message: `'appearance' should be listed after '-webkit-appearance'.`, message: `'appearance' should be listed after '-webkit-appearance'.`,
position: { match: 'appearance: none; /* Report 2 */' } position: { match: 'appearance: none; /* Report 2 */', range: 'appearance' }
} }
], ],
serverConfig: generateConfig('multi-block') serverConfig: generateConfig('multi-block')
Expand All @@ -77,11 +77,11 @@ const tests: HintTest[] = [
reports: [ reports: [
{ {
message: `'appearance' should be listed after '-webkit-appearance'.`, message: `'appearance' should be listed after '-webkit-appearance'.`,
position: { match: 'appearance: none; /* Report 1 */' } position: { match: 'appearance: none; /* Report 1 */', range: 'appearance' }
}, },
{ {
message: `'background-size' should be listed after '-moz-background-size'.`, message: `'background-size' should be listed after '-moz-background-size'.`,
position: { match: 'background-size: cover; /* Report 2 */' } position: { match: 'background-size: cover; /* Report 2 */', range: 'background-size' }
} }
], ],
serverConfig: generateConfig('multi-property') serverConfig: generateConfig('multi-property')
Expand All @@ -98,7 +98,7 @@ const tests: HintTest[] = [
name: 'Prefixed values listed last fail', name: 'Prefixed values listed last fail',
reports: [{ reports: [{
message: `'display: grid' should be listed after 'display: -ms-grid'.`, message: `'display: grid' should be listed after 'display: -ms-grid'.`,
position: { match: 'display: grid; /* Report */' } position: { match: 'grid; /* Report */', range: 'grid' }
}], }],
serverConfig: generateConfig('prefixed-values-last') serverConfig: generateConfig('prefixed-values-last')
}, },
Expand All @@ -114,15 +114,15 @@ const tests: HintTest[] = [
name: 'Prefixed properties listed last fail (moz)', name: 'Prefixed properties listed last fail (moz)',
reports: [{ reports: [{
message: `'appearance' should be listed after '-moz-appearance'.`, message: `'appearance' should be listed after '-moz-appearance'.`,
position: { match: 'appearance: none; /* Report */' } position: { match: 'appearance: none; /* Report */', range: 'appearance' }
}], }],
serverConfig: generateConfig('prefixes-last-moz') serverConfig: generateConfig('prefixes-last-moz')
}, },
{ {
name: 'Prefixed properties listed last on same line fail', name: 'Prefixed properties listed last on same line fail',
reports: [{ reports: [{
message: `'appearance' should be listed after '-webkit-appearance'.`, message: `'appearance' should be listed after '-webkit-appearance'.`,
position: { match: 'appearance: none; /* Report */' } position: { match: 'appearance: none; /* Report */', range: 'appearance' }
}], }],
serverConfig: generateConfig('prefixes-last-same-line') serverConfig: generateConfig('prefixes-last-same-line')
}, },
Expand All @@ -141,7 +141,7 @@ const tests: HintTest[] = [
name: 'Prefixed properties listed last fail (webkit)', name: 'Prefixed properties listed last fail (webkit)',
reports: [{ reports: [{
message: `'appearance' should be listed after '-webkit-appearance'.`, message: `'appearance' should be listed after '-webkit-appearance'.`,
position: { match: 'appearance: none; /* Report */' } position: { match: 'appearance: none; /* Report */', range: 'appearance' }
}], }],
serverConfig: generateConfig('prefixes-last-webkit') serverConfig: generateConfig('prefixes-last-webkit')
}, },
Expand All @@ -164,7 +164,7 @@ const tests: HintTest[] = [
name: 'Prefixed properties in nested blocks only report once', name: 'Prefixed properties in nested blocks only report once',
reports: [{ reports: [{
message: `'appearance' should be listed after '-webkit-appearance'.`, message: `'appearance' should be listed after '-webkit-appearance'.`,
position: { match: 'appearance: none' } position: { match: 'appearance: none', range: 'appearance' }
}], }],
serverConfig: generateConfig('prefixes-nested-blocks.scss') serverConfig: generateConfig('prefixes-nested-blocks.scss')
} }
Expand Down
1 change: 0 additions & 1 deletion packages/hint-scoped-svg-styles/package.json
View file
Expand Up @@ -23,7 +23,6 @@
"eslint-plugin-markdown": "^1.0.0", "eslint-plugin-markdown": "^1.0.0",
"npm-run-all": "^4.1.5", "npm-run-all": "^4.1.5",
"nyc": "^14.1.0", "nyc": "^14.1.0",
"postcss": "^7.0.18",
"rimraf": "^3.0.0", "rimraf": "^3.0.0",
"typescript": "^3.6.4" "typescript": "^3.6.4"
}, },
Expand Down

0 comments on commit de43df9

Please sign in to comment.