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

fix(required-parent): Allow *item > group > *item nesting #2898

Merged
merged 3 commits into from
May 2, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
chore(required-parent): Add ownGroupRoles option
  • Loading branch information
WilcoFiers committed Apr 30, 2021
commit 2996146956d4faa91c8437bfebb64ee4427e56ec
24 changes: 24 additions & 0 deletions doc/check-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- [Global Options](#global-options)
- [aria-allowed-role](#aria-allowed-role)
- [aria-required-children](#aria-required-children)
- [aria-required-parent](#aria-required-parent)
- [aria-roledescription](#aria-roledescription)
- [color-contrast](#color-contrast)
- [page-has-heading-one](#page-has-heading-one)
Expand Down Expand Up @@ -107,6 +108,29 @@ All checks allow these global options:
</tbody>
</table>

### aria-required-parent

<table>
Copy link
Member

Choose a reason for hiding this comment

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

NBD but it's strange that we're not using Markdown to generate this table 🤷

Copy link
Contributor

@straker straker Apr 30, 2021

Choose a reason for hiding this comment

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

I remember it was because Markdown table syntax wasn't working so well with multiline code blocks in the cells.

<thead>
<tr>
<th>Option</th>
<th align="left">Default</th>
<th align="left">Description</th>
</tr>
</thead>
<tbody>
<tr>
<td>
<code>ownGroupRoles</code>
</td>
<td align="left">
<pre lang=js><code>['listitem', 'treeitem']</code></pre>
</td>
<td align="left">List of ARIA roles that when used in a group can have a grand parent with the same role. E.g. <code>list > listitem > group > listitem</code>.</td>
</tr>
</tbody>
</table>

### aria-roledescription

<table>
Expand Down
16 changes: 11 additions & 5 deletions lib/checks/aria/aria-required-parent-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { getExplicitRole, getRole, requiredContext } from '../../commons/aria';
import { getRootNode } from '../../commons/dom';
import { getNodeFromTree, escapeSelector } from '../../core/utils';

function getMissingContext(virtualNode, reqContext, includeElement) {
function getMissingContext(virtualNode, ownGroupRoles, reqContext, includeElement) {
const explicitRole = getExplicitRole(virtualNode);

if (!reqContext) {
Expand All @@ -21,7 +21,7 @@ function getMissingContext(virtualNode, reqContext, includeElement) {
// context, check next parent
if (reqContext.includes('group') && parentRole === 'group') {
// Allow the own role; i.e. tree > treeitem > group > treeitem
if (['treeitem', 'listitem'].includes(explicitRole)) {
if (ownGroupRoles.includes(explicitRole)) {
reqContext.push(explicitRole);
}
vNode = vNode.parent;
Expand Down Expand Up @@ -86,18 +86,24 @@ function getAriaOwners(element) {
* @return {Boolean} True if the element has a parent with a required role. False otherwise.
*/
function ariaRequiredParentEvaluate(node, options, virtualNode) {
var missingParents = getMissingContext(virtualNode);
const ownGroupRoles = (
options && Array.isArray(options.ownGroupRoles)
? options.ownGroupRoles
: []
);
let missingParents = getMissingContext(virtualNode, ownGroupRoles);

if (!missingParents) {
return true;
}

var owners = getAriaOwners(node);
const owners = getAriaOwners(node);

if (owners) {
for (var i = 0, l = owners.length; i < l; i++) {
for (let i = 0, l = owners.length; i < l; i++) {
missingParents = getMissingContext(
getNodeFromTree(owners[i]),
ownGroupRoles,
missingParents,
true
);
Expand Down
2 changes: 1 addition & 1 deletion lib/checks/aria/aria-required-parent.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"id": "aria-required-parent",
"evaluate": "aria-required-parent-evaluate",
"options": {
"selfNestingGroupRoles": ["listitem", "treeitem"]
"ownGroupRoles": ["listitem", "treeitem"]
},
"metadata": {
"impact": "critical",
Expand Down
90 changes: 57 additions & 33 deletions test/checks/aria/required-parent.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ describe('aria-required-parent', function() {
.getCheckEvaluate('aria-required-parent')
.apply(checkContext, params)
);
assert.deepEqual(checkContext._data, ['list']);
assert.deepEqual(checkContext._data, ['list', 'group']);
});

(shadowSupported ? it : xit)(
Expand All @@ -44,7 +44,7 @@ describe('aria-required-parent', function() {
.getCheckEvaluate('aria-required-parent')
.apply(checkContext, params)
);
assert.deepEqual(checkContext._data, ['list']);
assert.deepEqual(checkContext._data, ['list', 'group']);
}
);

Expand All @@ -70,7 +70,7 @@ describe('aria-required-parent', function() {
.getCheckEvaluate('aria-required-parent')
.apply(checkContext, params)
);
assert.deepEqual(checkContext._data, ['list']);
assert.deepEqual(checkContext._data, ['list', 'group']);
});

it('should pass when required parent is present in an aria-owns context', function() {
Expand Down Expand Up @@ -152,7 +152,7 @@ describe('aria-required-parent', function() {

it('should fail when intermediate node is role=group but this not an allowed context', function() {
var params = checkSetup(
'<div role="list"><div role="group"><p role="listitem" id="target">Nothing here.</p></div></div>'
'<div role="menu"><div role="group"><p role="listitem" id="target">Nothing here.</p></div></div>'
);
assert.isFalse(
axe.testUtils
Expand All @@ -161,36 +161,60 @@ describe('aria-required-parent', function() {
);
});

it('should pass when group is nested in an element of the same role', function() {
var params = checkSetup(
'<div role="list">' +
'<div role="listitem">' +
'<div role="group">' +
'<div role="listitem" id="target">' +
'</div></div></div></div>'
);

assert.isTrue(
axe.testUtils
.getCheckEvaluate('aria-required-parent')
.apply(checkContext, params)
);
});

it('should fail when group is nested in an element of the same menuitem role', function() {
var params = checkSetup(
'<div role="menu">' +
'<div role="menuitem">' +
'<div role="group">' +
'<div role="menuitem" id="target">' +
'</div></div></div></div>'
);
describe('group with ownGroupRoles', function () {
it('should pass when the role and grand parent role is in ownGroupRoles', function() {
var params = checkSetup(
'<div role="list">' +
'<div role="listitem">' +
'<div role="group">' +
'<div role="listitem" id="target">' +
'</div></div></div></div>', {
ownGroupRoles: ['listitem']
}
);

assert.isTrue(
axe.testUtils
.getCheckEvaluate('aria-required-parent')
.apply(checkContext, params)
);
});

it('should fail when the role and grand parent role is in ownGroupRoles', function() {
var params = checkSetup(
'<div role="menu">' +
'<div role="menuitem">' +
'<div role="group">' +
'<div role="menuitem" id="target">' +
'</div></div></div></div>', {
ownGroupRoles: ['listitem']
}
);

assert.isFalse(
axe.testUtils
.getCheckEvaluate('aria-required-parent')
.apply(checkContext, params)
);
});

assert.isFalse(
axe.testUtils
.getCheckEvaluate('aria-required-parent')
.apply(checkContext, params)
);
it('should fail when the role is not in a group', function () {
var params = checkSetup(
'<div role="list">' +
'<div role="listitem">' +
'<div role="none">' +
'<div role="listitem" id="target">' +
'</div></div></div></div>', {
ownGroupRoles: ['listitem']
}
);

assert.isFalse(
axe.testUtils
.getCheckEvaluate('aria-required-parent')
.apply(checkContext, params)
);
})
});

it('should pass when intermediate node is role=none', function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
["#fail4"],
["#fail5"],
["#fail6"],
["#fail7"],
["#fail7"]
],
"passes": [
["#pass1"],
Expand Down
6 changes: 5 additions & 1 deletion test/integration/rules/preprocessor.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ var createIntegrationPreprocessor = function(logger) {
// and add the test data to it
var htmlpath = file.originalPath.replace(extRegex, '.html');
var html = fs.readFileSync(htmlpath, 'utf-8');
var test = JSON.parse(content);
try {
var test = JSON.parse(content);
} catch (e) {
throw new Error('Unable to parse content of ' + file.originalPath)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Encountered this because I had a stray comma. Figured I'd tweak it a bit.

}
test.content = html;

var result = template.replace('{}; /*tests*/', JSON.stringify(test));
Expand Down