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

fix(checks): do not normalize options for custom checks #2435

Merged
merged 1 commit into from
Aug 4, 2020
Merged

fix(checks): do not normalize options for custom checks #2435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions lib/core/base/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import Rule from './rule';
import Check from './check';
import standards from '../../standards';
import metadataFunctionMap from './metadata-function-map';
import RuleResult from './rule-result';
import {
clone,
Expand Down Expand Up @@ -157,7 +156,6 @@ class Audit {
this.lang = 'en';
this.defaultConfig = audit;
this.standards = standards;
this.metadataFunctionMap = metadataFunctionMap;
this._init();
// A copy of the "default" locale. This will be set if the user
// provides a new locale to `axe.configure()` and used to undo
Expand Down
22 changes: 19 additions & 3 deletions lib/core/base/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,21 +180,37 @@ Check.prototype.runSync = function(node, options, context) {
*/

Check.prototype.configure = function(spec) {
// allow test specs (without evaluate functions) to work as
// internal checks
if (!spec.evaluate || metadataFunctionMap[spec.evaluate]) {
this._internalCheck = true;
}

if (spec.hasOwnProperty('enabled')) {
this.enabled = spec.enabled;
}

if (spec.hasOwnProperty('options')) {
this.options = normalizeOptions(spec.options);
// only normalize options for internal checks
if (this._internalCheck) {
this.options = normalizeOptions(spec.options);
} else {
this.options = spec.options;
}
}

['evaluate', 'after']
.filter(prop => spec.hasOwnProperty(prop))
.forEach(prop => (this[prop] = createExecutionContext(spec[prop])));
};

Check.prototype.getOptions = function getOptions(options = {}) {
return deepMerge(this.options, normalizeOptions(options));
Check.prototype.getOptions = function getOptions(options) {
// only merge and normalize options for internal checks
if (this._internalCheck) {
Comment on lines +208 to +209
Copy link
Contributor

@WilcoFiers WilcoFiers Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think we allow deep merge for any non-array object

Suggested change
// only merge and normalize options for internal checks
if (this._internalCheck) {
// only merge and normalize options for internal checks
if (typeof this.options === 'object' && !Array.isArray(this.options)) {

return deepMerge(this.options, normalizeOptions(options || {}));
} else {
return options || this.options;
}
};

export default Check;
4 changes: 3 additions & 1 deletion lib/core/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import Audit from './base/audit';
import CheckResult from './base/check-result';
import Check from './base/check';
import Context from './base/context';
import metadataFunctionMap from './base/metadata-function-map';
import RuleResult from './base/rule-result';
import Rule from './base/rule';

Expand Down Expand Up @@ -53,7 +54,8 @@ axe._thisWillBeDeletedDoNotUse.base = {
Check,
Context,
RuleResult,
Rule
Rule,
metadataFunctionMap
};

axe.imports = imports;
Expand Down
54 changes: 42 additions & 12 deletions test/core/base/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ describe('Check', function() {

var Check = axe._thisWillBeDeletedDoNotUse.base.Check;
var CheckResult = axe._thisWillBeDeletedDoNotUse.base.CheckResult;
var metadataFunctionMap =
axe._thisWillBeDeletedDoNotUse.base.metadataFunctionMap;
var noop = function() {};

var fixture = document.getElementById('fixture');
Expand Down Expand Up @@ -99,8 +101,7 @@ describe('Check', function() {
delete Check.prototype.test;
});
it('should override evaluate as ID', function() {
axe._load({});
axe._audit.metadataFunctionMap['custom-evaluate'] = function() {
metadataFunctionMap['custom-evaluate'] = function() {
return 'fong';
};

Expand All @@ -113,11 +114,10 @@ describe('Check', function() {
check.configure({ evaluate: 'custom-evaluate' });
assert.equal('fong', check.test());
delete Check.prototype.test;
delete axe._audit.metadataFunctionMap['custom-evaluate'];
delete metadataFunctionMap['custom-evaluate'];
});
it('should override after as ID', function() {
axe._load({});
axe._audit.metadataFunctionMap['custom-after'] = function() {
metadataFunctionMap['custom-after'] = function() {
return 'fong';
};

Expand All @@ -130,7 +130,7 @@ describe('Check', function() {
check.configure({ after: 'custom-after' });
assert.equal('fong', check.test());
delete Check.prototype.test;
delete axe._audit.metadataFunctionMap['custom-after'];
delete metadataFunctionMap['custom-after'];
});
it('should error if evaluate does not match an ID', function() {
function fn() {
Expand Down Expand Up @@ -226,10 +226,21 @@ describe('Check', function() {
}).run(fixture, { options: expected }, {}, noop);
});

it('should normalize non-object options', function(done) {
it('should normalize non-object options for internal checks', function(done) {
metadataFunctionMap['custom-check'] = function(node, options) {
assert.deepEqual(options, { value: 'foo' });
done();
};
new Check({
evaluate: 'custom-check'
}).run(fixture, { options: 'foo' }, {}, noop);
delete metadataFunctionMap['custom-check'];
});

it('should not normalize non-object options for external checks', function(done) {
new Check({
evaluate: function(node, options) {
assert.deepEqual(options, { value: 'foo' });
assert.deepEqual(options, 'foo');
done();
}
}).run(fixture, { options: 'foo' }, {}, noop);
Expand Down Expand Up @@ -388,13 +399,24 @@ describe('Check', function() {
}).runSync(fixture, { options: expected }, {});
});

it('should normalize non-object options', function(done) {
it('should normalize non-object options for internal checks', function(done) {
metadataFunctionMap['custom-check'] = function(node, options) {
assert.deepEqual(options, { value: 'foo' });
done();
};
new Check({
evaluate: 'custom-check'
}).runSync(fixture, { options: 'foo' }, {});
delete metadataFunctionMap['custom-check'];
});

it('should not normalize non-object options for external checks', function(done) {
new Check({
evaluate: function(node, options) {
assert.deepEqual(options, { value: 'foo' });
assert.deepEqual(options, 'foo');
done();
}
}).run(fixture, { options: 'foo' }, {}, noop);
}).runSync(fixture, { options: 'foo' }, {});
});

it('should pass the context through to check evaluate call', function() {
Expand Down Expand Up @@ -569,12 +591,20 @@ describe('Check', function() {
assert.equal(new Check(spec).options, spec.options);
});

it('should normalize non-object options', function() {
it('should normalize non-object options for internal checks', function() {
var spec = {
options: 'foo'
};
assert.deepEqual(new Check(spec).options, { value: 'foo' });
});

it('should not normalize non-object options for external checks', function() {
var spec = {
options: 'foo',
evaluate: function() {}
};
assert.deepEqual(new Check(spec).options, 'foo');
});
});

describe('.evaluate', function() {
Expand Down
10 changes: 6 additions & 4 deletions test/core/base/rule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ describe('Rule', function() {

var Rule = axe._thisWillBeDeletedDoNotUse.base.Rule;
var Check = axe._thisWillBeDeletedDoNotUse.base.Check;
var metadataFunctionMap =
axe._thisWillBeDeletedDoNotUse.base.metadataFunctionMap;
var fixture = document.getElementById('fixture');
var noop = function() {};
var isNotCalled = function(err) {
Expand Down Expand Up @@ -1940,10 +1942,10 @@ describe('Rule', function() {
});
it('should override matches (metadata function name)', function() {
axe._load({});
axe._audit.metadataFunctionMap['custom-matches'] = function() {
metadataFunctionMap['custom-matches'] = function() {
return 'custom-matches';
};
axe._audit.metadataFunctionMap['other-matches'] = function() {
metadataFunctionMap['other-matches'] = function() {
return 'other-matches';
};

Expand All @@ -1953,8 +1955,8 @@ describe('Rule', function() {
rule.configure({ matches: 'other-matches' });
assert.equal(rule._get('matches')(), 'other-matches');

delete axe._audit.metadataFunctionMap['custom-matches'];
delete axe._audit.metadataFunctionMap['other-matches'];
delete metadataFunctionMap['custom-matches'];
delete metadataFunctionMap['other-matches'];
});
it('should error if matches does not match an ID', function() {
function fn() {
Expand Down
66 changes: 64 additions & 2 deletions test/integration/full/configure-options/configure-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,68 @@ describe('Configure Options', function() {
}
);
});

it('should not normalize external check options', function(done) {
target.setAttribute('lang', 'en');

axe.configure({
checks: [
{
id: 'dylang',
options: ['dylan'],
evaluate:
'function (node, options) {\n var lang = (node.getAttribute("lang") || "").trim().toLowerCase();\n var xmlLang = (node.getAttribute("xml:lang") || "").trim().toLowerCase();\n var invalid = [];\n (options || []).forEach(function(cc) {\n cc = cc.toLowerCase();\n if (lang && (lang === cc || lang.indexOf(cc.toLowerCase() + "-") === 0)) {\n lang = null;\n }\n if (xmlLang && (xmlLang === cc || xmlLang.indexOf(cc.toLowerCase() + "-") === 0)) {\n xmlLang = null;\n }\n });\n if (xmlLang) {\n invalid.push(\'xml:lang="\' + xmlLang + \'"\');\n }\n if (lang) {\n invalid.push(\'lang="\' + lang + \'"\');\n }\n if (invalid.length) {\n this.data(invalid);\n return true;\n }\n return false;\n }',
messages: {
pass: 'Good language',
fail: 'You mst use the DYLAN language'
}
}
],
rules: [
{
id: 'dylang',
metadata: {
description:
"Ensures lang attributes have the value of 'dylan'",
help: "lang attribute must have the value of 'dylan'"
},
selector: '#target',
any: [],
all: [],
none: ['dylang'],
tags: ['wcag2aa']
}
],
data: {
rules: {
dylang: {
description:
"Ensures lang attributes have the value of 'dylan'",
help: "lang attribute must have the value of 'dylan'"
}
}
}
});

axe.run(
'#target',
{
runOnly: {
type: 'rule',
values: ['dylang']
}
},
function(err, results) {
try {
assert.isNull(err);
assert.lengthOf(results.violations, 1, 'violations');
done();
} catch (e) {
done(e);
}
}
);
});
});

describe('aria-required-attr', function() {
Expand Down Expand Up @@ -67,8 +129,8 @@ describe('Configure Options', function() {
});
});

describe('disableOtherRules', function(done) {
it('disables rules that are not in the `rules` array', function() {
describe('disableOtherRules', function() {
it('disables rules that are not in the `rules` array', function(done) {
axe.configure({
disableOtherRules: true,
rules: [
Expand Down