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

Add rule, landmark-main-is-top-level #462

Merged
merged 24 commits into from
Nov 14, 2017
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
edeb860
init
sulsanaul Jul 19, 2017
3d37aab
feat: add new rule, landmark-no-more-than-one-main
sulsanaul Jul 19, 2017
4e1b18b
test(landmark-at-least-one-main): updated integration tests for check
sulsanaul Jul 19, 2017
2840b02
feat(landmark-main-is-top-level): add rule ensuring each main landmar…
sulsanaul Jul 26, 2017
fa6069d
Merge https://github.com/dequelabs/axe-core into main/is-top-level
sulsanaul Jul 26, 2017
6eb553b
refactor(landmark-main-is-top-level): change help messages
sulsanaul Aug 16, 2017
094e251
feat(landmark-main-is-top-level): change a function used in check and…
sulsanaul Aug 18, 2017
2c55cbb
refactor(main-is-top-level): change pass/fail messages
sulsanaul Aug 23, 2017
b9b1bf5
refactor(landmark-main-is-top-level): change description/help messages
sulsanaul Aug 23, 2017
bf85e0d
Merge https://github.com/dequelabs/axe-core into main/is-top-level
sulsanaul Aug 23, 2017
747990d
Merge branch 'main/is-top-level' of https://github.com/ssanaul/axe-co…
sulsanaul Aug 23, 2017
9c0e75c
feat(main-is-top-level): update check for shadow dom features
sulsanaul Aug 23, 2017
824ee2d
fix(main-is-top-level): update check to ignore form as a landmark
sulsanaul Aug 28, 2017
05ee0a7
fix: edit incorrect usage of getComposedParent
sulsanaul Sep 5, 2017
73ed9cf
test: add unit test to check if main landmark in shadow dom is top level
sulsanaul Sep 5, 2017
f82c667
style: remove spaces in attributes
sulsanaul Sep 21, 2017
34407e9
fix: update test so that checkSetup passes in correct argument
sulsanaul Sep 21, 2017
3e5c6ad
test: add test to ensure correct number of violations nodes
sulsanaul Sep 21, 2017
ff463ef
Merge branch 'main/is-top-level' of https://github.com/ssanaul/axe-co…
sulsanaul Sep 21, 2017
03e5434
fix: revert 'application' type to landmark
sulsanaul Sep 21, 2017
56ad598
Merge https://github.com/dequelabs/axe-core into main/is-top-level
sulsanaul Sep 21, 2017
00b10c1
Merge https://github.com/dequelabs/axe-core into main/is-top-level
sulsanaul Sep 26, 2017
f936751
style: remove spaces in attributes
sulsanaul Oct 18, 2017
12b1903
fix: allow main landmark to be in form
sulsanaul Oct 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
1 change: 1 addition & 0 deletions doc/rule-descriptions.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
| input-image-alt | Ensures <input type="image"> elements have alternate text | cat.text-alternatives, wcag2a, wcag111, section508, section508.22.a | true |
| label-title-only | Ensures that every form element is not solely labeled using the title or aria-describedby attributes | cat.forms, best-practice | false |
| label | Ensures every form element has a label | cat.forms, wcag2a, wcag332, wcag131, section508, section508.22.n | true |
| landmark-main-is-top-level | The main landmark should not be contained in another landmark | best-practice | true |
| layout-table | Ensures presentational <table> elements do not use <th>, <caption> elements or the summary attribute | cat.semantics, wcag2a, wcag131 | true |
| link-in-text-block | Links can be distinguished without relying on color | cat.color, experimental, wcag2a, wcag141 | true |
| link-name | Ensures links have discernible text | cat.name-role-value, wcag2a, wcag111, wcag412, wcag244, section508, section508.22.a | true |
Expand Down
13 changes: 13 additions & 0 deletions lib/checks/keyboard/main-is-top-level.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var landmarks = axe.commons.aria.getRolesByType('landmark');
var parent = axe.commons.dom.getComposedParent(node);
while (parent){
var role = parent.getAttribute('role');
if (!role && (parent.tagName.toLowerCase() !== 'form')){
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to work with role=form too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does line 5 not accomplish that?

role = axe.commons.aria.implicitRole(parent);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is too strict. There are roles that can certainly be permitted, off the top of my head: form, application, presentation. Probably a bunch more. Can you go through the list and make sure those kind of things get ignored?

Copy link

Choose a reason for hiding this comment

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

WIlco,

  1. In ARIA 1.1 application and presentation roles are not considered landmarks.
  2. In the case of a form landmark:
    2.a Form landmarks should be fairly rare, there is discussion in the ARIA and HTML5 working groups that form landmarks should/must have an accessible name to be considered a landmark or at least useful, since without knowing what the form is about the navigation feature is somewhat limited since getting to form controls is easy from screen readers and makes the landmark more of a distraction than a useful feature.
    2.b Forms are typically just one part of a page so should be in another landmark, the main landmark is what people will be looking for, not a form landmark. In general Form landmarks should only be used if there are multiple forms on a page.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jongund Both of those suggestions make sense to me. Seeing as it's still an open discussion if forms should be a landmark, make an exception for this rule. That one is particularly relevant for the region rule. You can adjust the type of application and presentation in lib/commons/aria/index.

Copy link

@jongund jongund Aug 18, 2017

Choose a reason for hiding this comment

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

Wilco,

We will modify the file lib/commons/aria/index for role application:
type: 'landmark' to type: 'structure'

role presentation is already has type: 'structure' so no change need for it.

}
if (role && landmarks.includes(role)){
return false;
}
parent = axe.commons.dom.getComposedParent(parent);
}
return true;
11 changes: 11 additions & 0 deletions lib/checks/keyboard/main-is-top-level.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"id": "main-is-top-level",
"evaluate": "main-is-top-level.js",
"metadata": {
"impact": "moderate",
"messages": {
"pass": "The main landmark is at the top level.",
"fail": "The main landmark is contained in another landmark."
}
}
}
2 changes: 1 addition & 1 deletion lib/commons/aria/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ lookupTables.role = {
context: null
},
'application': {
type: 'landmark',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is causing the following test to fail, be sure to fix that:

1) table.isDataTable should be true if the table has a landmark role application:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WilcoFiers
In ARIA 1.1, 'application' is not a landmark role.
https://www.w3.org/TR/wai-aria-1.1/#application
The new superclass role is 'structure'.
Can you update the 'develop' branch to meet the ARIA 1.1 specification?

