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 div groups for dlitem rule #1284

Merged
merged 3 commits into from
Jan 7, 2019
Merged

fix: Allow div groups for dlitem rule #1284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0acffe
fix: Allow div groups for dlitem rule
WilcoFiers Dec 19, 2018
f6f1fc6
chore: Update test descriptions
WilcoFiers Dec 30, 2018
ba76e45
Merge branch 'develop' into dd-fix
WilcoFiers Jan 7, 2019
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
21 changes: 13 additions & 8 deletions lib/checks/lists/dlitem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
const parent = axe.commons.dom.getComposedParent(node);
const parentTagName = parent.nodeName.toUpperCase();
let parent = axe.commons.dom.getComposedParent(node);
let parentTagName = parent.nodeName.toUpperCase();
let parentRole = axe.commons.aria.getRole(parent, { noImplicit: true });

if (
parentTagName === 'DIV' &&
['presentation', 'none', null].includes(parentRole)
) {
parent = axe.commons.dom.getComposedParent(parent);
parentTagName = parent.nodeName.toUpperCase();
parentRole = axe.commons.aria.getRole(parent, { noImplicit: true });
}

// Unlike with UL|OL+LI, DT|DD must be in a DL
if (parentTagName !== 'DL') {
return false;
}

const parentRole = (parent.getAttribute('role') || '').toLowerCase();
if (!parentRole || !axe.commons.aria.isValidRole(parentRole)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isValidRole is done by aria.getRole(), in case you're wondering.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised it works with the noImplicit: true, can you explain that bit? This applies to native elements as well as bolt-on roles, right? The tests seem to pass, so 🤷‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The requirements here are: It needs to be in a dl, and if that dl has a role attribute, it can only be a list. So what's happening is first I check the parentTagName, and then I check if the explicit role (which is what we get from noImplicit: true is null or 'list'.

return true;
}

if (parentRole === 'list') {
if (!parentRole || parentRole === 'list') {
return true;
}

Expand Down
87 changes: 78 additions & 9 deletions test/checks/lists/dlitem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,60 +10,116 @@ describe('dlitem', function() {
});

it('should pass if the dlitem has a parent <dl>', function() {
var checkArgs = checkSetup('<dl><dt id="target">My list item</dl>');
var checkArgs = checkSetup('<dl><dt id="target">My list item</dt></dl>');

assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs));
});

it('should fail if the dt element has an incorrect parent', function() {
var checkArgs = checkSetup('<video><dt id="target">My list item</video>');
var checkArgs = checkSetup(
'<video><dt id="target">My list item</dt></video>'
);

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>'
'<dl role="list"><dt id="target">My list item</dt></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>'
'<dl role="presentation"><dt id="target">My list item</dt></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>'
'<dl role="menubar"><dt id="target">My list item<</dt>/dl>'
);
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs));
});

it('should pass if the dt element has a parent <dl> with an abstract role', function() {
var checkArgs = checkSetup(
'<dl role="section"><dt id="target">My list item</dl>'
'<dl role="section"><dt id="target">My list item</dt></dl>'
);
assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs));
});

it('should pass if the dt element has a parent <dl> with an invalid role', function() {
var checkArgs = checkSetup(
'<dl role="invalid-role"><dt id="target">My list item</dl>'
'<dl role="invalid-role"><dt id="target">My list item</dt></dl>'
);
assert.isTrue(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>'
'<dl role="menubar"><dt id="target">My list item</dt></dl>'
);
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs));
});

(shadowSupport.v1 ? it : xit)(
it('returns true if the dd/dt is in a div with a dl as grandparent', function() {
const nodeNames = ['dd', 'dt'];
nodeNames.forEach(function(nodeName) {
var checkArgs = checkSetup(
'<dl><div><' +
nodeName +
' id="target">My list item</' +
nodeName +
'></div></dl>'
);
assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs));
});
});

it('returns false if the dd/dt is in a div with a role with a dl as grandparent with a list role', function() {
const nodeNames = ['dd', 'dt'];
nodeNames.forEach(function(nodeName) {
var checkArgs = checkSetup(
'<dl><div role="list"><' +
nodeName +
' id="target">My list item</' +
nodeName +
'></div></dl>'
);
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs));
});
});

it('returns false if the dd/dt is in a div[role=presentation] with a dl as grandparent', function() {
const nodeNames = ['dd', 'dt'];
nodeNames.forEach(function(nodeName) {
var checkArgs = checkSetup(
'<dl><div role="presentation"><' +
nodeName +
' id="target">My list item</' +
nodeName +
'></div></dl>'
);
assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs));
});
});

it('returns false if the dd/dt is in a div[role=none] with a dl as grandparent', function() {
const nodeNames = ['dd', 'dt'];
nodeNames.forEach(function(nodeName) {
var checkArgs = checkSetup(
'<dl><div role="none"><' +
nodeName +
' id="target">My list item</' +
nodeName +
'></div></dl>'
);
assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs));
});
})(shadowSupport.v1 ? it : xit)(
'should return true in a shadow DOM pass',
function() {
var node = document.createElement('div');
Expand All @@ -88,4 +144,17 @@ describe('dlitem', function() {
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs));
}
);

(shadowSupport.v1 ? it : xit)(
'should return true when the item is grouped in dl > div in a shadow DOM',
function() {
var node = document.createElement('div');
node.innerHTML = '<dt>My list item </dt>';
var shadow = node.attachShadow({ mode: 'open' });
shadow.innerHTML = '<dl><div><slot></slot></div></dl>';

var checkArgs = checkSetup(node, 'dt');
assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs));
}
);
});
10 changes: 9 additions & 1 deletion test/integration/rules/dlitem/dlitem.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,16 @@
<dl>
<dt id="contained">Does belong to a list.</dt>
<dd id="alsocontained">Also belongs to a list.</dd>
<div>
<dt id="grouped">Grouped in a div.</dt>
<dd id="alsogrouped">Also grouped in a div.</dd>
</div>
<div role="main">
<dt id="incorrectlygrouped">Grouped in a div with role.</dt>
<dd id="alsoincorrectlygrouped">Also grouped in a div with role.</dd>
</div>
</dl>
<dl role="menubar">
<dt id="uncontainedbyrole">Not part of a list.</dt>
<dd id="alsouncontainedbyrole">Also not part of a list.</dd>
</dl>
</dl>
6 changes: 4 additions & 2 deletions test/integration/rules/dlitem/dlitem.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
["#uncontained"],
["#alsouncontained"],
["#uncontainedbyrole"],
["#alsouncontainedbyrole"]
["#alsouncontainedbyrole"],
["#incorrectlygrouped"],
["#alsoincorrectlygrouped"]
],
"passes": [["#contained"], ["#alsocontained"]]
"passes": [["#contained"], ["#alsocontained"], ["#grouped"], ["#alsogrouped"]]
}