fix: check for role changes in lists and list items #518

AlmeroSteyn · 2017-09-10T14:51:35Z

Change function of dlitem, listitem, only-dlitems and only-listitems checks to take into account role changes to either parent elements or child list item elements. Previously changing the role of either elements did not trigger the correct symantic markup errors.

Closes #463

The following additions have been made to the current checks:

Changing the role of the parent <ul> or <ol> will have the child <li> tags report that they are not part of a list.
Changing the role of the parent <dl> will have the child <dd> or <dt> tags report that they are not part of a list.
Changing the role of a <li> tag will mean that it will get ignored by the check if the list contains any other valid content.
Filling a list with ONLY <li> tags with a different role will trigger an invalid content error.

change function of dlitem, listitem, only-dlitems and only-listitems checks to take into account role changes to either parent elements or child list item elements. Previously changing the role of either elements did not trigger the correct symantic markup errors. closes dequelabs#463

CLAassistant · 2017-09-10T14:51:42Z

All committers have signed the CLA.

WilcoFiers · 2017-09-16T11:34:38Z

lib/checks/lists/dlitem.js

@@ -1,2 +1,3 @@
 var parent = axe.commons.dom.getComposedParent(node);
-return parent.nodeName.toUpperCase() === 'DL';
+return parent.nodeName.toUpperCase() === 'DL' && !parent.getAttribute('role');


Should this allow <dl role="list">?

Which would say it's okay to overload a <dl> with an ARIA role. Is that something we want to encourage? Wouldn't that also depend on the child items having role="listitem"?

WilcoFiers · 2017-09-16T11:36:41Z

lib/checks/lists/listitem.js

