From de9ba3a345dfb48c0ec75ab049de51cfb066544e Mon Sep 17 00:00:00 2001 From: jkodu Date: Wed, 25 Mar 2020 13:44:28 +0000 Subject: [PATCH 1/6] fix(required-children): handle descendant role when checking for required child roles --- lib/checks/aria/required-children.js | 192 +++++++++++++++----------- test/checks/aria/required-children.js | 60 ++++++++ 2 files changed, 175 insertions(+), 77 deletions(-) diff --git a/lib/checks/aria/required-children.js b/lib/checks/aria/required-children.js index 3341fd6ece..2ef7574612 100644 --- a/lib/checks/aria/required-children.js +++ b/lib/checks/aria/required-children.js @@ -1,58 +1,73 @@ -const requiredOwned = axe.commons.aria.requiredOwned; -const implicitNodes = axe.commons.aria.implicitNodes; -const matchesSelector = axe.utils.matchesSelector; -const idrefs = axe.commons.dom.idrefs; -const hasContentVirtual = axe.commons.dom.hasContentVirtual; +const { aria, dom } = axe.commons; +const { requiredOwned, implicitNodes, getRole } = aria; +const { hasContentVirtual, idrefs } = dom; +const { matchesSelector, querySelectorAll, getNodeFromTree } = axe.utils; + const reviewEmpty = options && Array.isArray(options.reviewEmpty) ? options.reviewEmpty : []; +const role = node.getAttribute('role'); +const required = requiredOwned(role); +if (!required) { + return true; +} -function owns(node, virtualTree, role, ariaOwned) { - if (node === null) { - return false; - } - const implicit = implicitNodes(role); - let selector = ['[role="' + role + '"]']; - - if (implicit) { - selector = selector.concat( - implicit.map(implicitSelector => implicitSelector + ':not([role])') - ); - } +let all = false; +let childRoles = required.one; +if (!childRoles) { + all = true; + childRoles = required.all; +} - selector = selector.join(','); - return ariaOwned - ? matchesSelector(node, selector) || - !!axe.utils.querySelectorAll(virtualTree, selector)[0] - : !!axe.utils.querySelectorAll(virtualTree, selector)[0]; +const missing = missingRequiredChildren(node, childRoles, all, role); +if (!missing) { + return true; } -function ariaOwns(nodes, role) { - for (let index = 0; index < nodes.length; index++) { - const node = nodes[index]; - if (node === null) { - continue; - } - const virtualTree = axe.utils.getNodeFromTree(node); - if (owns(node, virtualTree, role, true)) { - return true; - } - } - return false; +this.data(missing); + +// Only review empty nodes when a node is both empty and does not have an aria-owns relationship +if ( + reviewEmpty.includes(role) && + !hasContentVirtual(virtualNode, false, true) && + !getRolesOfDescendant(node).length && + idrefs(node, 'aria-owns').length === 0 +) { + return undefined; } +return false; + +/** + * Get missing children roles + * @param {HTMLElement} node node + * @param {String[]} childRoles expected children roles + * @param {Boolean} all should all child roles be present? + * @param {String} role role of given node + */ function missingRequiredChildren(node, childRoles, all, role) { - const missing = [], - ownedElements = idrefs(node, 'aria-owns'); + let missing = []; + const ownedElements = idrefs(node, 'aria-owns'); + const descendantRoles = getRolesOfDescendant(node); for (let index = 0; index < childRoles.length; index++) { const childRole = childRoles[index]; - if ( - owns(node, virtualNode, childRole) || - ariaOwns(ownedElements, childRole) - ) { + const ownsRole = owns(node, virtualNode, childRole); + const ariaOwnsRole = ariaOwns(ownedElements, childRole); + if (ownsRole || ariaOwnsRole) { if (!all) { return null; } + + /** + * Verify if descendants contain one of the requested child roles & that a requested child role is not nested within an overriding role + * Only handle when role is not `combobox`, given there is an exception/ different path for `combobox` + * Eg: + * `
List item 1
` + * should fail because `listitem` role not under `list` but has `tabpanel` between them, so although `listitem` is owned by `list` this is a failure. + */ + if (!descendantRoles.includes(childRole) && role !== 'combobox') { + missing.push(childRole); + } } else { if (all) { missing.push(childRole); @@ -106,49 +121,72 @@ function missingRequiredChildren(node, childRoles, all, role) { return null; } -function hasDecendantWithRole(node) { - return ( - node.children && - node.children.some(child => { - const role = axe.commons.aria.getRole(child); - return ( - !['presentation', 'none', null].includes(role) || - hasDecendantWithRole(child) - ); - }) - ); -} - -const role = node.getAttribute('role'); -const required = requiredOwned(role); +/** + * Helper to check if a given node owns an element with a given role + * @param {HTMLElement} node node + * @param {Object} virtualTree virtual node + * @param {String} role role + * @param {Boolean} ariaOwned + * @returns {Boolean} + */ +function owns(node, virtualTree, role, ariaOwned) { + if (node === null) { + return false; + } + const implicit = implicitNodes(role); + let selector = ['[role="' + role + '"]']; -if (!required) { - return true; -} + if (implicit) { + selector = selector.concat( + implicit.map(implicitSelector => implicitSelector + ':not([role])') + ); + } -let all = false; -let childRoles = required.one; -if (!childRoles) { - all = true; - childRoles = required.all; + selector = selector.join(','); + return ariaOwned + ? matchesSelector(node, selector) || + !!querySelectorAll(virtualTree, selector)[0] + : !!querySelectorAll(virtualTree, selector)[0]; } -const missing = missingRequiredChildren(node, childRoles, all, role); +/** + * Helper to check if a given node is `aria-owns` an element with a given role + * @param {HTMLElement[]} nodes nodes + * @param {String} role role + * @returns {Boolean} + */ +function ariaOwns(nodes, role) { + for (let index = 0; index < nodes.length; index++) { + const node = nodes[index]; + if (node === null) { + continue; + } -if (!missing) { - return true; + const virtualTree = getNodeFromTree(node); + if (owns(node, virtualTree, role, true)) { + return true; + } + } + return false; } -this.data(missing); +/** + * Get roles of all descendants for a given node + * @param {HTMLElement} node node + * @returns {String[]} + */ +function getRolesOfDescendant(node) { + if (!node || !node.children) { + return []; + } -// Only review empty nodes when a node is both empty and does not have an aria-owns relationship -if ( - reviewEmpty.includes(role) && - !hasContentVirtual(virtualNode, false, true) && - !hasDecendantWithRole(virtualNode) && - idrefs(node, 'aria-owns').length === 0 -) { - return undefined; -} else { - return false; + return Array.from(node.children).reduce((out, child) => { + const role = getRole(child); + if (['presentation', 'none', null].includes(role)) { + out.push(...getRolesOfDescendant(child)); + } else { + out.push(role); + } + return out; + }, []); } diff --git a/test/checks/aria/required-children.js b/test/checks/aria/required-children.js index a7132dd693..99ae81bcde 100644 --- a/test/checks/aria/required-children.js +++ b/test/checks/aria/required-children.js @@ -103,6 +103,15 @@ describe('aria-required-children', function() { ); }); + it('should pass all existing required children when all required', function() { + var params = checkSetup( + '' + ); + assert.isTrue( + checks['aria-required-children'].evaluate.apply(checkContext, params) + ); + }); + it('should return undefined when element is empty and is in reviewEmpty options', function() { var params = checkSetup('
', { reviewEmpty: ['list'] @@ -246,6 +255,39 @@ describe('aria-required-children', function() { assert.deepEqual(checkContext._data, ['listbox']); }); + it('should fail when list does not have required children listitem', function() { + var params = checkSetup( + '
Item 1
' + ); + assert.isFalse( + checks['aria-required-children'].evaluate.apply(checkContext, params) + ); + + assert.deepEqual(checkContext._data, ['listitem']); + }); + + it('should fail when list has intermediate child with role that is not a required role', function() { + var params = checkSetup( + '
List item 1
' + ); + assert.isFalse( + checks['aria-required-children'].evaluate.apply(checkContext, params) + ); + + assert.deepEqual(checkContext._data, ['listitem']); + }); + + it('should fail when nested child with role row does not have required child role cell', function() { + var params = checkSetup( + '
Item 1
' + ); + assert.isFalse( + checks['aria-required-children'].evaluate.apply(checkContext, params) + ); + + assert.includeMembers(checkContext._data, ['cell']); + }); + it('should pass one indirectly aria-owned child when one required', function() { var params = checkSetup( '
Nothing here.
' @@ -273,6 +315,24 @@ describe('aria-required-children', function() { ); }); + it('should pass one existing aria-owned child when one required', function() { + var params = checkSetup( + '' + ); + assert.isTrue( + checks['aria-required-children'].evaluate.apply(checkContext, params) + ); + }); + + it('should pass one existing aria-owned child when one required', function() { + var params = checkSetup( + 'Item 1
' + ); + assert.isTrue( + checks['aria-required-children'].evaluate.apply(checkContext, params) + ); + }); + it('should pass one existing required child when one required', function() { var params = checkSetup( '

Nothing here.

' From 00829e3113716bdde1ed4a3d36cf11a6ad51a197 Mon Sep 17 00:00:00 2001 From: jkodu Date: Wed, 25 Mar 2020 14:33:32 +0000 Subject: [PATCH 2/6] test: add integration tests --- lib/checks/aria/required-children.js | 6 +++- .../aria-required-children.html | 35 ++++++++++++++++--- .../aria-required-children.json | 20 +++++++++-- 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/lib/checks/aria/required-children.js b/lib/checks/aria/required-children.js index 2ef7574612..d94979fb9a 100644 --- a/lib/checks/aria/required-children.js +++ b/lib/checks/aria/required-children.js @@ -65,7 +65,11 @@ function missingRequiredChildren(node, childRoles, all, role) { * `
List item 1
` * should fail because `listitem` role not under `list` but has `tabpanel` between them, so although `listitem` is owned by `list` this is a failure. */ - if (!descendantRoles.includes(childRole) && role !== 'combobox') { + if ( + !ariaOwnsRole && + role !== 'combobox' && + !descendantRoles.includes(childRole) + ) { missing.push(childRole); } } else { diff --git a/test/integration/rules/aria-required-children/aria-required-children.html b/test/integration/rules/aria-required-children/aria-required-children.html index e3e913eda5..25cca78769 100644 --- a/test/integration/rules/aria-required-children/aria-required-children.html +++ b/test/integration/rules/aria-required-children/aria-required-children.html @@ -1,14 +1,20 @@ -
Item 1
+
+
Item 1
+
-
+
+ +
-
+
+
+
@@ -19,4 +25,25 @@
-
+
+
+
+
+
+
List item 1
+
+
+
+
+ Item 1 +
+
+
+ +
diff --git a/test/integration/rules/aria-required-children/aria-required-children.json b/test/integration/rules/aria-required-children/aria-required-children.json index 7030e1cbcb..31500091ff 100644 --- a/test/integration/rules/aria-required-children/aria-required-children.json +++ b/test/integration/rules/aria-required-children/aria-required-children.json @@ -1,7 +1,15 @@ { "description": "aria-required-children test", "rule": "aria-required-children", - "violations": [["#fail1"], ["#fail2"], ["#fail3"], ["#fail4"], ["#fail5"]], + "violations": [ + ["#fail1"], + ["#fail2"], + ["#fail3"], + ["#fail4"], + ["#fail5"], + ["#fail6"], + ["#fail7"] + ], "passes": [ ["#pass1"], ["#pass2"], @@ -10,7 +18,15 @@ ["#pass5"], ["#pass6"], ["#pass7"], - ["#pass8"] + ["#pass8"], + ["#pass9"], + ["#pass10"], + ["#pass11"], + ["#pass12"], + ["#pass13"], + ["#pass14"], + ["#pass15"], + ["#pass16"] ], "incomplete": [ ["#incomplete1"], From c4247363c8983b6d0aa03f188ffc98a350458325 Mon Sep 17 00:00:00 2001 From: jkodu Date: Wed, 1 Apr 2020 11:10:05 +0100 Subject: [PATCH 3/6] update check and test --- lib/checks/.eslintrc | 2 +- lib/checks/aria/required-children.js | 49 +++++++++++++++++---------- test/checks/aria/required-children.js | 17 +++++++--- 3 files changed, 46 insertions(+), 22 deletions(-) diff --git a/lib/checks/.eslintrc b/lib/checks/.eslintrc index e22052a9b0..357031c0a9 100644 --- a/lib/checks/.eslintrc +++ b/lib/checks/.eslintrc @@ -36,7 +36,7 @@ "strict": 0, "max-params": [ 2, - 5 + 6 ], "max-depth": [ 2, diff --git a/lib/checks/aria/required-children.js b/lib/checks/aria/required-children.js index d94979fb9a..cc30951da2 100644 --- a/lib/checks/aria/required-children.js +++ b/lib/checks/aria/required-children.js @@ -18,7 +18,16 @@ if (!childRoles) { childRoles = required.all; } -const missing = missingRequiredChildren(node, childRoles, all, role); +const ownedElements = idrefs(node, 'aria-owns'); +const descendantRole = getDescendantRole(node, ownedElements); +const missing = missingRequiredChildren( + node, + childRoles, + all, + role, + ownedElements, + descendantRole +); if (!missing) { return true; } @@ -29,7 +38,7 @@ this.data(missing); if ( reviewEmpty.includes(role) && !hasContentVirtual(virtualNode, false, true) && - !getRolesOfDescendant(node).length && + !descendantRole.length && idrefs(node, 'aria-owns').length === 0 ) { return undefined; @@ -44,15 +53,20 @@ return false; * @param {Boolean} all should all child roles be present? * @param {String} role role of given node */ -function missingRequiredChildren(node, childRoles, all, role) { +function missingRequiredChildren( + node, + childRoles, + all, + role, + ownedEls, + descRole +) { let missing = []; - const ownedElements = idrefs(node, 'aria-owns'); - const descendantRoles = getRolesOfDescendant(node); for (let index = 0; index < childRoles.length; index++) { const childRole = childRoles[index]; const ownsRole = owns(node, virtualNode, childRole); - const ariaOwnsRole = ariaOwns(ownedElements, childRole); + const ariaOwnsRole = ariaOwns(ownedEls, childRole); if (ownsRole || ariaOwnsRole) { if (!all) { return null; @@ -65,11 +79,7 @@ function missingRequiredChildren(node, childRoles, all, role) { * `
List item 1
` * should fail because `listitem` role not under `list` but has `tabpanel` between them, so although `listitem` is owned by `list` this is a failure. */ - if ( - !ariaOwnsRole && - role !== 'combobox' && - !descendantRoles.includes(childRole) - ) { + if (role !== 'combobox' && !descRole.includes(childRole)) { missing.push(childRole); } } else { @@ -89,7 +99,7 @@ function missingRequiredChildren(node, childRoles, all, role) { node.nodeName.toUpperCase() === 'INPUT' && textTypeInputs.includes(node.type)) || owns(node, virtualNode, 'searchbox') || - ariaOwns(ownedElements, 'searchbox') + ariaOwns(ownedEls, 'searchbox') ) { missing.splice(textboxIndex, 1); } @@ -175,19 +185,24 @@ function ariaOwns(nodes, role) { } /** - * Get roles of all descendants for a given node + * Get role (that is not presentation or none) of descendant * @param {HTMLElement} node node * @returns {String[]} */ -function getRolesOfDescendant(node) { - if (!node || !node.children) { +function getDescendantRole(node, ownedEls) { + const isOwns = ownedEls && ownedEls.length > 0; + const el = isOwns ? ownedEls[0] : node; + + if (!el) { return []; } - return Array.from(node.children).reduce((out, child) => { + const items = isOwns ? [el, ...el.children] : el.children; + + return Array.from(items).reduce((out, child) => { const role = getRole(child); if (['presentation', 'none', null].includes(role)) { - out.push(...getRolesOfDescendant(child)); + out.push(...getDescendantRole(child)); } else { out.push(role); } diff --git a/test/checks/aria/required-children.js b/test/checks/aria/required-children.js index 99ae81bcde..44460ed8a0 100644 --- a/test/checks/aria/required-children.js +++ b/test/checks/aria/required-children.js @@ -315,18 +315,27 @@ describe('aria-required-children', function() { ); }); - it('should pass one existing aria-owned child when one required', function() { + it('should fail one existing aria-owned child when an intermediate child with role that is not a required role exists', function() { var params = checkSetup( - '
  • Tab 1
  • Tab 2
' + '
' + ); + assert.isFalse( + checks['aria-required-children'].evaluate.apply(checkContext, params) + ); + }); + + it('should pass one existing required child when one required (has explicit role of tab)', function() { + var params = checkSetup( + '
  • Tab 1
  • Tab 2
' ); assert.isTrue( checks['aria-required-children'].evaluate.apply(checkContext, params) ); }); - it('should pass one existing aria-owned child when one required', function() { + it('should pass required child roles (grid contains row, which contains cell)', function() { var params = checkSetup( - 'Item 1
' + '
Item 1
' ); assert.isTrue( checks['aria-required-children'].evaluate.apply(checkContext, params) From ad395e1acc8cfda70dccf05df83ad11668307ccc Mon Sep 17 00:00:00 2001 From: jkodu Date: Wed, 1 Apr 2020 16:03:11 +0100 Subject: [PATCH 4/6] update for ie --- lib/checks/aria/required-children.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checks/aria/required-children.js b/lib/checks/aria/required-children.js index cc30951da2..c3bc9bb657 100644 --- a/lib/checks/aria/required-children.js +++ b/lib/checks/aria/required-children.js @@ -202,7 +202,7 @@ function getDescendantRole(node, ownedEls) { return Array.from(items).reduce((out, child) => { const role = getRole(child); if (['presentation', 'none', null].includes(role)) { - out.push(...getDescendantRole(child)); + out = out.concat(getDescendantRole(child)); } else { out.push(role); } From d4ff9e199dec95dec4d73c99267ab01468f7ae06 Mon Sep 17 00:00:00 2001 From: jkodu Date: Thu, 2 Apr 2020 13:20:10 +0100 Subject: [PATCH 5/6] remove usage of spread operator --- lib/checks/aria/required-children.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checks/aria/required-children.js b/lib/checks/aria/required-children.js index c3bc9bb657..e77b69df45 100644 --- a/lib/checks/aria/required-children.js +++ b/lib/checks/aria/required-children.js @@ -197,7 +197,7 @@ function getDescendantRole(node, ownedEls) { return []; } - const items = isOwns ? [el, ...el.children] : el.children; + const items = isOwns ? [el].concat(el.children) : el.children; return Array.from(items).reduce((out, child) => { const role = getRole(child); From ef9a849fd812cde1fa594ea867e0161f93c75ecf Mon Sep 17 00:00:00 2001 From: jkodu Date: Thu, 2 Apr 2020 13:50:49 +0100 Subject: [PATCH 6/6] update helper fn --- lib/checks/aria/required-children.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/checks/aria/required-children.js b/lib/checks/aria/required-children.js index e77b69df45..a566b215e4 100644 --- a/lib/checks/aria/required-children.js +++ b/lib/checks/aria/required-children.js @@ -197,9 +197,17 @@ function getDescendantRole(node, ownedEls) { return []; } - const items = isOwns ? [el].concat(el.children) : el.children; - - return Array.from(items).reduce((out, child) => { + const items = isOwns + ? Array.from(el.children).reduce( + (out, child) => { + out.push(child); + return out; + }, + [el] + ) + : Array.from(el.children); + + return items.reduce((out, child) => { const role = getRole(child); if (['presentation', 'none', null].includes(role)) { out = out.concat(getDescendantRole(child));