type: 'structure',
attributes: {
allowed: ['aria-expanded']
},
Expand Down
16 changes: 16 additions & 0 deletions lib/rules/landmark-main-is-top-level.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"id": "landmark-main-is-top-level",
"selector": "main,[role=main]",
"tags": [
"best-practice"
],
"metadata": {
"description": "The main landmark should not be contained in another landmark",
"help": "Main landmark is not at top level"
},
"all": [],
"any": [
"main-is-top-level"
],
"none": []
}
60 changes: 60 additions & 0 deletions test/checks/keyboard/main-is-top-level.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
describe('main-is-top-level', function () {
'use strict';

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

var shadowSupported = axe.testUtils.shadowSupport.v1;
var checkSetup = axe.testUtils.checkSetup;

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

it('should return false if main landmark is in another landmark', function () {
var mainLandmark = document.createElement('main');
var bannerDiv = document.createElement('div');
bannerDiv.setAttribute('role','banner');
bannerDiv.appendChild(mainLandmark);
fixture.appendChild(bannerDiv);
assert.isFalse(checks['main-is-top-level'].evaluate(mainLandmark));
});

it('should return false if div with role set to main is in another landmark', function () {
var mainDiv = document.createElement('div');
mainDiv.setAttribute('role','main');
var navDiv = document.createElement('div');
navDiv.setAttribute('role','navigation');
navDiv.appendChild(mainDiv);
fixture.appendChild(navDiv);
assert.isFalse(checks['main-is-top-level'].evaluate(mainDiv));
});

it('should return true if main landmark is not in another landmark', function () {
var mainLandmark = document.createElement('main');
var bannerDiv = document.createElement('div');
bannerDiv.setAttribute('role','banner');
fixture.appendChild(bannerDiv);
fixture.appendChild(mainLandmark);
assert.isTrue(checks['main-is-top-level'].evaluate(mainLandmark));
});

it('should return true if div with role set to main is not in another landmark', function () {
var mainDiv = document.createElement('div');
mainDiv.setAttribute('role','main');
var navDiv = document.createElement('div');
navDiv.setAttribute('role','navigation');
fixture.appendChild(navDiv);
fixture.appendChild(mainDiv);
assert.isTrue(checks['main-is-top-level'].evaluate(mainDiv));
});


(shadowSupported ? it : xit)('should test if main in shadow DOM is top level', function () {
var div = document.createElement('div');
var shadow = div.attachShadow({ mode: 'open' });
shadow.innerHTML = '<main>Main content</main>';
var checkArgs = checkSetup(div);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will select the div, not the <main> element. This should be checkSetup(shadow.querySelector('main'));

assert.isTrue(checks['main-is-top-level'].evaluate.apply(null, checkArgs));
});

});
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<!doctype html>
<html id="violation2">
<head>
<meta charset="utf8">
<script src="/axe.js"></script>
</head>
<body>
<p>This iframe should fail, too</p>
<div role = "complementary">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use spaces in attributes.