+var isListRole = parentRole === 'list';
+
+return (


This doesn't follow our spacing convention. It's also a little hard to read. Can you reformat?

@WilcoFiers which spacing convention are you referring to?

WilcoFiers · 2017-09-16T11:41:29Z

lib/checks/lists/only-dlitems.js

-		bad.push(actualNode);
+
+	if (actualNode.nodeType === 1 && permitted.indexOf(nodeName) === -1) {
+		var role = (actualNode.getAttribute('role') || '').toLowerCase();


This should allow role=definition and role=term, possibly also role=list?

WilcoFiers · 2017-10-18T10:11:46Z

lib/checks/lists/only-listitems.js

+	if (actualNode.nodeType === 1 && permitted.indexOf(nodeName) === -1) {
+		var role = (actualNode.getAttribute('role') || '').toLowerCase();
+		isListItemRole = role === 'listitem' || (nodeName === 'LI' && !role);
+		hasListItem = hasListItem || (nodeName === 'LI' && isListItemRole) || isListItemRole;


This needs either a comment, or it needs to be cleared up a bit, because it's not immediately obvious what the if statements in this block do.

WilcoFiers · 2017-10-18T10:13:07Z

test/checks/lists/dlitem.js

@@ -21,6 +21,12 @@ describe('dlitem', function () {
 		assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs));
 	});

+	it('should fail if the dlitem has a parent <dl> with a changed role', function(){


Should be "should fail if the dt element has a parent
with a changed role"

WilcoFiers · 2017-10-18T10:18:49Z

test/checks/lists/only-dlitems.js

 	it('should return false if <meta> is used along side dt', function () {
 		var checkArgs = checkSetup('<dl id="target"><meta name="description" content=""><dt>A list</dt></dl>');

 		assert.isFalse(checks['only-dlitems'].evaluate.apply(checkContext, checkArgs));
 	});

+	it('should return true if <meta> is used along side dt with its role changed', function () {


This test seems unnecessary, and so do the new tests below this one.

WilcoFiers · 2017-10-18T10:23:19Z

test/checks/lists/only-listitems.js

+	});
+
+	it('should return false if the list has at least one li while others have their roles changed', function () {
+		var checkArgs = checkSetup('<ol id="target"><li >A list item</li><li role="menuitem">Not a list item</li></ol>');


@dylanb I remember you suggesting this should be a violation? My initial thought about these was that there might be roles we should allow. separator for instance would be one I think is very reasonable. Menuitem itself would fail here because it isn't owned by a menu, so that's its own error.

this example does not represent a violation IMO

WilcoFiers · 2017-10-18T10:24:20Z

test/checks/lists/only-listitems.js

 	it('should return false if <meta> is used along side li', function () {
 		var checkArgs = checkSetup('<ol id="target"><meta name="description" content=""><li>A list</li></ol>');

 		assert.isFalse(checks['only-listitems'].evaluate.apply(checkContext, checkArgs));
 	});

+	it('should return true if <meta> is used along side only li items with their roles changed', function () {
+		var checkArgs = checkSetup('<ol id="target"><meta name="description" content=""><li role="menuitem">Not a list item</li></ol>');


This one doesn't seem useful, and neither do tests added below this one.

WilcoFiers · 2017-10-18T10:26:02Z

test/checks/lists/only-listitems.js

@@ -60,43 +66,97 @@ describe('only-listitems', function () {
 		assert.isFalse(checks['only-listitems'].evaluate.apply(checkContext, checkArgs));
 	});

+	it('should return false if the list has only an element with role listitem', function () {
+		var checkArgs = checkSetup('<ol id="target"><div role="listitem">A list</div></ol>');


We should add tests for multiple listitem elms, and for mixing li and listitem.

marcysutton · 2017-12-08T21:22:47Z

Hi @AlmeroSteyn, just checking in to see if you plan to address Wilco's feedback on this PR? We'd love to get this updated and merged.

WilcoFiers · 2017-12-09T12:22:31Z

@marcysutton I heard from Almero that he wasn't. We need to sort this one out ourselves.

marcysutton · 2017-12-12T00:26:35Z

lib/checks/lists/only-listitems.js

+	if (actualNode.nodeType === 1 && permitted.indexOf(nodeName) === -1) {
+		var role = (actualNode.getAttribute('role') || '').toLowerCase();
+		isListItemRole = role === 'listitem' || (nodeName === 'LI' && !role);
+		hasListItem = hasListItem || (nodeName === 'LI' && isListItemRole) || isListItemRole;


It also seems a little redundant:
hasListItem = hasListItem || (nodeName === 'LI' && isListItemRole) || isListItemRole;
The code checks the nodeName for 'LI' and a role more than once, and then checks against itself again a few times. I wonder if we can simplify.

jeeyyy · 2018-06-08T17:21:14Z

@AlmeroSteyn - cheers for the work.
I have circled up all comments on here and re-worked it in to this PR - #949

Hence closing it.

@AlmeroSteyn

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: #518 @AlmeroSteyn - thanks for the initial work). Closes issue: - #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

WilcoFiers reviewed Oct 18, 2017

View reviewed changes

WilcoFiers added the help wanted We welcome PRs or discussions for issues marked as help wanted label Dec 9, 2017

marcysutton reviewed Dec 12, 2017

View reviewed changes

jeeyyy self-assigned this Apr 18, 2018

jeeyyy mentioned this pull request Jun 8, 2018

fix(rule): update for lists, list-items to handle role changes. #949

Merged

4 tasks

jeeyyy closed this Jun 8, 2018

mrtnvh pushed a commit to mrtnvh/axe-core that referenced this pull request Nov 24, 2023

chore: update selenium version (dequelabs#518)

689fae1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check for role changes in lists and list items #518

fix: check for role changes in lists and list items #518

AlmeroSteyn commented Sep 10, 2017 •

edited

Loading

CLAassistant commented Sep 10, 2017 •

edited

Loading

WilcoFiers Sep 16, 2017

marcysutton Dec 11, 2017

jeeyyy Jun 8, 2018

WilcoFiers Sep 16, 2017

marcysutton Dec 11, 2017

jeeyyy Jun 8, 2018

WilcoFiers Sep 16, 2017

WilcoFiers Oct 18, 2017

WilcoFiers Oct 18, 2017

WilcoFiers Oct 18, 2017

WilcoFiers Oct 18, 2017

dylanb Jun 11, 2018

WilcoFiers Oct 18, 2017

WilcoFiers Oct 18, 2017

marcysutton commented Dec 8, 2017

WilcoFiers commented Dec 9, 2017

marcysutton Dec 12, 2017

jeeyyy commented Jun 8, 2018

fix: check for role changes in lists and list items #518

fix: check for role changes in lists and list items #518

Conversation

AlmeroSteyn commented Sep 10, 2017 • edited Loading

CLAassistant commented Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Dec 8, 2017

WilcoFiers commented Dec 9, 2017

Choose a reason for hiding this comment

jeeyyy commented Jun 8, 2018

AlmeroSteyn commented Sep 10, 2017 •

edited

Loading

CLAassistant commented Sep 10, 2017 •

edited

Loading