Skip to content

Commit

Permalink
feat(list/definition-list): only allow required owned roles (#3707)
Browse files Browse the repository at this point in the history
* fix(list/definition-list): Clarify remediation messages

* Add docs

* Tweaks

* Remove log
  • Loading branch information
WilcoFiers committed Oct 17, 2022
1 parent 7e2f0af commit a920d35
Show file tree
Hide file tree
Showing 12 changed files with 462 additions and 487 deletions.
11 changes: 11 additions & 0 deletions doc/check-options.md
Expand Up @@ -35,6 +35,7 @@
- [target-size](#target-size)
- [region](#region)
- [inline-style-property](#inline-style-property)
- [invalid-children](#invalid-children)

## How Checks Work

Expand Down Expand Up @@ -526,3 +527,13 @@ This evaluate method is used in the following checks. Default vary between check
| `normalValue` | The value to use when `normal` is set, defaults to `0` |

If `minValue` and `maxValue` are both undefined, the check returns `false` if the property is used with !important. If done along with `noImportant: true`, the check returns false if the property is set at all in the style attribute.

### invalid-children

This evaluation method is used in the `list` and `definition-list` rule to determine whether its child nodes are allowed.

| Option | Description |
| ---------------- | :---------------------------------------------------------------------------------- |
| `validNodeNames` | Nodes without role allowed as children |
| `validRoles` | Roles allowed on child elements |
| `divGroups` | Whether the child nodes can be grouped in a div without any role (false by default) |
75 changes: 75 additions & 0 deletions lib/checks/lists/invalid-children-evaluate.js
@@ -0,0 +1,75 @@
import { isVisibleToScreenReaders } from '../../commons/dom';
import { getExplicitRole } from '../../commons/aria';

export default function invalidChildrenEvaluate(
node,
options = {},
virtualNode
) {
const relatedNodes = [];
const issues = [];
if (!virtualNode.children) {
return undefined;
}

const vChildren = mapWithNested(virtualNode.children);
while (vChildren.length) {
const { vChild, nested } = vChildren.shift();
if (options.divGroups && !nested && isDivGroup(vChild)) {
if (!vChild.children) {
return undefined;
}
const vGrandChildren = mapWithNested(vChild.children, true);
vChildren.push(...vGrandChildren);
continue;
}

const issue = getInvalidSelector(vChild, nested, options);
if (!issue) {
continue;
}
if (!issues.includes(issue)) {
issues.push(issue);
}
if (vChild?.actualNode?.nodeType === 1) {
relatedNodes.push(vChild.actualNode);
}
}
if (issues.length === 0) {
return false;
}

this.data({ values: issues.join(', ') });
this.relatedNodes(relatedNodes);
return true;
}

function getInvalidSelector(
vChild,
nested,
{ validRoles = [], validNodeNames = [] }
) {
const { nodeName, nodeType, nodeValue } = vChild.props;
const selector = nested ? 'div > ' : '';
if (nodeType === 3 && nodeValue.trim() !== '') {
return selector + `#text`;
}
if (nodeType !== 1 || !isVisibleToScreenReaders(vChild)) {
return false;
}

const role = getExplicitRole(vChild);
if (role) {
return validRoles.includes(role) ? false : selector + `[role=${role}]`;
} else {
return validNodeNames.includes(nodeName) ? false : selector + nodeName;
}
}

function isDivGroup(vNode) {
return vNode.props.nodeName === 'div' && getExplicitRole(vNode) === null;
}

function mapWithNested(vNodes, nested = false) {
return vNodes.map(vChild => ({ vChild, nested }));
}
11 changes: 8 additions & 3 deletions lib/checks/lists/only-dlitems.json
@@ -1,11 +1,16 @@
{
"id": "only-dlitems",
"evaluate": "only-dlitems-evaluate",
"evaluate": "invalid-children-evaluate",
"options": {
"validRoles": ["definition", "term", "listitem"],
"validNodeNames": ["dt", "dd"],
"divGroups": true
},
"metadata": {
"impact": "serious",
"messages": {
"pass": "List element only has direct children that are allowed inside <dt> or <dd> elements",
"fail": "List element has direct children that are not allowed inside <dt> or <dd> elements"
"pass": "dl element only has direct children that are allowed inside; <dt>, <dd>, or <div> elements",
"fail": "dl element has direct children that are not allowed: ${data.values}"
}
}
}
11 changes: 6 additions & 5 deletions lib/checks/lists/only-listitems.json
@@ -1,14 +1,15 @@
{
"id": "only-listitems",
"evaluate": "only-listitems-evaluate",
"evaluate": "invalid-children-evaluate",
"options": {
"validRoles": ["listitem"],
"validNodeNames": ["li"]
},
"metadata": {
"impact": "serious",
"messages": {
"pass": "List element only has direct children that are allowed inside <li> elements",
"fail": {
"default": "List element has direct children that are not allowed inside <li> elements",
"roleNotValid": "List element has direct children with a role that is not allowed: ${data.roles}"
}
"fail": "List element has direct children that are not allowed: ${data.values}"
}
}
}
4 changes: 2 additions & 2 deletions lib/rules/no-role-matches.js
@@ -1,5 +1,5 @@
function noRoleMatches(node) {
return !node.getAttribute('role');
function noRoleMatches(node, vNode) {
return !vNode.attr('role');
}

export default noRoleMatches;
9 changes: 3 additions & 6 deletions locales/_template.json
Expand Up @@ -782,15 +782,12 @@
}
},
"only-dlitems": {
"pass": "List element only has direct children that are allowed inside <dt> or <dd> elements",
"fail": "List element has direct children that are not allowed inside <dt> or <dd> elements"
"pass": "dl element only has direct children that are allowed inside; <dt>, <dd>, or <div> elements",
"fail": "dl element has direct children that are not allowed: ${data.values}"
},
"only-listitems": {
"pass": "List element only has direct children that are allowed inside <li> elements",
"fail": {
"default": "List element has direct children that are not allowed inside <li> elements",
"roleNotValid": "List element has direct children with a role that is not allowed: ${data.roles}"
}
"fail": "List element has direct children that are not allowed: ${data.values}"
},
"structured-dlitems": {
"pass": "When not empty, element has both <dt> and <dd> elements",
Expand Down

0 comments on commit a920d35

Please sign in to comment.