From 3a8729b479dd62fe5e94c5100c1f09a1f5b90683 Mon Sep 17 00:00:00 2001 From: Jey Date: Wed, 18 Jul 2018 10:04:21 +0100 Subject: [PATCH] fix(rule): update for lists, list-items to handle role changes. (#949) This rule updates list(items) to cater to role changes to either parent or child list elements. This rule is a taken over PR from the community contribution (PR: https://github.com/dequelabs/axe-core/pull/518 @AlmeroSteyn - thanks for the initial work). Closes issue: - https://github.com/dequelabs/axe-core/issues/463 ## Reviewer checks **Required fields, to be filled out by PR reviewer(s)** - [x] Follows the commit message policy, appropriate for next version - [x] Has documentation updated, a DU ticket, or requires no documentation change - [x] Includes new tests, or was unnecessary - [x] Code is reviewed for security by: @wilcofiers --- lib/checks/lists/dlitem.js | 24 ++- lib/checks/lists/listitem.js | 19 +- lib/checks/lists/only-dlitems.js | 62 +++--- lib/checks/lists/only-listitems.js | 107 +++++++--- lib/core/utils/flattened-tree.js | 5 +- test/checks/lists/dlitem.js | 69 ++++--- test/checks/lists/listitem.js | 66 +++--- test/checks/lists/only-dlitems.js | 187 +++++++---------- test/checks/lists/only-listitems.js | 188 ++++++++---------- test/core/utils/flattened-tree.js | 4 +- .../definition-list/definition-list.html | 15 +- .../definition-list/definition-list.json | 2 +- test/integration/rules/dlitem/dlitem.html | 7 +- test/integration/rules/dlitem/dlitem.json | 2 +- test/integration/rules/list/list.html | 63 +++++- test/integration/rules/list/list.json | 6 +- test/integration/rules/listitem/listitem.html | 18 +- test/integration/rules/listitem/listitem.json | 2 +- 18 files changed, 477 insertions(+), 369 deletions(-) diff --git a/lib/checks/lists/dlitem.js b/lib/checks/lists/dlitem.js index 80a6608fc7..b4f11e5315 100644 --- a/lib/checks/lists/dlitem.js +++ b/lib/checks/lists/dlitem.js @@ -1,2 +1,22 @@ -var parent = axe.commons.dom.getComposedParent(node); -return parent.nodeName.toUpperCase() === 'DL'; +const parent = axe.commons.dom.getComposedParent(node); +const parentTagName = parent.nodeName.toUpperCase(); + +if (parentTagName !== 'DL') { + return false; +} + +const parentRole = (parent.getAttribute('role') || '').toLowerCase(); + +if (!parentRole) { + return true; +} + +if (!axe.commons.aria.isValidRole(parentRole)) { + return false; +} + +if (parentRole === 'list') { + return true; +} + +return false; diff --git a/lib/checks/lists/listitem.js b/lib/checks/lists/listitem.js index 2feabc4673..3270c8b4e3 100644 --- a/lib/checks/lists/listitem.js +++ b/lib/checks/lists/listitem.js @@ -1,5 +1,18 @@ -var parent = axe.commons.dom.getComposedParent(node); +const parent = axe.commons.dom.getComposedParent(node); +if (!parent) { + return false; +} + +const ALLOWED_TAGS = ['UL', 'OL']; +const parentTagName = parent.nodeName.toUpperCase(); + +const parentRole = (parent.getAttribute('role') || '').toLowerCase(); +if (parentRole && !axe.commons.aria.isValidRole(parentRole)) { + return false; +} + +const isListRole = parentRole === 'list'; return ( - ['UL', 'OL'].includes(parent.nodeName.toUpperCase()) || - (parent.getAttribute('role') || '').toLowerCase() === 'list' + (ALLOWED_TAGS.includes(parentTagName) && (!parentRole || isListRole)) || + isListRole ); diff --git a/lib/checks/lists/only-dlitems.js b/lib/checks/lists/only-dlitems.js index c71891dd6d..5227fb60de 100644 --- a/lib/checks/lists/only-dlitems.js +++ b/lib/checks/lists/only-dlitems.js @@ -1,33 +1,41 @@ -var bad = [], - permitted = [ - 'STYLE', - 'META', - 'LINK', - 'MAP', - 'AREA', - 'SCRIPT', - 'DATALIST', - 'TEMPLATE' - ], - hasNonEmptyTextNode = false; +const ALLOWED_TAGS = [ + 'STYLE', + 'META', + 'LINK', + 'MAP', + 'AREA', + 'SCRIPT', + 'DATALIST', + 'TEMPLATE' +]; -virtualNode.children.forEach(({ actualNode }) => { - var nodeName = actualNode.nodeName.toUpperCase(); - if ( - actualNode.nodeType === 1 && - nodeName !== 'DT' && - nodeName !== 'DD' && - permitted.indexOf(nodeName) === -1 - ) { - bad.push(actualNode); +const ALLOWED_ROLES = ['definition', 'term', 'list']; + +let base = { + badNodes: [], + hasNonEmptyTextNode: false +}; + +const result = virtualNode.children.reduce((out, childNode) => { + const { actualNode } = childNode; + const tagName = actualNode.nodeName.toUpperCase(); + + if (actualNode.nodeType === 1 && !ALLOWED_TAGS.includes(tagName)) { + const role = (actualNode.getAttribute('role') || '').toLowerCase(); + if ((tagName !== 'DT' && tagName !== 'DD') || role) { + if (!ALLOWED_ROLES.includes(role)) { + // handle comment - https://github.com/dequelabs/axe-core/pull/518/files#r139284668 + out.badNodes.push(actualNode); + } + } } else if (actualNode.nodeType === 3 && actualNode.nodeValue.trim() !== '') { - hasNonEmptyTextNode = true; + out.hasNonEmptyTextNode = true; } -}); + return out; +}, base); -if (bad.length) { - this.relatedNodes(bad); +if (result.badNodes.length) { + this.relatedNodes(result.badNodes); } -var retVal = !!bad.length || hasNonEmptyTextNode; -return retVal; +return !!result.badNodes.length || result.hasNonEmptyTextNode; diff --git a/lib/checks/lists/only-listitems.js b/lib/checks/lists/only-listitems.js index cdb88c62db..5a66a4b0e2 100644 --- a/lib/checks/lists/only-listitems.js +++ b/lib/checks/lists/only-listitems.js @@ -1,31 +1,84 @@ -var bad = [], - permitted = [ - 'STYLE', - 'META', - 'LINK', - 'MAP', - 'AREA', - 'SCRIPT', - 'DATALIST', - 'TEMPLATE' - ], - hasNonEmptyTextNode = false; - -virtualNode.children.forEach(({ actualNode }) => { - var nodeName = actualNode.nodeName.toUpperCase(); - if ( - actualNode.nodeType === 1 && - nodeName !== 'LI' && - permitted.indexOf(nodeName) === -1 - ) { - bad.push(actualNode); - } else if (actualNode.nodeType === 3 && actualNode.nodeValue.trim() !== '') { - hasNonEmptyTextNode = true; +const ALLOWED_TAGS = [ + 'STYLE', + 'META', + 'LINK', + 'MAP', + 'AREA', + 'SCRIPT', + 'DATALIST', + 'TEMPLATE' +]; + +const getIsListItemRole = (role, tagName) => { + return role === 'listitem' || (tagName === 'LI' && !role); +}; + +const getHasListItem = (hasListItem, tagName, isListItemRole) => { + return hasListItem || (tagName === 'LI' && isListItemRole) || isListItemRole; +}; + +let base = { + badNodes: [], + isEmpty: true, + hasNonEmptyTextNode: false, + hasListItem: false, + liItemsWithRole: 0 +}; + +let out = virtualNode.children.reduce((out, { actualNode }) => { + /*eslint + max-statements: ["error", 20] + complexity: ["error", 11] + */ + const tagName = actualNode.nodeName.toUpperCase(); + + if (actualNode.nodeType === 1) { + if (!ALLOWED_TAGS.includes(tagName)) { + const role = (actualNode.getAttribute('role') || '').toLowerCase(); + const isListItemRole = getIsListItemRole(role, tagName); + + out.hasListItem = getHasListItem( + out.hasListItem, + tagName, + isListItemRole + ); + + if (isListItemRole) { + out.isEmpty = false; + } + if (tagName === 'LI' && !isListItemRole) { + out.liItemsWithRole++; + } + if (tagName !== 'LI' && !isListItemRole) { + out.badNodes.push(actualNode); + } + } + } + if (actualNode.nodeType === 3) { + if (actualNode.nodeValue.trim() !== '') { + out.hasNonEmptyTextNode = true; + } + } + + return out; +}, base); + +const virtualNodeChildrenOfTypeLi = virtualNode.children.filter( + ({ actualNode }) => { + return actualNode.nodeName.toUpperCase() === 'LI'; } -}); +); + +const allLiItemsHaveRole = + out.liItemsWithRole > 0 && + virtualNodeChildrenOfTypeLi.length === out.liItemsWithRole; -if (bad.length) { - this.relatedNodes(bad); +if (out.badNodes.length) { + this.relatedNodes(out.badNodes); } -return !!bad.length || hasNonEmptyTextNode; +const isInvalidListItem = !( + out.hasListItem || + (out.isEmpty && !allLiItemsHaveRole) +); +return isInvalidListItem || !!out.badNodes.length || out.hasNonEmptyTextNode; diff --git a/lib/core/utils/flattened-tree.js b/lib/core/utils/flattened-tree.js index ada61d96d4..80f977e992 100644 --- a/lib/core/utils/flattened-tree.js +++ b/lib/core/utils/flattened-tree.js @@ -94,7 +94,10 @@ axe.utils.getFlattenedTree = function(node, shadowId) { if (nodeName === 'content') { realArray = Array.from(node.getDistributedNodes()); return realArray.reduce(reduceShadowDOM, []); - } else if (nodeName === 'slot' && typeof node.assignedNodes === 'function') { + } else if ( + nodeName === 'slot' && + typeof node.assignedNodes === 'function' + ) { realArray = Array.from(node.assignedNodes()); if (!realArray.length) { // fallback content diff --git a/test/checks/lists/dlitem.js b/test/checks/lists/dlitem.js index fe85c26535..77e7f144b8 100644 --- a/test/checks/lists/dlitem.js +++ b/test/checks/lists/dlitem.js @@ -1,49 +1,60 @@ -describe('dlitem', function() { +describe('dlitem', function () { 'use strict'; var fixture = document.getElementById('fixture'); var checkSetup = axe.testUtils.checkSetup; var shadowSupport = axe.testUtils.shadowSupport; - afterEach(function() { + afterEach(function () { fixture.innerHTML = ''; }); - it('should pass if the dlitem has a parent
', function() { + it('should pass if the dlitem has a parent
', function () { var checkArgs = checkSetup('
My list item
'); assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs)); }); - it('should fail if the dlitem has an incorrect parent', function() { + it('should fail if the dt element has an incorrect parent', function () { var checkArgs = checkSetup('
'); - assert.isFalse( - checks['only-dlitems'].evaluate.apply(checkContext, checkArgs) - ); + assert.isFalse(checks['only-dlitems'].evaluate.apply(checkContext, checkArgs)); }); - it('should return false if