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

feat(landmark-one-main): add rule ensuring one main landmark in document #498

Merged
merged 21 commits into from
Nov 19, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
59e1844
feat(landmark-one-main): add rule ensuring one main landmark in document
sulsanaul Aug 23, 2017
4737cbd
fix: rename integration tests
sulsanaul Aug 30, 2017
ac0c517
fix: move checks from 'any' to 'all'
sulsanaul Aug 30, 2017
d80efbd
Merge https://github.com/dequelabs/axe-core into main/only-one
sulsanaul Sep 18, 2017
456415c
fix: add missing 'after' check
sulsanaul Sep 26, 2017
337d129
Merge https://github.com/dequelabs/axe-core into main/only-one
sulsanaul Sep 26, 2017
c0f2efb
fix: update to pass virtualnode in to check
sulsanaul Sep 26, 2017
ff6417b
fix: change incorrect rule name in integration tests
sulsanaul Oct 10, 2017
4319175
fix: remove line for debugging
sulsanaul Oct 10, 2017
f1d1b2f
fix: correct faulty check tests
sulsanaul Oct 10, 2017
93e8f11
test: add shadowCheckSetup util
Oct 10, 2017
0319db8
test: fix main tests for shadowdom
Oct 10, 2017
5562767
Merge https://github.com/dequelabs/axe-core into main/only-one
sulsanaul Oct 18, 2017
8640681
feat: add shadow support
sulsanaul Oct 18, 2017
59e6d16
fix: resolve timeout issues
sulsanaul Oct 18, 2017
26ee38b
fix: add event listener
sulsanaul Oct 20, 2017
47c7e66
fix: change where to check for passes
sulsanaul Oct 20, 2017
a3beee1
style: comment code for comprehension
sulsanaul Nov 16, 2017
6359af1
test: add test for second violation node
sulsanaul Nov 17, 2017
823ef51
Merge branch 'develop' into main/only-one
WilcoFiers Nov 19, 2017
5ed8ccf
Merge branch 'develop' into main/only-one
WilcoFiers Nov 19, 2017
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
3 changes: 3 additions & 0 deletions lib/checks/keyboard/has-at-least-one-main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
var main = document.querySelector('main,[role=main]');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do the following instead. That way you'll query shadow DOM trees too.

const main = axe.utils.querySelectorAll(virtualNode, 'main,[role=main')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What purpose does adding 'virtualnode' serve? I am getting errors concerning an undefined object.

Copy link

Choose a reason for hiding this comment

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

The change to use "const main = axe.utils.querySelectorAll(virtualNode, 'main,[role=main')" seems to break all our test cases and we get the following error:

"TypeError: Cannot read property 'length' of undefined"

Is there something we need to change in the test cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jongund When you call the check in a test, and that test uses virtualNode, you need to pass a virtualNode object in. We've got a test utility to help set that up on axe.testUtils.checkSetup. You can see an example here:

https://github.com/dequelabs/axe-core/blob/develop/test/checks/aria/required-children.js#L22

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't appear to be possible to test the rule on an 'html' node using checkSetup. Do you have any suggestions on how to go about doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgive me for trying to catch up on this PR...but why do you need to test the HTML node for a main rule? Tests are scoped to the #fixture element, which is already present in the test environment, to avoid auditing the test harness UI itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The selector of the rule itself is 'html' so that the rule runs even if there are no main landmarks. My thought was that it wouldn't really be a proper test if it didn't begin from the specified node.

this.data(!!main);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tells the after function whether or not the main exists; if there's a main in any one document, all of the documents will pass. This will probably make more sense when I add the file for the after function.

return !!main;
12 changes: 12 additions & 0 deletions lib/checks/keyboard/has-at-least-one-main.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"id": "has-at-least-one-main",
"evaluate": "has-at-least-one-main.js",
"after": "has-at-least-one-main-after.js",
"metadata": {
"impact": "moderate",
"messages": {
"pass": "Document has at least one main landmark",
"fail": "Document has no main landmarks"
}
}
}
2 changes: 2 additions & 0 deletions lib/checks/keyboard/has-no-more-than-one-main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
var mains = document.querySelectorAll('main,[role=main]');
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, use axe.utils.querySelectorAll instead, so you can query shadow DOM trees.

