From b2d1bb618e761865a6062da79fcc7627f8f4a71d Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Fri, 14 Jul 2023 19:07:13 +0200 Subject: [PATCH 1/5] fix(aria-required-childen): test visibility of grandchildren --- .../aria/aria-required-children-evaluate.js | 28 +-- test/checks/aria/required-children.js | 218 +++++++++++------- .../aria-required-children.html | 12 + .../aria-required-children.json | 3 +- 4 files changed, 159 insertions(+), 102 deletions(-) diff --git a/lib/checks/aria/aria-required-children-evaluate.js b/lib/checks/aria/aria-required-children-evaluate.js index beef5289af..098140592c 100644 --- a/lib/checks/aria/aria-required-children-evaluate.js +++ b/lib/checks/aria/aria-required-children-evaluate.js @@ -76,18 +76,20 @@ export default function ariaRequiredChildrenEvaluate( */ function getOwnedRoles(virtualNode, required) { const ownedRoles = []; - const ownedElements = getOwnedVirtual(virtualNode).filter(vNode => { - return vNode.props.nodeType !== 1 || isVisibleToScreenReaders(vNode); - }); - - for (let i = 0; i < ownedElements.length; i++) { - const ownedElement = ownedElements[i]; - if (ownedElement.props.nodeType !== 1) { + const ownedElements = []; + + const ownedVirtual = getOwnedVirtual(virtualNode); + while (ownedVirtual.length) { + const ownedElement = ownedVirtual.shift(); + const { props, children } = ownedElement; + if (props.nodeType === 3) { + ownedElements.push(ownedElement); + } + if (props.nodeType !== 1 || !isVisibleToScreenReaders(ownedElement)) { continue; } const role = getRole(ownedElement, { noPresentational: true }); - const globalAriaAttr = getGlobalAriaAttr(ownedElement); const hasGlobalAriaOrFocusable = !!globalAriaAttr || isFocusable(ownedElement); @@ -101,8 +103,9 @@ function getOwnedRoles(virtualNode, required) { (['group', 'rowgroup'].includes(role) && required.some(requiredRole => requiredRole === role)) ) { - ownedElements.push(...ownedElement.children); + ownedVirtual.push(...children); } else if (role || hasGlobalAriaOrFocusable) { + ownedElements.push(ownedElement); ownedRoles.push({ role, attr: globalAriaAttr || 'tabindex', @@ -140,7 +143,9 @@ function missingRequiredChildren(virtualNode, role, required, ownedRoles) { * @return {String|null} */ function getGlobalAriaAttr(vNode) { - return getGlobalAriaAttrs().find(attr => vNode.hasAttr(attr)); + return getGlobalAriaAttrs().find( + attr => attr !== 'aria-hidden' && vNode.hasAttr(attr) + ); } /** @@ -151,7 +156,6 @@ function getGlobalAriaAttr(vNode) { */ function getUnallowedSelector(vNode, attr) { const { nodeName, nodeType } = vNode.props; - if (nodeType === 3) { return `#text`; } @@ -160,11 +164,9 @@ function getUnallowedSelector(vNode, attr) { if (role) { return `[role=${role}]`; } - if (attr) { return nodeName + `[${attr}]`; } - return nodeName; } diff --git a/test/checks/aria/required-children.js b/test/checks/aria/required-children.js index 1b7165b127..d216a974b9 100644 --- a/test/checks/aria/required-children.js +++ b/test/checks/aria/required-children.js @@ -293,6 +293,20 @@ describe('aria-required-children', () => { assert.isTrue(requiredChildrenCheck.apply(checkContext, params)); }); + it('should ignore hidden children inside the group', () => { + const params = checkSetup(` + + `); + assert.isTrue(requiredChildrenCheck.apply(checkContext, params)); + }); + it('should fail when role allows group and group does not have required child', () => { const params = checkSetup( '' @@ -329,19 +343,6 @@ describe('aria-required-children', () => { }); describe('options', () => { - it('should return undefined instead of false when the role is in options.reviewEmpty', () => { - const params = checkSetup('
', { - reviewEmpty: [] - }); - assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); - - // Options: - params[1] = { - reviewEmpty: ['grid'] - }; - assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); - }); - it('should not throw when options is incorrect', () => { const params = checkSetup('
'); @@ -358,88 +359,129 @@ describe('aria-required-children', () => { assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); }); - it('should return undefined when the element has empty children', () => { - const params = checkSetup( - '
' - ); - params[1] = { - reviewEmpty: ['listbox'] - }; - assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); - }); + describe('reviewEmpty', () => { + it('should return undefined instead of false when the role is in options.reviewEmpty', () => { + const params = checkSetup('
', { + reviewEmpty: [] + }); + assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); + + // Options: + params[1] = { + reviewEmpty: ['grid'] + }; + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + }); - it('should return false when the element has empty child with role', () => { - const params = checkSetup( - '
' - ); - params[1] = { - reviewEmpty: ['listbox'] - }; - assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); - }); + it('should return undefined when the element has empty children', () => { + const params = checkSetup( + '
' + ); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + }); - it('should return undefined when there is a empty text node', () => { - const params = checkSetup( - '
  \n\t
' - ); - params[1] = { - reviewEmpty: ['listbox'] - }; - assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); - }); + it('should return false when the element has empty child with role', () => { + const params = checkSetup( + '
' + ); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); + }); - it('should return false when there is a non-empty text node', () => { - const params = checkSetup( - '
hello
' - ); - params[1] = { - reviewEmpty: ['listbox'] - }; - assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); - }); + it('should return undefined when there is a empty text node', () => { + const params = checkSetup( + '
  \n\t
' + ); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + }); - it('should return undefined when the element has empty child with role=presentation', () => { - const params = checkSetup( - '
' - ); - params[1] = { - reviewEmpty: ['listbox'] - }; - assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); - }); + it('should return false when there is a non-empty text node', () => { + const params = checkSetup( + '
hello
' + ); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); + }); - it('should return undefined when the element has empty child with role=none', () => { - const params = checkSetup( - '
' - ); - params[1] = { - reviewEmpty: ['listbox'] - }; - assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); - }); + it('should return undefined when the element has empty child with role=presentation', () => { + const params = checkSetup( + '
' + ); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + }); - it('should return undefined when the element has hidden children', () => { - const params = checkSetup( - `` - ); - params[1] = { - reviewEmpty: ['menu'] - }; - assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); - }); + it('should return false when role=none child has visible content', () => { + const params = checkSetup( + '
hello
' + ); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); + }); + + it('should return undefined when role=none child has hidden content', () => { + const params = checkSetup(` +
+
+

hello

+

hello

+

hello

+
+
+ `); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + }); + + it('should return undefined when the element has empty child with role=none', () => { + const params = checkSetup( + '
' + ); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + }); + + it('should return undefined when the element has hidden children', () => { + const params = checkSetup( + `` + ); + params[1] = { + reviewEmpty: ['menu'] + }; + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + }); - it('should return undefined when the element has empty child and aria-label', () => { - const params = checkSetup( - '
' - ); - params[1] = { - reviewEmpty: ['listbox'] - }; - assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + it('should return undefined when the element has empty child and aria-label', () => { + const params = checkSetup( + '
' + ); + params[1] = { + reviewEmpty: ['listbox'] + }; + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); + }); }); }); }); 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 410434d972..def03ca181 100644 --- a/test/integration/rules/aria-required-children/aria-required-children.html +++ b/test/integration/rules/aria-required-children/aria-required-children.html @@ -27,6 +27,9 @@
@@ -143,3 +146,12 @@
menu item
+ +
+
+
ignore
+ +
  • item 1
    • +
  • item 2
    • +
    +
    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 23f2822f0a..21f2ed2f3a 100644 --- a/test/integration/rules/aria-required-children/aria-required-children.json +++ b/test/integration/rules/aria-required-children/aria-required-children.json @@ -33,7 +33,8 @@ ["#pass15"], ["#pass16"], ["#pass17"], - ["#pass18"] + ["#pass18"], + ["#pass19"] ], "incomplete": [ ["#incomplete1"], From 4bc61cbf4bd0a33aa51d6e91a7ac2b2bb417b5aa Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Mon, 17 Jul 2023 11:40:08 +0200 Subject: [PATCH 2/5] Little refactoring --- .../aria/aria-required-children-evaluate.js | 4 +- test/checks/aria/required-children.js | 67 +++++++------------ 2 files changed, 25 insertions(+), 46 deletions(-) diff --git a/lib/checks/aria/aria-required-children-evaluate.js b/lib/checks/aria/aria-required-children-evaluate.js index 098140592c..c376ba1971 100644 --- a/lib/checks/aria/aria-required-children-evaluate.js +++ b/lib/checks/aria/aria-required-children-evaluate.js @@ -143,9 +143,7 @@ function missingRequiredChildren(virtualNode, role, required, ownedRoles) { * @return {String|null} */ function getGlobalAriaAttr(vNode) { - return getGlobalAriaAttrs().find( - attr => attr !== 'aria-hidden' && vNode.hasAttr(attr) - ); + return getGlobalAriaAttrs().find(attr => vNode.hasAttr(attr)); } /** diff --git a/test/checks/aria/required-children.js b/test/checks/aria/required-children.js index d216a974b9..dcd8a1ae5b 100644 --- a/test/checks/aria/required-children.js +++ b/test/checks/aria/required-children.js @@ -375,87 +375,72 @@ describe('aria-required-children', () => { it('should return undefined when the element has empty children', () => { const params = checkSetup( - '
    ' + '
    ', + { reviewEmpty: ['listbox'] } ); - params[1] = { - reviewEmpty: ['listbox'] - }; assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); }); it('should return false when the element has empty child with role', () => { const params = checkSetup( - '
    ' + '
    ', + { reviewEmpty: ['listbox'] } ); - params[1] = { - reviewEmpty: ['listbox'] - }; assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); }); it('should return undefined when there is a empty text node', () => { const params = checkSetup( - '
      \n\t
    ' + '
      \n\t
    ', + { reviewEmpty: ['listbox'] } ); - params[1] = { - reviewEmpty: ['listbox'] - }; assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); }); it('should return false when there is a non-empty text node', () => { const params = checkSetup( - '
    hello
    ' + '
    hello
    ', + { reviewEmpty: ['listbox'] } ); - params[1] = { - reviewEmpty: ['listbox'] - }; assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); }); it('should return undefined when the element has empty child with role=presentation', () => { const params = checkSetup( - '
    ' + '
    ', + { reviewEmpty: ['listbox'] } ); - params[1] = { - reviewEmpty: ['listbox'] - }; assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); }); it('should return false when role=none child has visible content', () => { const params = checkSetup( - '
    hello
    ' + '
    hello
    ', + { reviewEmpty: ['listbox'] } ); - params[1] = { - reviewEmpty: ['listbox'] - }; assert.isFalse(requiredChildrenCheck.apply(checkContext, params)); }); it('should return undefined when role=none child has hidden content', () => { - const params = checkSetup(` -
    + const params = checkSetup( + `

    hello

    hello

    hello

    -
    - `); - params[1] = { - reviewEmpty: ['listbox'] - }; +
    `, + { reviewEmpty: ['listbox'] } + ); + assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); }); it('should return undefined when the element has empty child with role=none', () => { const params = checkSetup( - '
    ' + '
    ', + { reviewEmpty: ['listbox'] } ); - params[1] = { - reviewEmpty: ['listbox'] - }; assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); }); @@ -465,21 +450,17 @@ describe('aria-required-children', () => { - ` + `, + { reviewEmpty: ['menu'] } ); - params[1] = { - reviewEmpty: ['menu'] - }; assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); }); it('should return undefined when the element has empty child and aria-label', () => { const params = checkSetup( - '
    ' + '
    ', + { reviewEmpty: ['listbox'] } ); - params[1] = { - reviewEmpty: ['listbox'] - }; assert.isUndefined(requiredChildrenCheck.apply(checkContext, params)); }); }); From 20cdca9935b2784f15ca6f0e079f71ff06c7c108 Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Mon, 17 Jul 2023 12:45:58 +0200 Subject: [PATCH 3/5] More refactoring --- .../aria/aria-required-children-evaluate.js | 77 ++++++------------- test/checks/aria/required-children.js | 11 ++- 2 files changed, 34 insertions(+), 54 deletions(-) diff --git a/lib/checks/aria/aria-required-children-evaluate.js b/lib/checks/aria/aria-required-children-evaluate.js index c376ba1971..5a141171dd 100644 --- a/lib/checks/aria/aria-required-children-evaluate.js +++ b/lib/checks/aria/aria-required-children-evaluate.js @@ -34,37 +34,31 @@ export default function ariaRequiredChildrenEvaluate( return true; } - const { ownedRoles, ownedElements } = getOwnedRoles(virtualNode, required); - const unallowed = ownedRoles.filter(({ role }) => !required.includes(role)); + const ownedRoles = getOwnedRoles(virtualNode, required); + const unallowed = ownedRoles.filter( + ({ role, vNode }) => vNode.props.nodeType === 1 && !required.includes(role) + ); if (unallowed.length) { - this.relatedNodes(unallowed.map(({ ownedElement }) => ownedElement)); + this.relatedNodes(unallowed.map(({ vNode }) => vNode)); this.data({ messageKey: 'unallowed', values: unallowed - .map(({ ownedElement, attr }) => - getUnallowedSelector(ownedElement, attr) - ) + .map(({ vNode, attr }) => getUnallowedSelector(vNode, attr)) .filter((selector, index, array) => array.indexOf(selector) === index) .join(', ') }); return false; } - const missing = missingRequiredChildren( - virtualNode, - role, - required, - ownedRoles - ); - if (!missing) { + if (hasRequiredChildren(required, ownedRoles)) { return true; } - this.data(missing); + this.data(required); // Only review empty nodes when a node is both empty and does not have an aria-owns relationship - if (reviewEmpty.includes(role) && !ownedElements.some(isContent)) { + if (reviewEmpty.includes(role) && !ownedRoles.some(isContent)) { return undefined; } @@ -75,24 +69,20 @@ export default function ariaRequiredChildrenEvaluate( * Get all owned roles of an element */ function getOwnedRoles(virtualNode, required) { + let vNode; const ownedRoles = []; - const ownedElements = []; - const ownedVirtual = getOwnedVirtual(virtualNode); - while (ownedVirtual.length) { - const ownedElement = ownedVirtual.shift(); - const { props, children } = ownedElement; - if (props.nodeType === 3) { - ownedElements.push(ownedElement); + while ((vNode = ownedVirtual.shift())) { + if (vNode.props.nodeType === 3) { + ownedRoles.push({ vNode, role: null }); } - if (props.nodeType !== 1 || !isVisibleToScreenReaders(ownedElement)) { + if (vNode.props.nodeType !== 1 || !isVisibleToScreenReaders(vNode)) { continue; } - const role = getRole(ownedElement, { noPresentational: true }); - const globalAriaAttr = getGlobalAriaAttr(ownedElement); - const hasGlobalAriaOrFocusable = - !!globalAriaAttr || isFocusable(ownedElement); + const role = getRole(vNode, { noPresentational: true }); + const globalAriaAttr = getGlobalAriaAttr(vNode); + const hasGlobalAriaOrFocusable = !!globalAriaAttr || isFocusable(vNode); // if owned node has no role or is presentational, or if role // allows group or rowgroup, we keep parsing the descendant tree. @@ -103,38 +93,21 @@ function getOwnedRoles(virtualNode, required) { (['group', 'rowgroup'].includes(role) && required.some(requiredRole => requiredRole === role)) ) { - ownedVirtual.push(...children); + ownedVirtual.push(...vNode.children); } else if (role || hasGlobalAriaOrFocusable) { - ownedElements.push(ownedElement); - ownedRoles.push({ - role, - attr: globalAriaAttr || 'tabindex', - ownedElement - }); + const attr = globalAriaAttr || 'tabindex'; + ownedRoles.push({ role, attr, vNode }); } } - return { ownedRoles, ownedElements }; + return ownedRoles; } /** - * Get missing children roles + * See if any required roles are in the ownedRoles array */ -function missingRequiredChildren(virtualNode, role, required, ownedRoles) { - for (let i = 0; i < ownedRoles.length; i++) { - const { role } = ownedRoles[i]; - - if (required.includes(role)) { - required = required.filter(requiredRole => requiredRole !== role); - return null; - } - } - - if (required.length) { - return required; - } - - return null; +function hasRequiredChildren(required, ownedRoles) { + return ownedRoles.some(({ role }) => role && required.includes(role)); } /** @@ -173,7 +146,7 @@ function getUnallowedSelector(vNode, attr) { * @param {VirtualNode} vNode * @returns {Boolean} */ -function isContent(vNode) { +function isContent({ vNode }) { if (vNode.props.nodeType === 3) { return vNode.props.nodeValue.trim().length > 0; } diff --git a/test/checks/aria/required-children.js b/test/checks/aria/required-children.js index dcd8a1ae5b..87dc71aac2 100644 --- a/test/checks/aria/required-children.js +++ b/test/checks/aria/required-children.js @@ -69,9 +69,16 @@ describe('aria-required-children', () => { it('should pass all existing required children when all required', () => { const params = checkSetup( - '' + `` ); - assert.isTrue(requiredChildrenCheck.apply(checkContext, params)); + const out = requiredChildrenCheck.apply(checkContext, params); + console.log(checkContext._data); + assert.isTrue(out); }); it('should return undefined when element is empty and is in reviewEmpty options', () => { From 6856b87429bda0e2f42fa1a099882f91dcfcda99 Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Mon, 24 Jul 2023 18:26:12 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com> --- test/checks/aria/required-children.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/checks/aria/required-children.js b/test/checks/aria/required-children.js index 87dc71aac2..06d09cb044 100644 --- a/test/checks/aria/required-children.js +++ b/test/checks/aria/required-children.js @@ -76,9 +76,7 @@ describe('aria-required-children', () => {
    Item 3
    ` ); - const out = requiredChildrenCheck.apply(checkContext, params); - console.log(checkContext._data); - assert.isTrue(out); + assert.isTrue(requiredChildrenCheck.apply(checkContext, params)) }); it('should return undefined when element is empty and is in reviewEmpty options', () => { From 5a5d7d841f32b21ce19b7ee1a7890cc8dacfa0c2 Mon Sep 17 00:00:00 2001 From: WilcoFiers Date: Mon, 24 Jul 2023 16:27:54 +0000 Subject: [PATCH 5/5] :robot: Automated formatting fixes --- test/checks/aria/required-children.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/checks/aria/required-children.js b/test/checks/aria/required-children.js index 06d09cb044..fe5a15ff82 100644 --- a/test/checks/aria/required-children.js +++ b/test/checks/aria/required-children.js @@ -76,7 +76,7 @@ describe('aria-required-children', () => {
    Item 3
    ` ); - assert.isTrue(requiredChildrenCheck.apply(checkContext, params)) + assert.isTrue(requiredChildrenCheck.apply(checkContext, params)); }); it('should return undefined when element is empty and is in reviewEmpty options', () => {