Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Allow only-dlitem / only-listitem to have any hidden content #1098

Merged
merged 2 commits into from
Aug 27, 2018
Merged

fix: Allow only-dlitem / only-listitem to have any hidden content #1098

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82271da
fix: Allow only-dlitem / only-listitem to have any hidden content
WilcoFiers Aug 24, 2018
b6def5f
Merge branch 'develop' into hidden-listitems
dylanb Aug 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 4 additions & 16 deletions lib/checks/lists/only-dlitems.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,5 @@
const ALLOWED_TAGS = [
'STYLE',
'META',
'LINK',
'MAP',
'AREA',
'SCRIPT',
'DATALIST',
'TEMPLATE'
];

const { dom, aria } = axe.commons;
const ALLOWED_ROLES = ['definition', 'term', 'list'];
const { getRole } = axe.commons.aria;

let base = {
badNodes: [],
hasNonEmptyTextNode: false
Expand All @@ -21,7 +9,7 @@ const content = virtualNode.children.reduce((content, child) => {
const { actualNode } = child;
if (
actualNode.nodeName.toUpperCase() === 'DIV' &&
getRole(actualNode) === null
aria.getRole(actualNode) === null
) {
return content.concat(child.children);
}
Expand All @@ -32,8 +20,8 @@ const result = content.reduce((out, childNode) => {
const { actualNode } = childNode;
const tagName = actualNode.nodeName.toUpperCase();

if (actualNode.nodeType === 1 && !ALLOWED_TAGS.includes(tagName)) {
const explicitRole = getRole(actualNode, { noImplicit: true });
if (actualNode.nodeType === 1 && dom.isVisible(actualNode, true, false)) {
const explicitRole = aria.getRole(actualNode, { noImplicit: true });

if ((tagName !== 'DT' && tagName !== 'DD') || explicitRole) {
if (!ALLOWED_ROLES.includes(explicitRole)) {
Expand Down
44 changes: 14 additions & 30 deletions lib/checks/lists/only-listitems.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,4 @@
const ALLOWED_TAGS = [
'STYLE',
'META',
'LINK',
'MAP',
'AREA',
'SCRIPT',
'DATALIST',
'TEMPLATE'
];

const { dom } = axe.commons;
const getIsListItemRole = (role, tagName) => {
return role === 'listitem' || (tagName === 'LI' && !role);
};
Expand All @@ -28,30 +18,24 @@ let base = {
let out = virtualNode.children.reduce((out, { actualNode }) => {
/*eslint
max-statements: ["error", 20]
complexity: ["error", 11]
complexity: ["error", 12]
*/
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);
if (actualNode.nodeType === 1 && dom.isVisible(actualNode, true, false)) {
const role = (actualNode.getAttribute('role') || '').toLowerCase();
const isListItemRole = getIsListItemRole(role, tagName);

out.hasListItem = getHasListItem(
out.hasListItem,
tagName,
isListItemRole
);
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 (isListItemRole) {
out.isEmpty = false;
}
if (tagName === 'LI' && !isListItemRole) {
out.liItemsWithRole++;
}
if (tagName !== 'LI' && !isListItemRole) {
out.badNodes.push(actualNode);
}
}
if (actualNode.nodeType === 3) {
Expand Down
36 changes: 36 additions & 0 deletions test/checks/lists/only-dlitems.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,42 @@ describe('only-dlitems', function() {
);
});

it('returns false if there are display:none elements that normally would not be allowed', function() {
var checkArgs = checkSetup(
'<dl id="target"> <dt>An item</dt> <dd>A list</dd> <h1 style="display:none">heading</h1> </dl>'
);
assert.isFalse(
checks['only-dlitems'].evaluate.apply(checkContext, checkArgs)
);
});

it('returns false if there are visibility:hidden elements that normally would not be allowed', function() {
var checkArgs = checkSetup(
'<dl id="target"> <dt>An item</dt> <dd>A list</dd> <h1 style="visibility:hidden">heading</h1> </dl>'
);
assert.isFalse(
checks['only-dlitems'].evaluate.apply(checkContext, checkArgs)
);
});

it('returns false if there are aria-hidden=true elements that normally would not be allowed', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, I don't agree with this case

var checkArgs = checkSetup(
'<dl id="target"> <dt>An item</dt> <dd>A list</dd> <h1 aria-hidden="true">heading</h1> </dl>'
);
assert.isFalse(
checks['only-dlitems'].evaluate.apply(checkContext, checkArgs)
);
});

it('returns true if there are aria-hidden=false elements that normally would not be allowed', function() {
var checkArgs = checkSetup(
'<dl id="target"> <dt>An item</dt> <dd>A list</dd> <h1 aria-hidden="false">heading</h1> </dl>'
);
assert.isTrue(
checks['only-dlitems'].evaluate.apply(checkContext, checkArgs)
);
});

(shadowSupport.v1 ? it : xit)(
'should return false in a shadow DOM pass',
function() {
Expand Down
36 changes: 36 additions & 0 deletions test/checks/lists/only-listitems.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,42 @@ describe('only-listitems', function() {
);
});

it('returns false if there are display:none elements that normally would not be allowed', function() {
var checkArgs = checkSetup(
'<ul id="target"> <li>An item</li> <h1 style="display:none">heading</h1> </ul>'
);
assert.isFalse(
checks['only-listitems'].evaluate.apply(checkContext, checkArgs)
);
});

it('returns false if there are visibility:hidden elements that normally would not be allowed', function() {
var checkArgs = checkSetup(
'<ul id="target"> <li>An item</li> <h1 style="visibility:hidden">heading</h1> </ul>'
);
assert.isFalse(
checks['only-listitems'].evaluate.apply(checkContext, checkArgs)
);
});

it('returns false if there are aria-hidden=true elements that normally would not be allowed', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this

var checkArgs = checkSetup(
'<ul id="target"> <li>An item</li> <h1 aria-hidden="true">heading</h1> </ul>'
);
assert.isFalse(
checks['only-listitems'].evaluate.apply(checkContext, checkArgs)
);
});

it('returns true if there are aria-hidden=false elements that normally would not be allowed', function() {
var checkArgs = checkSetup(
'<ul id="target"> <li>An item</li> <h1 aria-hidden="false">heading</h1> </ul>'
);
assert.isTrue(
checks['only-listitems'].evaluate.apply(checkContext, checkArgs)
);
});

(shadowSupport.v1 ? it : xit)(
'should return false in a shadow DOM pass',
function() {
Expand Down