-
Notifications
You must be signed in to change notification settings - Fork 791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(rule): update for lists, list-items to handle role changes. #949
Changes from 7 commits
5f21fe8
0bf9863
ce2b6b5
fbbb178
e3c2244
372c82c
0f52a61
6821aab
6edf44d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,26 @@ | ||
var parent = axe.commons.dom.getComposedParent(node); | ||
return parent.nodeName.toUpperCase() === 'DL'; | ||
const parent = axe.commons.dom.getComposedParent(node); | ||
if (!parent) { | ||
return false; | ||
} | ||
|
||
const parentTagName = parent.nodeName.toUpperCase(); | ||
if (parentTagName !== 'DL') { | ||
return false; | ||
} | ||
|
||
const parentRole = (parent.getAttribute('role') || '').toLowerCase(); | ||
|
||
if (!parentRole) { | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
return true; | ||
} | ||
|
||
if (!axe.commons.aria.isValidRole(parentRole)) { | ||
return false; | ||
} | ||
|
||
if (parentRole) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need this if. |
||
if (parentRole === 'list') { | ||
return true; | ||
} | ||
return false; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. filter roles to only accept valid ARIA roles |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could just inline There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. leaving as is, abstraction reads better here, as otherwise it feels repeated. |
||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,33 +1,40 @@ | ||
var bad = [], | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
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 out = virtualNode.children.reduce((out, { actualNode }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe use destructive assignment so that you're never using this meaningless variable name? |
||
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 (out.badNodes.length) { | ||
this.relatedNodes(out.badNodes); | ||
} | ||
|
||
var retVal = !!bad.length || hasNonEmptyTextNode; | ||
return retVal; | ||
return !!out.badNodes.length || out.hasNonEmptyTextNode; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,31 +1,84 @@ | ||
var bad = [], | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 <dl>', function() { | ||
it('should pass if the dlitem has a parent <dl>', function () { | ||
var checkArgs = checkSetup('<dl><dt id="target">My list item</dl>'); | ||
|
||
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('<video><dt id="target">My list item</video>'); | ||
|
||
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs)); | ||
}); | ||
|
||
(shadowSupport.v1 ? it : xit)( | ||
'should return true in a shadow DOM pass', | ||
function() { | ||
var node = document.createElement('div'); | ||
node.innerHTML = '<dt>My list item </dt>'; | ||
var shadow = node.attachShadow({ mode: 'open' }); | ||
shadow.innerHTML = '<dl><slot></slot></dl>'; | ||
|
||
var checkArgs = checkSetup(node, 'dt'); | ||
assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs)); | ||
} | ||
); | ||
|
||
(shadowSupport.v1 ? it : xit)( | ||
'should return false in a shadow DOM fail', | ||
function() { | ||
var node = document.createElement('div'); | ||
node.innerHTML = '<dt>My list item </dt>'; | ||
var shadow = node.attachShadow({ mode: 'open' }); | ||
shadow.innerHTML = '<div><slot></slot></div>'; | ||
|
||
var checkArgs = checkSetup(node, 'dt'); | ||
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs)); | ||
} | ||
); | ||
it('should pass if the dt element has a parent <dl> with role="list"', function () { | ||
var checkArgs = checkSetup('<dl role="list"><dt id="target">My list item</dl>'); | ||
assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs)); | ||
}) | ||
|
||
it('should fail if the dt element has a parent <dl> with role="presentation"', function () { | ||
var checkArgs = checkSetup('<dl role="presentation"><dt id="target">My list item</dl>'); | ||
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs)); | ||
}); | ||
|
||
it('should fail if the dt element has a parent <dl> with a changed role', function(){ | ||
var checkArgs = checkSetup('<dl role="menubar"><dt id="target">My list item</dl>'); | ||
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs)); | ||
}); | ||
|
||
(shadowSupport.v1 ? it : xit)('should return true in a shadow DOM pass', function () { | ||
var node = document.createElement('div'); | ||
node.innerHTML = '<dt>My list item </dt>'; | ||
var shadow = node.attachShadow({ mode: 'open' }); | ||
shadow.innerHTML = '<dl><slot></slot></dl>'; | ||
|
||
var checkArgs = checkSetup(node, 'dt'); | ||
assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs)); | ||
}); | ||
|
||
(shadowSupport.v1 ? it : xit)('should return false in a shadow DOM fail', function () { | ||
var node = document.createElement('div'); | ||
node.innerHTML = '<dt>My list item </dt>'; | ||
var shadow = node.attachShadow({ mode: 'open' }); | ||
shadow.innerHTML = '<div><slot></slot></div>'; | ||
|
||
var checkArgs = checkSetup(node, 'dt'); | ||
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs)); | ||
}); | ||
}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the cases where there isn't a composed parent?