return mains.length<=1;
11 changes: 11 additions & 0 deletions lib/checks/keyboard/has-no-more-than-one-main.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"id": "has-no-more-than-one-main",
"evaluate": "has-no-more-than-one-main.js",
"metadata": {
"impact": "moderate",
"messages": {
"pass": "Document has no more than one main landmark",
"fail": "Document has more than one main landmark"
}
}
}
17 changes: 17 additions & 0 deletions lib/rules/landmark-one-main.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"id": "landmark-at-least-one-main",
"selector": "html",
"tags": [
"best-practice"
],
"metadata": {
"description": "Ensures a navigation point to the primary content of the page. If the page contains iframes, each iframe should contain either no main landmarks or just one.",
"help": "Page must contain one main landmark."
},
"all": [],
"any": [
"has-at-least-one-main",
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be on all. Putting them on any just requires one of them to pass, which it always will.

"has-no-more-than-one-main"
],
"none": []
}
88 changes: 88 additions & 0 deletions test/checks/keyboard/has-at-least-one-main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
describe('has-at-least-one-main', function () {
'use strict';

var fixture = document.getElementById('fixture');
var checkContext = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using var checkContext = new axe.testUtils.MockCheckContext();

_data: null,
data: function (d) {
this._data = d;
},
};

afterEach(function () {
fixture.innerHTML = '';
checkContext._data = null;
});


it('should return false if main does not exist', function () {
var node = document.querySelector('html');
var mainIsFound = checks['has-at-least-one-main'].evaluate.call(checkContext, node);
assert.isFalse(mainIsFound);
assert.equal(checkContext._data, mainIsFound);
});

it('should return false if no div has role property', function() {
var node = document.querySelector('html');
var notMain = document.createElement('div');
node.appendChild(notMain);
var mainIsFound = checks['has-at-least-one-main'].evaluate.call(checkContext, node);
assert.isFalse(mainIsFound);
assert.equal(checkContext._data, mainIsFound);
node.removeChild(notMain);
});

it('should return false if div has empty role', function() {
var node = document.querySelector('html');
var notMain = document.createElement('div');
notMain.setAttribute('role','');
node.appendChild(notMain);
var mainIsFound = checks['has-at-least-one-main'].evaluate.call(checkContext, node);
assert.isFalse(mainIsFound);
assert.equal(checkContext._data, mainIsFound);
node.removeChild(notMain);
});

it('should return false if div has role not equal to main', function() {
var node = document.querySelector('html');
var notMain = document.createElement('div');
notMain.setAttribute('role','bananas');
node.appendChild(notMain);
var mainIsFound = checks['has-at-least-one-main'].evaluate.call(checkContext, node);
assert.isFalse(mainIsFound);
assert.equal(checkContext._data, mainIsFound);
node.removeChild(notMain);
});

it('should return true if main landmark exists', function(){
var node = document.querySelector('html');
var mainLandmark = document.createElement('main');
node.appendChild(mainLandmark);
var mainIsFound = checks['has-at-least-one-main'].evaluate.call(checkContext, node);
assert.isTrue(mainIsFound);
assert.equal(checkContext._data, mainIsFound);
node.removeChild(mainLandmark);
});

it('should return true if one div has role equal to main', function() {
var node = document.querySelector('html');
var mainLandmark = document.createElement('div');
mainLandmark.setAttribute('role','main');
node.appendChild(mainLandmark);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to append these to the fixture instead. Saves you a few lines of code + it's guaranteed to be cleaned up with afterEach.

var mainIsFound = checks['has-at-least-one-main'].evaluate.call(checkContext, node);
assert.isTrue(mainIsFound);
assert.equal(checkContext._data, mainIsFound);
node.removeChild(mainLandmark);
});

it('should return true if any document has a main landmark', function() {
var results = [{data: false, result: false}, {data: true, result: true}];
assert.isTrue(checks['has-at-least-one-main'].after(results)[0].result && checks['has-at-least-one-main'].after(results)[1].result);
});

it('should return false if no document has a main landmark', function() {
var results = [{data: false, result: false}, {data: false, result: false}];
assert.isFalse(checks['has-at-least-one-main'].after(results)[0].result && checks['has-at-least-one-main'].after(results)[1].result);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a shadow DOM test too. Have a look at the develop docs https://github.com/dequelabs/axe-core/blob/develop/doc/developer-guide.md#test-utilities about testUtils.shadowSupport

});
51 changes: 51 additions & 0 deletions test/checks/keyboard/has-no-more-than-one-main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
describe('has-no-more-than-one-main', function () {
'use strict';

var fixture = document.getElementById('fixture');

afterEach(function () {
fixture.innerHTML = '';
});

it('should return false if there is more than one element with role main', function () {
var mainDiv = document.createElement('div');
mainDiv.setAttribute('role','main');
var anotherMain = document.createElement('div');
anotherMain.setAttribute('role','main');
var node = document.querySelector('html');
node.appendChild(mainDiv);
node.appendChild(anotherMain);
assert.isFalse(checks['has-no-more-than-one-main'].evaluate(node));
node.removeChild(mainDiv);
node.removeChild(anotherMain);
});

it('should return false if there is more than one main element', function () {
var mainLandmark = document.createElement('main');
var anotherMain = document.createElement('main');
var node = document.querySelector('html');
node.appendChild(mainLandmark);
node.appendChild(anotherMain);
assert.isFalse(checks['has-no-more-than-one-main'].evaluate(node));
node.removeChild(mainLandmark);
node.removeChild(anotherMain);
});

it('should return true if there is only one element with role main', function(){
var mainDiv = document.createElement('div');
mainDiv.setAttribute('role','main');
var node = document.querySelector('html');
node.appendChild(mainDiv);
assert.isTrue(checks['has-no-more-than-one-main'].evaluate(node));
node.removeChild(mainDiv);
});

it('should return true if there is only one main element', function(){
var mainLandmark = document.createElement('main');
var node = document.querySelector('html');
node.appendChild(mainLandmark);
assert.isTrue(checks['has-no-more-than-one-main'].evaluate(node));
node.removeChild(mainLandmark);
});

});
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html lang="en" id="violation2">
<head>
<meta charset="utf8">
<script src="/axe.js"></script>
</head>
<body>
<p>No main content here either</p>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<!doctype html>
<html lang="en" id="pass2">
<head>
<meta charset="utf8">
<script src="/axe.js"></script>
</head>
<body>
<p>No main content here either</p>
<iframe id="frame2" src="level2-a.html"></iframe>
<iframe id="frame3" src="level2.html"></iframe>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<!doctype html>
<html lang="en" id="pass3">
<head>
<meta charset="utf8">
<script src="/axe.js"></script>
</head>
<body>
<main>
<p>Main landmark created with main tag</p>
</main>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<!doctype html>
Copy link
Contributor

Choose a reason for hiding this comment

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

This dir, and some of the files in it, should be renamed from landmark-at-least-one-main to landmark-one-main so that it matches the rule name.

<html lang="en" id="pass4">
<head>
<meta charset="utf8">
<script src="/axe.js"></script>
</head>
<body>
<p>No main content in this iframe</p>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<!doctype html>
Copy link
Contributor

Choose a reason for hiding this comment

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

We need integration tests for when there are no main landmarks too.

<html lang="en" id="fail1">
<head>
<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: 10000,
ui: 'bdd'
});
var assert = chai.assert;
</script>
</head>
<body>
<p>No main content here</p>
<iframe id="frame1" src="frames/level1-fail.html"></iframe>
<div id="mocha"></div>
<script src="landmark-at-least-one-main-fail.js"></script>
<script src="/test/integration/adapter.js"></script>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
describe('landmark-at-least-one-main test failure', function () {
'use strict';
var results;

before(function (done) {
axe.run({ runOnly: { type: 'rule', values: ['landmark-at-least-one-main'] } }, function (err, r) {
assert.isNull(err);
results = r;
done();
});
});

describe('violations', function () {
it('should find 1', function () {
assert.lengthOf(results.violations[0].nodes, 1);
});
it('should find first level iframe', function () {
assert.deepEqual(results.violations[0].nodes[0].target, ['#fail1']);
});
});

describe('passes', function () {
it('should find 0', function () {
assert.lengthOf(results.passes, 0);
});
});

it('should find 0 inapplicable', function () {
assert.lengthOf(results.inapplicable, 0);
});

it('should find 0 incomplete', function () {
assert.lengthOf(results.incomplete, 0);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<!doctype html>
<html lang="en" id="pass1">
<head>
<title>landmark-at-least-one-main test pass</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: 10000,
ui: 'bdd'
});
var assert = chai.assert;
</script>
</head>
<body>
<p>No main content</p>
<iframe id="frame1" src="frames/level1.html"></iframe>
<div id="mocha"></div>
<script src="landmark-at-least-one-main-pass.js"></script>
<script src="/test/integration/adapter.js"></script>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
describe('landmark-at-least-one-main test pass', function () {
'use strict';
var results;
before(function (done) {
window.addEventListener('load', function () {
axe.run({ runOnly: { type: 'rule', values: ['landmark-at-least-one-main'] } }, function (err, r) {
assert.isNull(err);
results = r;
done();
});
});
});

describe('violations', function () {
it('should find 0', function () {
document.innerHTML = results;
assert.lengthOf(results.violations, 0);
});
});

describe('passes', function () {
it('should find 4', function () {
assert.lengthOf(results.passes[0].nodes, 4);
});

it('should find #pass1', function () {
assert.deepEqual(results.passes[0].nodes[0].target, ['#pass1']);
});

it('should find #frame1, #pass2', function () {
assert.deepEqual(results.passes[0].nodes[1].target, ['#frame1', '#pass2']);
});

it('should find #frame1, #frame2, #pass3', function () {
assert.deepEqual(results.passes[0].nodes[2].target, ['#frame1', '#frame2', '#pass3']);
});

it('should find #frame1, #frame3, #pass4', function () {
assert.deepEqual(results.passes[0].nodes[3].target, ['#frame1', '#frame3', '#pass4']);
});
});

it('should find 0 inapplicable', function () {
assert.lengthOf(results.inapplicable, 0);
});

it('should find 0 incomplete', function () {
assert.lengthOf(results.incomplete, 0);
});

});