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

Support options in aria-allowed-attr and aria-required-attr #673

Merged
merged 8 commits into from
Jan 18, 2018
Merged

Support options in aria-allowed-attr and aria-required-attr #673

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
66646a8
chore: rename lut in tests
Jan 5, 2018
cf9af49
feat: allow options in aria-allowed-attr
Jan 5, 2018
e30a372
chore: rename options integ. tests for clarity
Jan 6, 2018
4f266c7
feat: add required-attr options, integration tests
Jan 6, 2018
69fba9d
chore: PR feedback for aria-allowed-attr
Jan 6, 2018
fd6d671
feat: use object for ARIA check options
Jan 10, 2018
86fd5bf
chore: remove duplicate to-array util
Jan 18, 2018
a51514d
chore: simplify ARIA options usage
Jan 18, 2018
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
4 changes: 3 additions & 1 deletion lib/checks/aria/allowed-attr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
options = Array.isArray(options) ? options : [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if this should allow per role specification, instead of (or in addition to) a generic "allowed everywhere". So you could do: { separator: ['aria-valuenow', 'aria-valuemin', 'aria-valuemax'] }. You could still allow the array, and you could add a wildcard option for the "generic" case you've got now: { '*': ['always-allowed'] }.


var invalid = [];

var attr, attrName, allowed,
Expand All @@ -12,7 +14,7 @@ if (role && allowed) {
for (var i = 0, l = attrs.length; i < l; i++) {
attr = attrs[i];
attrName = attr.name;
if (axe.commons.aria.validateAttr(attrName) && allowed.indexOf(attrName) === -1) {
if (!options.includes(attrName) && axe.commons.aria.validateAttr(attrName) && allowed.indexOf(attrName) === -1) {
invalid.push(attrName + '="' + attr.nodeValue + '"');
}
}
Expand Down
11 changes: 11 additions & 0 deletions lib/checks/aria/required-attr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,21 @@
options = Array.isArray(options) ? options : [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my comment above. This is a generic "required everywhere". That seems a bit useless. You wouldn't require a particular attribute on every element. You'd need to set certain things to be required for certain roles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks for the tip!


var uniqueArray = (arrArg) => {
return arrArg.filter((elem, pos, arr) => {
return arr.indexOf(elem) === pos;
});
};

var missing = [];

if (node.hasAttributes()) {
var attr,
role = node.getAttribute('role'),
required = axe.commons.aria.requiredAttr(role);

if (options.length) {
required = uniqueArray(required.concat(options));
}
if (role && required) {
for (var i = 0, l = required.length; i < l; i++) {
attr = required[i];
Expand Down
7 changes: 7 additions & 0 deletions test/checks/aria/allowed-attr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,11 @@ describe('aria-allowed-attr', function () {
assert.isNull(checkContext._data);
});

describe('options', function () {
it('should allow provided attribute names', function () {
fixture.innerHTML = '<div role="separator" id="target" aria-valuenow="0" aria-valuemin="-2" aria-valuemax="4"></div>';
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to use fake aria properties instead. Otherwise, once we've added those values, this test becomes invalid.

var target = fixture.children[0];
assert.isTrue(checks['aria-allowed-attr'].evaluate.call(checkContext, target, ['aria-valuenow', 'aria-valuemin', 'aria-valuemax']));
});
});
});
8 changes: 8 additions & 0 deletions test/checks/aria/required-attr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,12 @@ describe('aria-required-attr', function () {
axe.commons.aria.requiredAttr = orig;
});

describe('options', function () {
it('should require provided attribute names', function () {
fixture.innerHTML = '<div role="slider" id="target"></div>';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, you should use a made up role. May I suggest role="McCheddarton"? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Love it! will do.

var target = fixture.children[0];
assert.isFalse(checks['aria-required-attr'].evaluate.call(checkContext, target, ['aria-valuemax', 'aria-bats']));
assert.deepEqual(checkContext._data, ['aria-valuenow', 'aria-valuemax', 'aria-valuemin', 'aria-bats']);
});
});
});
6 changes: 3 additions & 3 deletions test/commons/aria/roles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe('aria.isValidRole', function () {

});

it('should return false if role is not found in the lut', function () {
it('should return false if role is not found in the lookup table', function () {
assert.isFalse(axe.commons.aria.isValidRole('cats'));

});
Expand Down Expand Up @@ -57,7 +57,7 @@ describe('aria.getRolesByType', function () {

});

it('should return empty array if role is not found in the lut', function () {
it('should return empty array if role is not found in the lookup table', function () {
assert.deepEqual(axe.commons.aria.getRolesByType('blahblahblah'), []);
});
});
Expand All @@ -77,7 +77,7 @@ describe('aria.getRoleType', function () {

});

it('should return null if role is not found in the lut', function () {
it('should return null if role is not found in the lookup table', function () {
assert.isNull(axe.commons.aria.getRoleType('cats'));
});
});
Expand Down
25 changes: 25 additions & 0 deletions test/integration/full/configure-options/configure-options.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<!doctype html>
<html lang="en" id="main">
<head>
<title></title>
<meta charset="utf8">
<link rel="stylesheet" type="text/css" href="/node_modules/mocha/mocha.css" />
<script src="/node_modules/mocha/mocha.js"></script>
<script src="/node_modules/chai/chai.js"></script>
<script src="/axe.js"></script>
<script>
mocha.setup({
timeout: 50000,
ui: 'bdd'
});
var assert = chai.assert;
</script>
</head>
<body>
<div id="target"></div>

<div id="mocha"></div>
<script src="configure-options.js"></script>
<script src="/test/integration/adapter.js"></script>
</body>
</html>
50 changes: 50 additions & 0 deletions test/integration/full/configure-options/configure-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
describe('Check Configure Options', function() {
'use strict';

var target = document.querySelector('#target');

describe('aria-allowed-attr', function() {
it('should allow an attribute supplied in options', function(done) {
target.setAttribute('role', 'separator');
target.setAttribute('aria-valuenow', '0');
Copy link
Contributor

Choose a reason for hiding this comment

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

In the event that 'aria-valuenow' becomes an allowed attribute in our lookup table, this test won't be testing anything anymore. Just to make the test more future-proof, I suggest using a random value like 'peanutbutter' instead of a real attribute so we can guarantee the test will always be testing the override functionality with a truly unexpected value.


axe.configure({
checks: [{
id: 'aria-allowed-attr',
options: ['aria-valuenow']
}]
});
axe.run(target, {
runOnly: {
type: 'rule',
values: [ 'aria-allowed-attr' ]
}
}, function(error, results) {
assert.lengthOf(results.violations, 0, 'violations');
done();
});
});
});

describe('aria-required-attr', function() {
it('should report unique attributes when supplied from options', function(done) {
target.setAttribute('role', 'slider');
axe.configure({
checks: [{
id: 'aria-required-attr',
options: ['aria-checked']
}]
});
axe.run('#target', {
runOnly: {
type: 'rule',
values: [ 'aria-required-attr' ]
}
}, function(error, results) {
assert.lengthOf(results.violations, 1, 'violations');
assert.sameMembers(results.violations[0].nodes[0].any[0].data, ['aria-valuemax', 'aria-valuemin', 'aria-checked']);
done();
});
});
});
});
0 ...ntegration/full/options/frames/frame.html → .../full/options-parameter/frames/frame.html
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion test/integration/full/options/options.html → .../options-parameter/options-parameter.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
</div>

<div id="mocha"></div>
<script src="options.js"></script>
<script src="options-parameter.js"></script>
<script src="/test/integration/adapter.js"></script>
</body>
</html>
2 changes: 1 addition & 1 deletion test/integration/full/options/options.js → ...ll/options-parameter/options-parameter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
describe('Options', function() {
describe('Options parameter', function() {
'use strict';

before(function (done) {
Expand Down