<div role = "main">
<p>This main landmark is in a complementary landmark</p>
</div>
</div>
<iframe id = "frame2" src = "level2.html"></iframe>
<iframe id = "frame3" src = "level2-a.html"></iframe>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<!doctype html>
<html id="pass2">
<head>
<meta charset="utf8">
<script src="/axe.js"></script>
</head>
<body>
<p>This iframe should pass, too</p>

<div role = "banner">
<p>This div has role banner</p>
</div>
<div role = "navigation">
<p>This div has role navigation</p>
</div>
<div role = "main">
<p>This main content is not within another landmark</p>
</div>
<div role = "complementary">
<p>This div has role complementary</p>
</div>
<div role = "search">
<p>This div has role search</p>
</div>
<div role = "form">
<p>This div has role form<p>
</div>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!doctype html>
<html id="violation4">
<head>
<meta charset="utf8">
<script src="/axe.js"></script>
</head>
<body>
<p>This iframe is also a violation</p>
<div role = "form">
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be a failure. Neither should role="application". Make sure there is a test for that too.

<main>
<p>This main landmark is in a form landmark</p>
</main>
</div>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!doctype html>
<html id="violation3">
<head>
<meta charset="utf8">
<script src="/axe.js"></script>
</head>
<body>
<p>This iframe is another violation<p>
<div role = "search">
<main>
<p>This main landmark is in a search landmark</p>
</main>
</div>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<!doctype html>
<html lang="en" id="violation1">
<head>
<title>landmark-main-is-top-level test</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>
<div role = "navigation">
<div role = "main">
<p>This is going to fail</p>
</div>
</div>
<iframe id="frame1" src="frames/level1-fail.html"></iframe>
<div id="mocha"></div>
<script src="landmark-main-is-top-level-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,38 @@

describe('landmark-main-is-top-level test fail', function () {
'use strict';
var results;
before(function (done) {
window.addEventListener('load', function () {
axe.run({ runOnly: { type: 'rule', values: ['landmark-main-is-top-level'] } }, function (err, r) {
assert.isNull(err);
results = r;
done();
});
});
});

describe('violations', function () {
it('should find 1', function () {
assert.lengthOf(results.violations, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a node count check too:

assert.lengthOf(results.violations[0].nodes, 2);

Copy link
Contributor

Choose a reason for hiding this comment

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

@ssanaul This issue isn't addressed yet.

Copy link
Contributor Author

@sulsanaul sulsanaul Oct 19, 2017

Choose a reason for hiding this comment

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

I thought 3e5c6ad resolved it. It checks for 4 nodes because I believe there are 4 violations. @WilcoFiers

});
});

describe('passes', function () {
it('should find none', 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,42 @@
<!doctype html>
<html lang="en" id="pass1">
<head>
<title>landmark-main-is-top-level test</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>
<div role = "banner">
<p>This div has role banner</p>
</div>
<div role = "navigation">
<p>This div has role navigation</p>
</div>
<main>
<p>This main content is not within another landmark</p>
</main>
<div role = "complementary">
<p>This div has role complementary</p>
</div>
<div role = "search">
<p>This div has role search</p>
</div>
<div role = "form">
<p>This div has role form<p>
</div>
<iframe id="frame1" src="frames/level1.html"></iframe>
<div id="mocha"></div>
<script src="landmark-main-is-top-level-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,36 @@

describe('landmark-main-is-top-level test pass', function () {
'use strict';
var results;
before(function (done) {
window.addEventListener('load', function () {
axe.run({ runOnly: { type: 'rule', values: ['landmark-main-is-top-level'] } }, function (err, r) {
assert.isNull(err);
results = r;
done();
});
});
});

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

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

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

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

});