fix(prohibited-attr): always report incomplete if there is text in th…

…e 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
dequelabs · Jan 14, 2022 · 2e27dca · 2e27dca
1 parent d92a7e5
commit 2e27dca
Show file tree

Hide file tree

Showing 6 changed files with 139 additions and 57 deletions.
diff --git 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
@@ -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
@@ -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(
-      '<div id="target" role="code" aria-hidden="false" aria-label="foo">Contents</div>'
+      '<div id="target" role="code" aria-hidden="false" aria-label="foo"></div>'
     );
     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(
+      '<div id="target" role="code" aria-hidden="false" aria-label="foo">Contents</div>'
+    );
+    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(
-      '<div id="target" role="code" aria-hidden="false"  aria-label="foo" aria-labelledby="foo">Contents</div>'
+      '<div id="target" role="code" aria-hidden="false"  aria-label="foo" aria-labelledby="foo"></div>'
     );
     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(
+      '<div id="target" aria-label="foo">Contents</div>'
+    );
+    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(
       '<div id="target" aria-label="foo" aria-labelledby="foo">Contents</div>'
     );
     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(
+      '<div id="target" aria-label="foo"></div>'
+    );
+    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(
       '<div id="target" aria-label="foo" aria-labelledby="foo"></div>'
     );
     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
@@ -1,25 +1,25 @@
-<div role="alert" aria-selected="true" id="fail1">fail</div>
-<div role="link" aria-selected="true" id="fail2">fail</div>
-<div role="row" aria-colcount="value" id="fail3">fail</div>
-<div role="row" aria-rowcount="value" id="fail4">fail</div>
-<div role="caption" aria-label="value" id="fail5">fail</div>
-<div role="caption" aria-labelledby="value" id="fail6">fail</div>
-<div role="code" aria-label="value" id="fail7">fail</div>
-<div role="code" aria-labelledby="value" id="fail8">fail</div>
-<div role="deletion" aria-label="value" id="fail9">fail</div>
-<div role="deletion" aria-labelledby="value" id="fail10">fail</div>
-<div role="emphasis" aria-label="value" id="fail11">fail</div>
-<div role="emphasis" aria-labelledby="value" id="fail12">fail</div>
-<div role="insertion" aria-label="value" id="fail13">fail</div>
-<div role="insertion" aria-labelledby="value" id="fail14">fail</div>
-<div role="paragraph" aria-label="value" id="fail15">fail</div>
-<div role="paragraph" aria-labelledby="value" id="fail16">fail</div>
-<div role="strong" aria-label="value" id="fail17">fail</div>
-<div role="strong" aria-labelledby="value" id="fail18">fail</div>
-<div role="subscript" aria-label="value" id="fail19">fail</div>
-<div role="subscript" aria-labelledby="value" id="fail20">fail</div>
-<div role="superscript" aria-label="value" id="fail21">fail</div>
-<div role="superscript" aria-labelledby="value" id="fail22">fail</div>
+<div role="alert" aria-selected="true" id="fail1"></div>
+<div role="link" aria-selected="true" id="fail2"></div>
+<div role="row" aria-colcount="value" id="fail3"></div>
+<div role="row" aria-rowcount="value" id="fail4"></div>
+<div role="caption" aria-label="value" id="fail5"></div>
+<div role="caption" aria-labelledby="value" id="fail6"></div>
+<div role="code" aria-label="value" id="fail7"></div>
+<div role="code" aria-labelledby="value" id="fail8"></div>
+<div role="deletion" aria-label="value" id="fail9"></div>
+<div role="deletion" aria-labelledby="value" id="fail10"></div>
+<div role="emphasis" aria-label="value" id="fail11"></div>
+<div role="emphasis" aria-labelledby="value" id="fail12"></div>
+<div role="insertion" aria-label="value" id="fail13"></div>
+<div role="insertion" aria-labelledby="value" id="fail14"></div>
+<div role="paragraph" aria-label="value" id="fail15"></div>
+<div role="paragraph" aria-labelledby="value" id="fail16"></div>
+<div role="strong" aria-label="value" id="fail17"></div>
+<div role="strong" aria-labelledby="value" id="fail18"></div>
+<div role="subscript" aria-label="value" id="fail19"></div>
+<div role="subscript" aria-labelledby="value" id="fail20"></div>
+<div role="superscript" aria-label="value" id="fail21"></div>
+<div role="superscript" aria-labelledby="value" id="fail22"></div>
 <div aria-label="value" id="fail23"></div>
 <div aria-labelledby="value" id="fail24"></div>
 <!-- technically presentation and none roles do not allow aria-label and aria-labelledby, but since those are global attributes the presentation role conflict will not resolve the roles to none or presentation -->
@@ -34,10 +34,10 @@
   aria-orientation="horizontal"
   id="fail30"
 ></audio>
-<div role="mark" aria-label="value" id="fail31">fail</div>
-<div role="mark" aria-labelledby="value" id="fail32">fail</div>
-<div role="suggestion" aria-label="value" id="fail33">fail</div>
-<div role="suggestion" aria-labelledby="value" id="fail34">fail</div>
+<div role="mark" aria-label="value" id="fail31"></div>
+<div role="mark" aria-labelledby="value" id="fail32"></div>
+<div role="suggestion" aria-label="value" id="fail33"></div>
+<div role="suggestion" aria-labelledby="value" id="fail34"></div>
 
 <div role="table">
   <div role="row" aria-expanded="false" id="fail35"></div>

diff --git a/test/integration/rules/aria-allowed-attr/incomplete.html b/test/integration/rules/aria-allowed-attr/incomplete.html
@@ -1,3 +1,4 @@
 <div id="incomplete0" aria-label="foo">Foo</div>
 <div id="incomplete1" aria-labelledby="missing">Foo</div>
 <my-custom-elm id="incomplete2" aria-expanded="true">Foo</my-custom-elm>
+<div id="incomplete3" aria-label="foo" role="code">Foo</div>
diff --git 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"]
+  ]
 }