fix(only-listitem): add message about invalid role on li elements (#1954

)

* fix(only-listitem): add message about invalid role on li elements

* add nodes as relatedNodes

lib/checks/lists/only-listitems.js

@@ -1,64 +1,58 @@
-const { dom } = axe.commons;
-const getIsListItemRole = (role, tagName) => {
-	return role === 'listitem' || (tagName === 'LI' && !role);
-};
+const { dom, aria } = axe.commons;
 
-const getHasListItem = (hasListItem, tagName, isListItemRole) => {
-	return hasListItem || (tagName === 'LI' && isListItemRole) || isListItemRole;
-};
+let hasNonEmptyTextNode = false;
+let atLeastOneListitem = false;
+let isEmpty = true;
+let badNodes = [];
+let badRoleNodes = [];
+let badRoles = [];
 
-let base = {
-	badNodes: [],
-	isEmpty: true,
-	hasNonEmptyTextNode: false,
-	hasListItem: false,
-	liItemsWithRole: 0
-};
+virtualNode.children.forEach(vNode => {
+	const { actualNode } = vNode;
 
-let out = virtualNode.children.reduce((out, { actualNode }) => {
-	const tagName = actualNode.nodeName.toUpperCase();
+	if (actualNode.nodeType === 3 && actualNode.nodeValue.trim() !== '') {
+		hasNonEmptyTextNode = true;
+		return;
+	}
 
-	if (actualNode.nodeType === 1 && dom.isVisible(actualNode, true, false)) {
-		const role = (actualNode.getAttribute('role') || '').toLowerCase();
-		const isListItemRole = getIsListItemRole(role, tagName);
+	if (actualNode.nodeType !== 1 || !dom.isVisible(actualNode, true, false)) {
+		return;
+	}
 
-		out.hasListItem = getHasListItem(out.hasListItem, tagName, isListItemRole);
+	isEmpty = false;
+	const isLi = actualNode.nodeName.toUpperCase() === 'LI';
+	const role = aria.getRole(vNode);
+	const isListItemRole = role === 'listitem';
 
-		if (isListItemRole) {
-			out.isEmpty = false;
-		}
-		if (tagName === 'LI' && !isListItemRole) {
-			out.liItemsWithRole++;
-		}
-		if (tagName !== 'LI' && !isListItemRole) {
-			out.badNodes.push(actualNode);
-		}
+	if (!isLi && !isListItemRole) {
+		badNodes.push(actualNode);
 	}
-	if (actualNode.nodeType === 3) {
-		if (actualNode.nodeValue.trim() !== '') {
-			out.hasNonEmptyTextNode = true;
+
+	if (isLi && !isListItemRole) {
+		badRoleNodes.push(actualNode);
+
+		if (!badRoles.includes(role)) {
+			badRoles.push(role);
 		}
 	}
 
-	return out;
-}, base);
-
-const virtualNodeChildrenOfTypeLi = virtualNode.children.filter(
-	({ actualNode }) => {
-		return actualNode.nodeName.toUpperCase() === 'LI';
+	if (isListItemRole) {
+		atLeastOneListitem = true;
 	}
-);
+});
 
-const allLiItemsHaveRole =
-	out.liItemsWithRole > 0 &&
-	virtualNodeChildrenOfTypeLi.length === out.liItemsWithRole;
+if (hasNonEmptyTextNode || badNodes.length) {
+	this.relatedNodes(badNodes);
+	return true;
+}
 
-if (out.badNodes.length) {
-	this.relatedNodes(out.badNodes);
+if (isEmpty || atLeastOneListitem) {
+	return false;
 }
 
-const isInvalidListItem = !(
-	out.hasListItem ||
-	(out.isEmpty && !allLiItemsHaveRole)
-);
-return isInvalidListItem || !!out.badNodes.length || out.hasNonEmptyTextNode;
+this.relatedNodes(badRoleNodes);
+this.data({
+	messageKey: 'roleNotValid',
+	roles: badRoles.join(', ')
+});
+return true;

lib/checks/lists/only-listitems.json

@@ -5,7 +5,10 @@
 		"impact": "serious",
 		"messages": {
 			"pass": "List element only has direct children that are allowed inside <li> elements",
-			"fail": "List element has direct children that are not 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}"
+			}
 		}
 	}
 }

test/checks/lists/only-listitems.js

@@ -123,11 +123,19 @@ describe('only-listitems', function() {
 
 	it('should return true if the list has only li items with their roles changed', function() {
 		var checkArgs = checkSetup(
-			'<ol id="target"><li role="menuitem">Not a list item</li><li role="menuitem">Not a list item</li></ol>'
+			'<ol id="target"><li id="fail1" role="menuitem">Not a list item</li><li id="fail2" role="menuitem">Not a list item</li></ol>'
 		);
 		assert.isTrue(
 			checks['only-listitems'].evaluate.apply(checkContext, checkArgs)
 		);
+		assert.deepEqual(checkContext._data, {
+			messageKey: 'roleNotValid',
+			roles: 'menuitem'
+		});
+		assert.deepEqual(checkContext._relatedNodes, [
+			fixture.querySelector('#fail1'),
+			fixture.querySelector('#fail2')
+		]);
 	});
 
 	it('should return true if <link> is used along side only li items with their roles changed', function() {