From cc4274f42b3aae6e04c80c8a918cc9f72558bbbd Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Sat, 8 Oct 2022 21:49:34 +0100 Subject: [PATCH 1/3] fix(list/definition-list): Clarify remediation messages --- lib/checks/lists/invalid-children-evaluate.js | 75 ++++ lib/checks/lists/only-dlitems.json | 11 +- lib/checks/lists/only-listitems.json | 11 +- lib/rules/no-role-matches.js | 4 +- locales/_template.json | 9 +- test/checks/lists/only-dlitems.js | 354 ++++++------------ test/checks/lists/only-listitems.js | 342 +++++++---------- test/integration/rules/list/list.json | 8 +- .../virtual-rules/definition-list.js | 53 +++ test/integration/virtual-rules/list.js | 36 ++ test/rule-matches/no-role-matches.js | 37 +- 11 files changed, 453 insertions(+), 487 deletions(-) create mode 100644 lib/checks/lists/invalid-children-evaluate.js create mode 100644 test/integration/virtual-rules/definition-list.js create mode 100644 test/integration/virtual-rules/list.js diff --git a/lib/checks/lists/invalid-children-evaluate.js b/lib/checks/lists/invalid-children-evaluate.js new file mode 100644 index 0000000000..fd9485fd32 --- /dev/null +++ b/lib/checks/lists/invalid-children-evaluate.js @@ -0,0 +1,75 @@ +import { isVisibleToScreenReaders } from '../../commons/dom'; +import { getExplicitRole } from '../../commons/aria'; + +export default function invalidChildrenEvaluate( + node, + options = {}, + virtualNode +) { + const relatedNodes = []; + const issues = []; + if (!virtualNode.children) { + return undefined; + } + + const vChildren = mapWithNested(virtualNode.children); + while (vChildren.length) { + const { vChild, nested } = vChildren.shift(); + if (options.divGroups && !nested && isDivGroup(vChild)) { + if (!vChild.children) { + return undefined; + } + const vGrandChildren = mapWithNested(vChild.children, true); + vChildren.push(...vGrandChildren); + continue; + } + + const issue = isInvalidChild(vChild, nested, options); + if (!issue) { + continue; + } + if (!issues.includes(issue)) { + issues.push(issue); + } + if (vChild?.actualNode?.nodeType === 1) { + relatedNodes.push(vChild.actualNode); + } + } + if (issues.length === 0) { + return false; + } + + this.data({ values: issues.join(', ') }); + this.relatedNodes(relatedNodes); + return true; +} + +function isInvalidChild( + vChild, + nested, + { validRoles = [], validNodeNames = [] } +) { + const { nodeName, nodeType, nodeValue } = vChild.props; + const selector = nested ? 'div > ' : ''; + if (nodeType === 3 && nodeValue.trim() !== '') { + return selector + `#text`; + } + if (nodeType !== 1 || !isVisibleToScreenReaders(vChild)) { + return false; + } + + const role = getExplicitRole(vChild); + if (role) { + return validRoles.includes(role) ? false : selector + `[role=${role}]`; + } else { + return validNodeNames.includes(nodeName) ? false : selector + nodeName; + } +} + +function isDivGroup(vNode) { + return vNode.props.nodeName === 'div' && getExplicitRole(vNode) === null; +} + +function mapWithNested(vNodes, nested = false) { + return vNodes.map(vChild => ({ vChild, nested })); +} diff --git a/lib/checks/lists/only-dlitems.json b/lib/checks/lists/only-dlitems.json index 580af8b16f..91ddf26b16 100644 --- a/lib/checks/lists/only-dlitems.json +++ b/lib/checks/lists/only-dlitems.json @@ -1,11 +1,16 @@ { "id": "only-dlitems", - "evaluate": "only-dlitems-evaluate", + "evaluate": "invalid-children-evaluate", + "options": { + "validRoles": ["definition", "term", "listitem"], + "validNodeNames": ["dt", "dd"], + "divGroups": true + }, "metadata": { "impact": "serious", "messages": { - "pass": "List element only has direct children that are allowed inside
or
elements", - "fail": "List element has direct children that are not allowed inside
or
elements" + "pass": "dl element only has direct children that are allowed inside;
,
, or
elements", + "fail": "dl element has direct children that are not allowed: ${data.values}" } } } diff --git a/lib/checks/lists/only-listitems.json b/lib/checks/lists/only-listitems.json index 26426048be..946eeadca4 100644 --- a/lib/checks/lists/only-listitems.json +++ b/lib/checks/lists/only-listitems.json @@ -1,14 +1,15 @@ { "id": "only-listitems", - "evaluate": "only-listitems-evaluate", + "evaluate": "invalid-children-evaluate", + "options": { + "validRoles": ["listitem"], + "validNodeNames": ["li"] + }, "metadata": { "impact": "serious", "messages": { "pass": "List element only has direct children that are allowed inside
  • elements", - "fail": { - "default": "List element has direct children that are not allowed inside
  • elements", - "roleNotValid": "List element has direct children with a role that is not allowed: ${data.roles}" - } + "fail": "List element has direct children that are not allowed: ${data.values}" } } } diff --git a/lib/rules/no-role-matches.js b/lib/rules/no-role-matches.js index 968452ec00..d9ee28f99b 100644 --- a/lib/rules/no-role-matches.js +++ b/lib/rules/no-role-matches.js @@ -1,5 +1,5 @@ -function noRoleMatches(node) { - return !node.getAttribute('role'); +function noRoleMatches(node, vNode) { + return !vNode.attr('role'); } export default noRoleMatches; diff --git a/locales/_template.json b/locales/_template.json index 1d3201fbf0..4bf9ce275f 100644 --- a/locales/_template.json +++ b/locales/_template.json @@ -782,15 +782,12 @@ } }, "only-dlitems": { - "pass": "List element only has direct children that are allowed inside
    or
    elements", - "fail": "List element has direct children that are not allowed inside
    or
    elements" + "pass": "dl element only has direct children that are allowed inside;
    ,
    , or
    elements", + "fail": "dl element has direct children that are not allowed: ${data.values}" }, "only-listitems": { "pass": "List element only has direct children that are allowed inside
  • elements", - "fail": { - "default": "List element has direct children that are not allowed inside
  • elements", - "roleNotValid": "List element has direct children with a role that is not allowed: ${data.roles}" - } + "fail": "List element has direct children that are not allowed: ${data.values}" }, "structured-dlitems": { "pass": "When not empty, element has both
    and
    elements", diff --git a/test/checks/lists/only-dlitems.js b/test/checks/lists/only-dlitems.js index 29d7040bf6..fd7af0dc06 100644 --- a/test/checks/lists/only-dlitems.js +++ b/test/checks/lists/only-dlitems.js @@ -1,349 +1,227 @@ -describe('only-dlitems', function () { - 'use strict'; +describe('only-dlitems', () => { + const fixture = document.getElementById('fixture'); + const checkSetup = axe.testUtils.checkSetup; + const checkContext = axe.testUtils.MockCheckContext(); + const checkEvaluate = axe.testUtils.getCheckEvaluate('only-dlitems'); - var fixture = document.getElementById('fixture'); - var checkSetup = axe.testUtils.checkSetup; - var shadowSupport = axe.testUtils.shadowSupport; - var checkContext = axe.testUtils.MockCheckContext(); - - afterEach(function () { - fixture.innerHTML = ''; + afterEach(() => { checkContext.reset(); }); - it('should return false if the list has no contents', function () { - var checkArgs = checkSetup('
    '); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + it('should return false if the list has no contents', () => { + const checkArgs = checkSetup('
    '); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return true if the list has non-dd/dt contents', function () { - var checkArgs = checkSetup('

    Not a list

    '); + it('should return true if the list has non-dd/dt contents', () => { + const checkArgs = checkSetup('

    Not a list

    '); - assert.isTrue( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isTrue(checkEvaluate.apply(checkContext, checkArgs)); assert.deepEqual(checkContext._relatedNodes, [fixture.querySelector('p')]); + assert.deepEqual(checkContext._data, { values: 'p' }); }); - it('should return true if the list has non-dd content through role change', function () { - var checkArgs = checkSetup( + it('should return true if the list has non-dd content through role change', () => { + const checkArgs = checkSetup( '
    Not a list
    ' ); - assert.isTrue( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isTrue(checkEvaluate.apply(checkContext, checkArgs)); + assert.deepEqual(checkContext._data, { values: '[role=menuitem]' }); }); - it('should return true if the list has non-dt content through role change', function () { - var checkArgs = checkSetup( + it('should return true if the list has non-dt content through role change', () => { + const checkArgs = checkSetup( '
    Not a list
    ' ); - - assert.isTrue( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isTrue(checkEvaluate.apply(checkContext, checkArgs)); + assert.deepEqual(checkContext._data, { values: '[role=menuitem]' }); }); - it('should return false if the list has only a dd', function () { - var checkArgs = checkSetup('
    A list
    '); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + it('should return false if the list has only a dd', () => { + const checkArgs = checkSetup('
    A list
    '); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return true if is used along side dt with its role changed', function () { - var checkArgs = checkSetup( + it('should return true if is used along side dt with its role changed', () => { + const checkArgs = checkSetup( '
    A list
    ' ); - assert.isTrue( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isTrue(checkEvaluate.apply(checkContext, checkArgs)); + assert.deepEqual(checkContext._data, { values: '[role=menuitem]' }); }); - it('should return false if the list has only a dt', function () { - var checkArgs = checkSetup('
    A list
    '); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + it('should return false if the list has only a dt', () => { + const checkArgs = checkSetup('
    A list
    '); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return false if the list has dt and dd with child content', function () { - var checkArgs = checkSetup( + it('should return false if the list has dt and dd with child content', () => { + const checkArgs = checkSetup( '

    An item

    A list
    ' ); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return false if the list has dt and dd', function () { - var checkArgs = checkSetup( + it('should return false if the list has dt and dd', () => { + const checkArgs = checkSetup( '
    An item
    A list
    ' ); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return false if the list has dt, dd and a comment', function () { - var checkArgs = checkSetup( + it('should return false if the list has dt, dd and a comment', () => { + const checkArgs = checkSetup( '
    An item
    A list
    ' ); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return true if the list has a dt and dd with other content', function () { - var checkArgs = checkSetup( + it('should return true if the list has a dt and dd with other content', () => { + const checkArgs = checkSetup( '
    Item one
    Description

    Not a list

    ' ); - - assert.isTrue( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isTrue(checkEvaluate.apply(checkContext, checkArgs)); assert.deepEqual(checkContext._relatedNodes, [fixture.querySelector('p')]); + assert.deepEqual(checkContext._data, { values: 'p' }); }); - it('should return true if the list has a textNode as a child', function () { - var checkArgs = checkSetup( + it('should return true if the list has a textNode as a child', () => { + const checkArgs = checkSetup( '
    hi
    hello
    hi
    ' ); - assert.isTrue( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isTrue(checkEvaluate.apply(checkContext, checkArgs)); assert.deepEqual(checkContext._relatedNodes, []); + assert.deepEqual(checkContext._data, { values: '#text' }); }); - it('should return false if is used along side dt', function () { - var checkArgs = checkSetup( + it('should return false if is used along side dt', () => { + const checkArgs = checkSetup( '
    A list
    ' ); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return false if is used along side dt', function () { - var checkArgs = checkSetup( + it('should return false if is used along side dt', () => { + const checkArgs = checkSetup( '
    A list
    ' ); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return false if
    A list
    ' ); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return false if
    A list
    ' ); - - assert.isFalse( - axe.testUtils - .getCheckEvaluate('only-dlitems') - .apply(checkContext, checkArgs) - ); + assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); }); - it('should return false if