From 2e27dca551d1aee273ad8ac055f7dfd45578dad0 Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Fri, 14 Jan 2022 10:35:28 +0100 Subject: [PATCH] fix(prohibited-attr): always report incomplete if there is text in the subtree (#3347) * fix(prohibited-attr): always report incomplete if there is text in the subtree Subtree text wasn't computed for elements that are not named from content. To force this, subtreeDescendant: true had to be passed. This PR also significantly improves the messages of the prohibited-attr check, which was the cause of some confusion. * more tests --- .../aria/aria-prohibited-attr-evaluate.js | 34 +++---- lib/checks/aria/aria-prohibited-attr.json | 14 ++- test/checks/aria/aria-prohibited-attr.js | 88 ++++++++++++++++--- .../rules/aria-allowed-attr/failures.html | 52 +++++------ .../rules/aria-allowed-attr/incomplete.html | 1 + .../rules/aria-allowed-attr/incomplete.json | 7 +- 6 files changed, 139 insertions(+), 57 deletions(-) diff --git a/lib/checks/aria/aria-prohibited-attr-evaluate.js b/lib/checks/aria/aria-prohibited-attr-evaluate.js index ff843065fd..c4e7085761 100644 --- a/lib/checks/aria/aria-prohibited-attr-evaluate.js +++ b/lib/checks/aria/aria-prohibited-attr-evaluate.js @@ -26,14 +26,12 @@ import standards from '../../standards'; * @memberof checks * @return {Boolean} True if the element uses any prohibited ARIA attributes. False otherwise. */ -function ariaProhibitedAttrEvaluate(node, options = {}, virtualNode) { - const extraElementsAllowedAriaLabel = options.elementsAllowedAriaLabel || []; - - const prohibitedList = listProhibitedAttrs( - virtualNode, - extraElementsAllowedAriaLabel - ); +export default function ariaProhibitedAttrEvaluate(node, options = {}, virtualNode) { + const elementsAllowedAriaLabel = options?.elementsAllowedAriaLabel || []; + const { nodeName } = virtualNode.props; + const role = getRole(virtualNode, { chromium: true }); + const prohibitedList = listProhibitedAttrs(role, nodeName, elementsAllowedAriaLabel); const prohibited = prohibitedList.filter(attrName => { if (!virtualNode.attrNames.includes(attrName)) { return false; @@ -45,24 +43,26 @@ function ariaProhibitedAttrEvaluate(node, options = {}, virtualNode) { return false; } - this.data(prohibited); - const hasTextContent = sanitize(subtreeText(virtualNode)) !== ''; - // Don't fail if there is text content to announce - return hasTextContent ? undefined : true; + let messageKey = virtualNode.hasAttr('role') ? 'hasRole' : 'noRole'; + messageKey += prohibited.length > 1 ? 'Plural' : 'Singular'; + this.data({ role, nodeName, messageKey, prohibited }); + + // `subtreeDescendant` to override namedFromContents + const textContent = subtreeText(virtualNode, { subtreeDescendant: true }); + if (sanitize(textContent) !== '') { + // Don't fail if there is text content to announce + return undefined; + } + return true; } -function listProhibitedAttrs(virtualNode, elementsAllowedAriaLabel) { - const role = getRole(virtualNode, { chromium: true }); +function listProhibitedAttrs(role, nodeName, elementsAllowedAriaLabel) { const roleSpec = standards.ariaRoles[role]; if (roleSpec) { return roleSpec.prohibitedAttrs || []; } - - const { nodeName } = virtualNode.props; if (!!role || elementsAllowedAriaLabel.includes(nodeName)) { return []; } return ['aria-label', 'aria-labelledby']; } - -export default ariaProhibitedAttrEvaluate; diff --git a/lib/checks/aria/aria-prohibited-attr.json b/lib/checks/aria/aria-prohibited-attr.json index fd2f3e72bc..21e8a007f9 100644 --- a/lib/checks/aria/aria-prohibited-attr.json +++ b/lib/checks/aria/aria-prohibited-attr.json @@ -8,8 +8,18 @@ "impact": "serious", "messages": { "pass": "ARIA attribute is allowed", - "fail": "ARIA attribute: ${data.values} is not allowed. Use a different role attribute or element.", - "incomplete": "ARIA attribute: ${data.values} is not well supported. Use a different role attribute or element." + "fail": { + "hasRolePlural": "${data.prohibited} attributes cannot be used with role \"${data.role}\".", + "hasRoleSingular": "${data.prohibited} attribute cannot be used with role \"${data.role}\".", + "noRolePlural": "${data.prohibited} attributes cannot be used on a ${data.nodeName} with no valid role attribute.", + "noRoleSingular": "${data.prohibited} attribute cannot be used on a ${data.nodeName} with no valid role attribute." + }, + "incomplete": { + "hasRoleSingular": "${data.prohibited} attribute is not well supported with role \"${data.role}\".", + "hasRolePlural": "${data.prohibited} attributes are not well supported with role \"${data.role}\".", + "noRoleSingular": "${data.prohibited} attribute is not well supported on a ${data.nodeName} with no valid role attribute.", + "noRolePlural": "${data.prohibited} attributes are not well supported on a ${data.nodeName} with no valid role attribute." + } } } } diff --git a/test/checks/aria/aria-prohibited-attr.js b/test/checks/aria/aria-prohibited-attr.js index 4900b6e3f5..c373bb521c 100644 --- a/test/checks/aria/aria-prohibited-attr.js +++ b/test/checks/aria/aria-prohibited-attr.js @@ -9,39 +9,105 @@ describe('aria-prohibited-attr', function() { checkContext.reset(); }); - it('should return true for prohibited attributes', function() { + it('should return true for prohibited attributes and no content', function() { var params = checkSetup( - '
Contents
' + '
' ); assert.isTrue(checkEvaluate.apply(checkContext, params)); - assert.deepEqual(checkContext._data, ['aria-label']); + assert.deepEqual(checkContext._data, { + nodeName: 'div', + role: 'code', + messageKey: 'hasRoleSingular', + prohibited: ['aria-label'] + }); + }); + + it('should return undefined for prohibited attributes and content', function() { + var params = checkSetup( + '
Contents
' + ); + assert.isUndefined(checkEvaluate.apply(checkContext, params)); + assert.deepEqual(checkContext._data, { + nodeName: 'div', + role: 'code', + messageKey: 'hasRoleSingular', + prohibited: ['aria-label'] + }); }); it('should return true for multiple prohibited attributes', function() { var params = checkSetup( - '
Contents
' + '
' ); assert.isTrue(checkEvaluate.apply(checkContext, params)); + assert.deepEqual(checkContext._data, { + nodeName: 'div', + role: 'code', + messageKey: 'hasRolePlural', + // attribute order not important + prohibited: [ + 'aria-label', + 'aria-labelledby' + ] + }); + }); - // attribute order not important - assert.sameDeepMembers(checkContext._data, [ - 'aria-label', - 'aria-labelledby' - ]); + it('should return undefined if element has no role and has text content (singular)', function() { + var params = checkSetup( + '
Contents
' + ); + assert.isUndefined(checkEvaluate.apply(checkContext, params)); + assert.deepEqual(checkContext._data, { + nodeName: 'div', + role: null, + messageKey: 'noRoleSingular', + prohibited: ['aria-label'] + }); }); - it('should return undefined if element has no role and has text content', function() { + it('should return undefined if element has no role and has text content (plural)', function() { var params = checkSetup( '
Contents
' ); assert.isUndefined(checkEvaluate.apply(checkContext, params)); + assert.deepEqual(checkContext._data, { + nodeName: 'div', + role: null, + messageKey: 'noRolePlural', + prohibited: [ + 'aria-label', + 'aria-labelledby' + ] + }); + }); + + it('should return true if element has no role and no text content (singular)', function() { + var params = checkSetup( + '
' + ); + assert.isTrue(checkEvaluate.apply(checkContext, params)); + assert.deepEqual(checkContext._data, { + nodeName: 'div', + role: null, + messageKey: 'noRoleSingular', + prohibited: ['aria-label'] + }); }); - it('should return true if element has no role and no text content', function() { + it('should return true if element has no role and no text content (plural)', function() { var params = checkSetup( '
' ); assert.isTrue(checkEvaluate.apply(checkContext, params)); + assert.deepEqual(checkContext._data, { + nodeName: 'div', + role: null, + messageKey: 'noRolePlural', + prohibited: [ + 'aria-label', + 'aria-labelledby' + ] + }); }); it('should return false if all attributes are allowed', function() { diff --git a/test/integration/rules/aria-allowed-attr/failures.html b/test/integration/rules/aria-allowed-attr/failures.html index 578b339141..c12fd3c7e0 100644 --- a/test/integration/rules/aria-allowed-attr/failures.html +++ b/test/integration/rules/aria-allowed-attr/failures.html @@ -1,25 +1,25 @@ - -
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
-
fail
+ +
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
@@ -34,10 +34,10 @@ aria-orientation="horizontal" id="fail30" > -
fail
-
fail
-
fail
-
fail
+
+
+
+
diff --git a/test/integration/rules/aria-allowed-attr/incomplete.html b/test/integration/rules/aria-allowed-attr/incomplete.html index 7391d58120..e187dba1c6 100644 --- a/test/integration/rules/aria-allowed-attr/incomplete.html +++ b/test/integration/rules/aria-allowed-attr/incomplete.html @@ -1,3 +1,4 @@
Foo
Foo
Foo +
Foo
diff --git a/test/integration/rules/aria-allowed-attr/incomplete.json b/test/integration/rules/aria-allowed-attr/incomplete.json index 1d8c7e3fd7..fa37187123 100644 --- a/test/integration/rules/aria-allowed-attr/incomplete.json +++ b/test/integration/rules/aria-allowed-attr/incomplete.json @@ -1,5 +1,10 @@ { "description": "aria-allowed-attr incomplete tests", "rule": "aria-allowed-attr", - "incomplete": [["#incomplete0"], ["#incomplete1"], ["#incomplete2"]] + "incomplete": [ + ["#incomplete0"], + ["#incomplete1"], + ["#incomplete2"], + ["#incomplete3"] + ] }