From 5259e8842e49d291d35aada0fefecfde3627299f Mon Sep 17 00:00:00 2001
From: Steven Lambert <2433219+straker@users.noreply.github.com>
Date: Thu, 17 Nov 2022 02:07:06 -0700
Subject: [PATCH] feat(aria-required-attr): require aria-controls on combobox
 and aria-valuenow on focusable separator (#3786)

* feat(aria-required-attr): require aria-controls on combobox and aria-valuenow on focusable separator

* integration tests
---
 .../aria/aria-required-attr-evaluate.js       |  83 ++++-----
 lib/standards/aria-roles.js                   |   8 +-
 test/checks/aria/aria-required-attr.js        | 158 ++++++++----------
 .../aria-required-attr/required-attr.html     |   3 +
 .../aria-required-attr/required-attr.json     |   7 +-
 5 files changed, 125 insertions(+), 134 deletions(-)

diff --git a/lib/checks/aria/aria-required-attr-evaluate.js b/lib/checks/aria/aria-required-attr-evaluate.js
index f8285efd9a..f10642c4c4 100644
--- a/lib/checks/aria/aria-required-attr-evaluate.js
+++ b/lib/checks/aria/aria-required-attr-evaluate.js
@@ -1,6 +1,10 @@
-import { requiredAttr, getExplicitRole } from '../../commons/aria';
+import {
+  requiredAttr as getRequiredAttrs,
+  getExplicitRole
+} from '../../commons/aria';
 import { getElementSpec } from '../../commons/standards';
 import { uniqueArray } from '../../core/utils';
+import { isFocusable } from '../../commons/dom';
 
 /**
  * Check that the element has all required attributes for its explicit role.
@@ -26,53 +30,52 @@ import { uniqueArray } from '../../core/utils';
  * @memberof checks
  * @return {Boolean} True if all required attributes are present. False otherwise.
  */
-function ariaRequiredAttrEvaluate(node, options = {}, virtualNode) {
-  const missing = [];
-  const attrs = virtualNode.attrNames;
+export default function ariaRequiredAttrEvaluate(
+  node,
+  options = {},
+  virtualNode
+) {
   const role = getExplicitRole(virtualNode);
-  if (attrs.length) {
-    let required = requiredAttr(role);
-    const elmSpec = getElementSpec(virtualNode);
-
-    // @deprecated: required attr options to pass more attrs.
-    // configure the standards spec instead
-    if (Array.isArray(options[role])) {
-      required = uniqueArray(options[role], required);
-    }
-    if (role && required) {
-      for (let i = 0, l = required.length; i < l; i++) {
-        const attr = required[i];
-        if (
-          !virtualNode.attr(attr) &&
-          !(
-            elmSpec.implicitAttrs &&
-            typeof elmSpec.implicitAttrs[attr] !== 'undefined'
-          )
-        ) {
-          missing.push(attr);
-        }
-      }
-    }
+  const attrs = virtualNode.attrNames;
+  // @deprecated: required attr options to pass more attrs.
+  // configure the standards spec instead
+  let requiredAttrs = getRequiredAttrs(role);
+  if (Array.isArray(options[role])) {
+    requiredAttrs = uniqueArray(options[role], requiredAttrs);
   }
-
-  // aria 1.2 combobox requires aria-controls, but aria-owns is acceptable instead in earlier versions of the guidelines. also either is only required if the element has aria-expanded=true
-  // https://github.com/dequelabs/axe-core/issues/2505#issuecomment-788703942
-  // https://github.com/dequelabs/axe-core/issues/2505#issuecomment-881947373
-  const comboboxMissingControls =
-    role === 'combobox' && missing.includes('aria-controls');
+  // Nothing to test
+  if (!role || !attrs.length || !requiredAttrs.length) {
+    return true;
+  }
+  // Some required props are conditional:
   if (
-    comboboxMissingControls &&
-    (virtualNode.hasAttr('aria-owns') ||
-      virtualNode.attr('aria-expanded') !== 'true')
+    isStaticSeparator(virtualNode, role) ||
+    isClosedCombobox(virtualNode, role)
   ) {
-    missing.splice(missing.indexOf('aria-controls', 1));
+    return true;
   }
 
-  if (missing.length) {
-    this.data(missing);
+  const elmSpec = getElementSpec(virtualNode);
+  const missingAttrs = requiredAttrs.filter(
+    requiredAttr =>
+      !virtualNode.attr(requiredAttr) && !hasImplicitAttr(elmSpec, requiredAttr)
+  );
+
+  if (missingAttrs.length) {
+    this.data(missingAttrs);
     return false;
   }
   return true;
 }
 
-export default ariaRequiredAttrEvaluate;
+function isStaticSeparator(vNode, role) {
+  return role === 'separator' && !isFocusable(vNode);
+}
+
+function hasImplicitAttr(elmSpec, attr) {
+  return elmSpec.implicitAttrs?.[attr] !== undefined;
+}
+
+function isClosedCombobox(vNode, role) {
+  return role === 'combobox' && vNode.attr('aria-expanded') === 'false';
+}
diff --git a/lib/standards/aria-roles.js b/lib/standards/aria-roles.js
index 428e786ea0..8edbc02ccd 100644
--- a/lib/standards/aria-roles.js
+++ b/lib/standards/aria-roles.js
@@ -397,8 +397,8 @@ const ariaRoles = {
   },
   meter: {
     type: 'structure',
-    allowedAttrs: ['aria-valuetext'],
-    requiredAttrs: ['aria-valuemax', 'aria-valuemin', 'aria-valuenow'],
+    requiredAttrs: ['aria-valuenow'],
+    allowedAttrs: ['aria-valuemax', 'aria-valuemin', 'aria-valuetext'],
     superclassRole: ['range'],
     accessibleNameRequired: true,
     childrenPresentational: true
@@ -610,14 +610,14 @@ const ariaRoles = {
   },
   separator: {
     type: 'structure',
+    requiredAttrs: ['aria-valuenow'],
     // Note: since the separator role has implicit
     // aria-orientation, aria-valuemax, aria-valuemin, and
-    // aria-valuenow values it is not required to be added by
+    // values it is not required to be added by
     // the user
     allowedAttrs: [
       'aria-valuemax',
       'aria-valuemin',
-      'aria-valuenow',
       'aria-orientation',
       'aria-valuetext'
     ],
diff --git a/test/checks/aria/aria-required-attr.js b/test/checks/aria/aria-required-attr.js
index 0577d0ca91..a18a96e42e 100644
--- a/test/checks/aria/aria-required-attr.js
+++ b/test/checks/aria/aria-required-attr.js
@@ -1,136 +1,120 @@
-describe('aria-required-attr', function () {
-  'use strict';
+describe('aria-required-attr', () => {
+  const { queryFixture, checkSetup } = axe.testUtils;
+  const checkContext = axe.testUtils.MockCheckContext();
+  const requiredAttrCheck =
+    axe.testUtils.getCheckEvaluate('aria-required-attr');
 
-  var queryFixture = axe.testUtils.queryFixture;
-  var checkContext = axe.testUtils.MockCheckContext();
-  var options = undefined;
-  var requiredAttrCheck = axe.testUtils.getCheckEvaluate('aria-required-attr');
-
-  afterEach(function () {
+  afterEach(() => {
     checkContext.reset();
-    axe.reset();
   });
 
-  it('returns true for valid attributes', function () {
-    var vNode = queryFixture(
+  it('returns true for valid attributes', () => {
+    const params = checkSetup(
       '<div id="target" role="switch" tabindex="1" aria-checked="false">'
     );
-    assert.isTrue(
-      requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-    );
+    assert.isTrue(requiredAttrCheck.apply(checkContext, params));
     assert.isNull(checkContext._data);
   });
 
-  it('returns false for missing attributes', function () {
-    var vNode = queryFixture('<div id="target" role="switch" tabindex="1">');
-    assert.isFalse(
-      requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-    );
+  it('returns false for missing attributes', () => {
+    const params = checkSetup('<div id="target" role="switch" tabindex="1">');
+    assert.isFalse(requiredAttrCheck.apply(checkContext, params));
     assert.deepEqual(checkContext._data, ['aria-checked']);
   });
 
-  it('returns false for null attributes', function () {
-    var vNode = queryFixture(
+  it('returns false for null attributes', () => {
+    const params = checkSetup(
       '<div id="target" role="switch" tabindex="1" aria-checked>'
     );
-    assert.isFalse(
-      requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-    );
+    assert.isFalse(requiredAttrCheck.apply(checkContext, params));
     assert.deepEqual(checkContext._data, ['aria-checked']);
   });
 
-  it('returns false for empty attributes', function () {
-    var vNode = queryFixture(
+  it('returns false for empty attributes', () => {
+    const params = checkSetup(
       '<div id="target" role="switch" tabindex="1" aria-checked="">'
     );
-    assert.isFalse(
-      requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-    );
+    assert.isFalse(requiredAttrCheck.apply(checkContext, params));
     assert.deepEqual(checkContext._data, ['aria-checked']);
   });
 
-  it('should return true if there is no role', function () {
-    var vNode = queryFixture('<div id="target">');
-
-    assert.isTrue(
-      requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-    );
+  it('returns true if there is no role', () => {
+    const params = checkSetup('<div id="target"></div>');
+    assert.isTrue(requiredAttrCheck.apply(checkContext, params));
     assert.isNull(checkContext._data);
   });
 
-  it('should pass aria-valuenow if element has value property', function () {
-    var vNode = queryFixture('<input id="target" type="range" role="slider">');
-
-    assert.isTrue(
-      requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-    );
+  it('passes aria-valuenow if element has value property', () => {
+    const params = checkSetup('<input id="target" type="range" role="slider">');
+    assert.isTrue(requiredAttrCheck.apply(checkContext, params));
   });
 
-  it('should pass aria-checkbox if element has checked property', function () {
-    var vNode = queryFixture(
+  it('passes aria-checkbox if element has checked property', () => {
+    const params = checkSetup(
       '<input id="target" type="checkbox" role="switch">'
     );
+    assert.isTrue(requiredAttrCheck.apply(checkContext, params));
+  });
 
-    assert.isTrue(
-      checks['aria-required-attr'].evaluate.call(
-        checkContext,
-        vNode.actualNode,
-        options,
-        vNode
-      )
-    );
+  describe('separator', () => {
+    it('fails a focusable separator', () => {
+      const params = checkSetup(
+        '<div id="target" role="separator" tabindex="0"></div>'
+      );
+      assert.isFalse(requiredAttrCheck.apply(checkContext, params));
+    });
+
+    it('passes a non-focusable separator', () => {
+      const params = checkSetup('<div id="target" role="separator"></div>');
+      assert.isTrue(requiredAttrCheck.apply(checkContext, params));
+    });
   });
 
-  describe('combobox special case', function () {
-    it('should pass comboboxes that have aria-expanded="false"', function () {
-      var vNode = queryFixture(
+  describe('combobox', () => {
+    it('passes comboboxes that have aria-expanded="false"', () => {
+      const params = checkSetup(
         '<div id="target" role="combobox" aria-expanded="false"></div>'
       );
+      assert.isTrue(requiredAttrCheck.apply(checkContext, params));
+    });
 
-      assert.isTrue(
-        requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
+    it('fails comboboxes without aria-controls and with an invalid aria-expanded', () => {
+      const params = checkSetup(
+        '<div id="target" role="combobox" aria-expanded="invalid-value"></div>'
       );
+      assert.isFalse(requiredAttrCheck.apply(checkContext, params));
     });
 
-    it('should pass comboboxes that have aria-owns and aria-expanded', function () {
-      var vNode = queryFixture(
+    it('fails comboboxes that has aria-owns without aria-controls', () => {
+      const params = checkSetup(
         '<div id="target" role="combobox" aria-expanded="true" aria-owns="ownedchild"></div>'
       );
-
-      assert.isTrue(
-        requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-      );
+      assert.isFalse(requiredAttrCheck.apply(checkContext, params));
     });
 
-    it('should pass comboboxes that have aria-controls and aria-expanded', function () {
-      var vNode = queryFixture(
+    it('passes comboboxes that have aria-controls and aria-expanded', () => {
+      const params = checkSetup(
         '<div id="target" role="combobox" aria-expanded="true" aria-controls="test"></div>'
       );
 
-      assert.isTrue(
-        requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-      );
+      assert.isTrue(requiredAttrCheck.apply(checkContext, params));
     });
 
-    it('should fail comboboxes that have no required attributes', function () {
-      var vNode = queryFixture('<div id="target" role="combobox"></div>');
+    it('fails comboboxes that have no required attributes', () => {
+      const params = checkSetup('<div id="target" role="combobox"></div>');
 
-      assert.isFalse(
-        requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-      );
+      assert.isFalse(requiredAttrCheck.apply(checkContext, params));
     });
 
-    it('should fail comboboxes that have aria-expanded only', function () {
-      var vNode = queryFixture(
+    it('fails comboboxes that have aria-expanded only', () => {
+      const params = checkSetup(
         '<div id="target" role="combobox" aria-expanded="true"></div>'
       );
 
-      assert.isFalse(
-        requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
-      );
+      assert.isFalse(requiredAttrCheck.apply(checkContext, params));
     });
 
-    it('should report missing of multiple attributes correctly', function () {
+    it('reports missing of multiple attributes correctly', () => {
       axe.configure({
         standards: {
           ariaRoles: {
@@ -141,18 +125,16 @@ describe('aria-required-attr', function () {
         }
       });
 
-      var vNode = queryFixture(
-        '<div id="target" role="combobox" aria-expanded="false"></div>'
-      );
-      assert.isFalse(
-        requiredAttrCheck.call(checkContext, vNode.actualNode, options, vNode)
+      const params = checkSetup(
+        '<div id="target" role="combobox" aria-expanded="true"></div>'
       );
-      assert.deepEqual(checkContext._data, ['aria-label']);
+      assert.isFalse(requiredAttrCheck.apply(checkContext, params));
+      assert.deepEqual(checkContext._data, ['aria-label', 'aria-controls']);
     });
   });
 
-  describe('options', function () {
-    it('should require provided attribute names for a role', function () {
+  describe('options', () => {
+    it('requires provided attribute names for a role', () => {
       axe.configure({
         standards: {
           ariaRoles: {
@@ -163,8 +145,8 @@ describe('aria-required-attr', function () {
         }
       });
 
-      var vNode = queryFixture('<div role="mccheddarton" id="target"></div>');
-      var options = {
+      const vNode = queryFixture('<div role="mccheddarton" id="target"></div>');
+      const options = {
         mccheddarton: ['aria-snuggles']
       };
       assert.isFalse(
diff --git a/test/integration/rules/aria-required-attr/required-attr.html b/test/integration/rules/aria-required-attr/required-attr.html
index e86489bb75..36f15f64a7 100644
--- a/test/integration/rules/aria-required-attr/required-attr.html
+++ b/test/integration/rules/aria-required-attr/required-attr.html
@@ -50,8 +50,11 @@
 <input type="number" role="spinbutton" id="pass9" value="10" />
 <input type="number" role="spinbutton" id="pass10" />
 <div role="spinbutton" id="pass11">fail</div>
+<div role="separator" id="pass12"></div>
+<div role="separator" id="pass13" tabindex="0" aria-valuenow="foo"></div>
 
 <div role="scrollbar" id="violation1">fail</div>
 <div role="slider" id="violation2">fail</div>
 <div role="heading" id="violation3">fail</div>
 <div role="combobox" id="violation4">fail</div>
+<div role="separator" id="violation5" tabindex="0"></div>
diff --git a/test/integration/rules/aria-required-attr/required-attr.json b/test/integration/rules/aria-required-attr/required-attr.json
index 8db6d97316..071a9976b7 100644
--- a/test/integration/rules/aria-required-attr/required-attr.json
+++ b/test/integration/rules/aria-required-attr/required-attr.json
@@ -5,7 +5,9 @@
     ["#violation1"],
     ["#violation2"],
     ["#violation3"],
-    ["#violation4"]
+    ["#violation4"],
+    ["#violation5"],
+    ["#comboboxWithOwns"]
   ],
   "passes": [
     ["#pass1"],
@@ -19,6 +21,7 @@
     ["#pass9"],
     ["#pass10"],
     ["#pass11"],
-    ["#comboboxWithOwns"]
+    ["#pass12"],
+    ["#pass13"]
   ]
 }