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

Do not warn about nesting css selectors when firefoxStrictMinVersion is >= 117 #5164

Merged
merged 11 commits into from
Feb 9, 2024
3 changes: 2 additions & 1 deletion src/messages/css.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ export const CSS_SYNTAX_ERROR = {
export const INVALID_SELECTOR_NESTING = {
code: 'INVALID_SELECTOR_NESTING',
message: i18n._('Invalid nesting of selectors found'),
description: i18n._(`Selectors should not be nested`),
description: i18n._(`Selector nesting is supported from firefox version 117.0
and above`),
};
17 changes: 17 additions & 0 deletions src/parsers/manifestjson.js
Original file line number Diff line number Diff line change
Expand Up @@ -1285,17 +1285,34 @@ export default class ManifestJSONParser extends JSONParser {
return apiPaths;
}

/**
* @typedef {Object} Metadata
* @property {string} id
* @property {number} manifestVersion
* @property {string} name
* @property {number} type
* @property {string} version
* @property {string} firefoxMinVersion
* @property {string} firefoxStrictMinVersion
* @property {Set<string>} experimentApiPaths
*
* @returns {Metadata}
*/
getMetadata() {
return {
id: this.getAddonId(),
manifestVersion: this.parsedJSON.manifest_version,
name: this.parsedJSON.name,
type: PACKAGE_EXTENSION,
version: this.parsedJSON.version,
// This is the `strict_min_version` value set in the `manifest.json` file
// for Firefox for desktop.
firefoxMinVersion:
this.parsedJSON.applications &&
this.parsedJSON.applications.gecko &&
this.parsedJSON.applications.gecko.strict_min_version,
// This is the strict min *major* version for Firefox for desktop.
firefoxStrictMinVersion: firefoxStrictMinVersion(this.parsedJSON),
experimentApiPaths: this.getExperimentApiPaths(),
};
}
Expand Down
16 changes: 15 additions & 1 deletion src/rules/css/invalidNesting.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,24 @@
import * as messages from 'messages';

import { basicCompatVersionComparison } from '../../utils';

const CSS_NESTING_MIN_VERSION = 117;

export function invalidNesting(
cssNode,
filename,
{ startLine, startColumn } = {}
{ startLine, startColumn, addonMetadata } = {}
) {
if (
addonMetadata?.firefoxStrictMinVersion &&
!basicCompatVersionComparison(
CSS_NESTING_MIN_VERSION,
addonMetadata?.firefoxStrictMinVersion
)
) {
return [];
}

const messageList = [];
if (cssNode.type === 'rule') {
for (let i = 0; i < cssNode.nodes.length; i++) {
Expand Down
12 changes: 12 additions & 0 deletions src/scanners/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ export default class BaseScanner {
throw new Error('scannerName is not implemented');
}

/**
* @typedef {Object} ScannerOptions
* @property {import('../parsers/manifestjson').Metadata} addonMetadata
* @property {import('../collector').default} collector
* @property {string[]} disabledRules
* @property {string[]} existingFiles
* @property {boolean} privileged
*
* @param {string|Buffer|import('stream').Readable} contents
* @param {string} filename
* @param {ScannerOptions} options
*/
Copy link
Member

Choose a reason for hiding this comment

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

That kinda looks like typescript stuff, is this really needed as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These kind of JSDoc comments are the compromise of having types without using typescript. It helps documenting interfaces, allowing IDEs with code suggestions and helping new devs to faster move around foreign code. I typed parts where I struggled with the given interfaces and needed to dig deeper and documented it right away, so others don't have to. Is this kind of type-documenting style unwelcome in this repository? I can definitely just remove it.

Copy link
Member

Choose a reason for hiding this comment

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

see other comment

constructor(contents, filename, options = {}) {
this.contents = contents;
this.filename = filename;
Expand Down
5 changes: 5 additions & 0 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,11 @@ export function androidStrictMinVersion(manifestJson) {
return version;
}

/**
* @param {*} versionAdded
* @param {number} minVersion
* @returns {boolean} true if versionAdded has a strictly greater major version than minVersion
*/
export function basicCompatVersionComparison(versionAdded, minVersion) {
const asNumber = parseInt(versionAdded, 10);
return !Number.isNaN(asNumber) && asNumber > minVersion;
Expand Down
46 changes: 36 additions & 10 deletions tests/unit/rules/css/test.invalidNesting.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,48 @@ import { VALIDATION_WARNING } from 'const';
import CSSScanner from 'scanners/css';

describe('CSS Rule InvalidNesting', () => {
it('should detect invalid nesting', async () => {
const code = oneLine`/* I'm a comment */
it.each([60, 116])(
'should detect invalid nesting when firefoxStrictMinVersion=%s',
async (firefoxStrictMinVersion) => {
const code = oneLine`/* I'm a comment */
#something {
.bar {
height: 100px;
}
}`;
const cssScanner = new CSSScanner(code, 'fakeFile.css');
const cssScanner = new CSSScanner(code, 'fakeFile.css', {
addonMetadata: {
firefoxStrictMinVersion,
},
});

const { linterMessages } = await cssScanner.scan();
expect(linterMessages.length).toEqual(1);
expect(linterMessages[0].code).toEqual(
messages.INVALID_SELECTOR_NESTING.code
);
expect(linterMessages[0].type).toEqual(VALIDATION_WARNING);
});
const { linterMessages } = await cssScanner.scan();
expect(linterMessages.length).toEqual(1);
expect(linterMessages[0].code).toEqual(
messages.INVALID_SELECTOR_NESTING.code
);
expect(linterMessages[0].type).toEqual(VALIDATION_WARNING);
}
);

it.each([117, 118])(
'should not report invalid nesting when firefoxStrictMinVersion=%s',
async (firefoxStrictMinVersion) => {
const code = oneLine`/* I'm a comment */
#something {
.bar {
height: 100px;
}
}`;
const cssScanner = new CSSScanner(code, 'fakeFile.css', {
addonMetadata: {
firefoxStrictMinVersion,
},
});
const { linterMessages } = await cssScanner.scan();
expect(linterMessages.length).toEqual(0);
}
);

it('should not detect invalid nesting', async () => {
const code = oneLine`/* I'm a comment */
Expand Down
4 changes: 4 additions & 0 deletions tests/unit/test.utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,10 @@ describe('basicCompatVersionComparison', () => {
expect(basicCompatVersionComparison('61', 60)).toBe(true);
});

it('should return false when version added is equal to min version', () => {
expect(basicCompatVersionComparison('61.5.2', 61)).toBe(false);
});

it('should return false when version added is smaller than min version', () => {
expect(basicCompatVersionComparison('59', 60)).toBe(false);
});
Expand Down