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

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

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:

well formed lists and ARIA #463

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Has documentation updated, a DU ticket, or requires no documentation change
Includes new tests, or was unnecessary
Code is reviewed for security by: @WilcoFiers

dylanb

Please address the comments

dylanb · 2018-06-08T18:18:55Z

lib/checks/lists/dlitem.js

+		if (ALLOWED_ROLES.includes(parentRole)) {
+			return true;
+		}
+		return false;


Should check to see whether the other role is a valid ARIA role or otherwise it will not actually override the semantic

What Dylan said.

Note to self: role='fake-role' should fallback to semantic.

dylanb · 2018-06-08T18:19:31Z

lib/checks/lists/listitem.js

+
+const parentTagName = parent.nodeName.toLowerCase();
+const parentRole = (parent.getAttribute('role') || '').toLowerCase();


filter roles to only accept valid ARIA roles

lib/checks/lists/only-dlitems.js

@@ -1,19 +1,41 @@
-var bad = [],


lib/checks/lists/only-listitems.js

@@ -1,18 +1,84 @@
-var bad = [],


dylanb · 2018-06-26T03:24:52Z

test/checks/lists/only-listitems.js

 	it('should return false if the list has whitespace', function () {
 		var checkArgs = checkSetup('<ol id="target"><li>Item</li>    </ol>');

 		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>');


@JKODU why has this not yet been addressed?

@dylanb I think that's on me - I replied to it saying that Axe shouldn't be a validator. As long as it works in AT, it shouldn't matter if it's valid HTML or not.

dylanb · 2018-06-26T03:25:15Z

test/checks/lists/only-listitems.js

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


See my previous comment.

dylanb · 2018-06-26T03:25:44Z

test/checks/lists/only-listitems.js

+	});
+
+	it('should return true if <link> is used along side only li items with their roles changed', function () {
+		var checkArgs = checkSetup('<ol id="target"><link rel="stylesheet" href="theme.css"><li role="menuitem">Not a list item</li></ol>');


What is the rationale for this?

@dylanb This isn't new - we've allowed pretty much any non-content elements inside of lists from way back. If you think we should change that I propose we do it in a separate PR.

jeeyyy · 2018-06-08T20:52:55Z

@dylanb
Cheers for the above, will address.

Could you expand on this comment as well (from the old PR)?
#518 (comment)

Updated.

test/checks/lists/only-listitems.js

 	it('should return false if the list has whitespace', function () {
 		var checkArgs = checkSetup('<ol id="target"><li>Item</li>    </ol>');

 		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>');


WilcoFiers · 2018-06-26T11:39:21Z

lib/checks/lists/dlitem.js

+	return false;
+}
+
+const ALLOWED_ROLES = ['list'];


That seems needlessly complicated.

WilcoFiers · 2018-06-26T11:41:13Z

lib/checks/lists/dlitem.js

+		if (ALLOWED_ROLES.includes(parentRole)) {
+			return true;
+		}
+		return false;


What Dylan said.

WilcoFiers · 2018-06-26T11:41:41Z

lib/checks/lists/listitem.js

+
+const ALLOWED_TAGS = ['ul', 'ol'];
+const ALLOWED_ROLES = ['list'];


This just complicates things.

WilcoFiers · 2018-06-26T11:43:55Z

lib/checks/lists/listitem.js

+}
+
+const ALLOWED_TAGS = ['ul', 'ol'];


Use upper case node names.

WilcoFiers · 2018-06-26T11:59:29Z

lib/checks/lists/only-listitems.js

+		*/
+		const tagName = actualNode.nodeName.toLowerCase();
+
+		switch (actualNode.nodeType) {


Should just be an if statement.

WilcoFiers · 2018-06-26T12:01:36Z

lib/checks/lists/only-listitems.js

+const getIsListItemRole = (r, t) => {
+	let out = false;
+	out = (r === 'listitem' || (t === 'li' && !r));
+	return out;


WilcoFiers · 2018-06-26T12:04:37Z

lib/checks/lists/only-listitems.js

+
+const getIsListItemRole = (r, t) => {
+	let out = false;
+	out = (r === 'listitem' || (t === 'li' && !r));


!r should test that this is a valid non-abstract role, not just that it's null. This entire function could do with some cleanup. Don't use single letter variables, and just return on this line rather than assign it to a var and immediately return that.

WilcoFiers · 2018-06-26T12:06:19Z

lib/checks/lists/only-listitems.js

+
+		switch (actualNode.nodeType) {
+		case 1:
+			if (!ALLOWED_TAGS.includes(tagName)) {


The logic of this block is pretty difficult to follow, far more than it needs to be.

WilcoFiers · 2018-06-26T12:07:15Z

lib/checks/lists/only-listitems.js

 }

-return !!bad.length || hasNonEmptyTextNode;
+return (


It's really difficult to see if this code actually does what it should. Please break this up.

Please review again.

jeeyyy · 2018-07-04T12:56:14Z

@dylanb @WilcoFiers - review again.

jeeyyy · 2018-07-16T13:55:09Z

@WilcoFiers - based on our chat, this can be approved I believe.

jeeyyy · 2018-07-16T23:22:14Z

@dylanb @marcysutton - review plz

marcysutton · 2018-07-17T16:24:24Z

lib/checks/lists/dlitem.js

-return parent.nodeName.toUpperCase() === 'DL';
+const parent = axe.commons.dom.getComposedParent(node);
+if (!parent) {
+	return false;


What are the cases where there isn't a composed parent?

lib/checks/lists/dlitem.js

+
+const parentRole = (parent.getAttribute('role') || '').toLowerCase();
+
+if (!parentRole) {


WilcoFiers · 2018-07-16T12:41:13Z

lib/checks/lists/dlitem.js

+	return false;
+}
+
+if (parentRole) {


You don't need this if.

WilcoFiers · 2018-07-16T12:50:02Z

lib/checks/lists/listitem.js

-	['UL', 'OL'].includes(parent.nodeName.toUpperCase()) ||
-	(parent.getAttribute('role') || '').toLowerCase() === 'list'
+	(ALLOWED_TAGS.includes(parentTagName) && (!parentRole || isListRole)) ||
+	isListRole


You could just inline parentRole === 'list'.

leaving as is, abstraction reads better here, as otherwise it feels repeated.

WilcoFiers · 2018-07-16T12:51:33Z

lib/checks/lists/only-dlitems.js

+	hasNonEmptyTextNode: false
+};
+
+const out = virtualNode.children.reduce((out, { actualNode }) => {


Maybe use destructive assignment so that you're never using this meaningless variable name?

jeeyyy added 2 commits June 8, 2018 18:10

fix: list and list items fixes for role changes.

5f21fe8

fix: integration test for listitem

0bf9863

This was referenced Jun 8, 2018

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

Closed

well formed lists and ARIA #463

Closed

jeeyyy requested review from WilcoFiers, dylanb, stephenmathieson, marcysutton, akornmeier and isner June 8, 2018 17:23

jeeyyy self-assigned this Jun 8, 2018

jeeyyy added the rules Issue or false result from an axe-core rule label Jun 8, 2018

dylanb previously requested changes Jun 8, 2018

View reviewed changes

jeeyyy changed the title ~~rule: update for lists, list-items to handle role changes.~~ fix(rule): update for lists, list-items to handle role changes. Jun 11, 2018

jeeyyy changed the title ~~fix(rule): update for lists, list-items to handle role changes.~~ [WIP] fix(rule): update for lists, list-items to handle role changes. Jun 20, 2018

jeeyyy changed the title ~~[WIP] fix(rule): update for lists, list-items to handle role changes.~~ fix(rule): update for lists, list-items to handle role changes. Jun 26, 2018

WilcoFiers previously requested changes Jun 26, 2018

View reviewed changes

jeeyyy changed the title ~~fix(rule): update for lists, list-items to handle role changes.~~ [WIP] fix(rule): update for lists, list-items to handle role changes. Jun 27, 2018

jeeyyy added 2 commits July 2, 2018 15:30

fix: merge from develop

ce2b6b5

fix: tackle comments on PR

fbbb178

jeeyyy changed the title ~~[WIP] fix(rule): update for lists, list-items to handle role changes.~~ fix(rule): update for lists, list-items to handle role changes. Jul 3, 2018

jeeyyy added 2 commits July 3, 2018 21:06

style: update eslint max-statements allowed

e3c2244

Merge branch 'develop' into rule-update-lists

372c82c

Merge branch 'develop' into rule-update-lists

0f52a61

marcysutton reviewed Jul 17, 2018

View reviewed changes

WilcoFiers reviewed Jul 18, 2018

View reviewed changes

jeeyyy added 2 commits July 18, 2018 09:55

refactor: changes based on code review

6821aab

Merge branch 'develop' into rule-update-lists

6edf44d

WilcoFiers approved these changes Jul 18, 2018

View reviewed changes

WilcoFiers merged commit 3a8729b into develop Jul 18, 2018

WilcoFiers deleted the rule-update-lists branch July 18, 2018 09:04


		const parentTagName = parent.nodeName.toLowerCase();
		const parentRole = (parent.getAttribute('role') \|\| '').toLowerCase();


		const ALLOWED_TAGS = ['ul', 'ol'];
		const ALLOWED_ROLES = ['list'];


		const parentRole = (parent.getAttribute('role') \|\| '').toLowerCase();

		if (!parentRole) {

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

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

Conversation

jeeyyy commented Jun 8, 2018 • edited by WilcoFiers Loading

Reviewer checks

dylanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as off-topic.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy commented Jun 8, 2018

This comment was marked as resolved.

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

jeeyyy commented Jul 4, 2018

jeeyyy commented Jul 16, 2018

jeeyyy commented Jul 16, 2018

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

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

jeeyyy commented Jun 8, 2018 •

edited by WilcoFiers

Loading

jeeyyy Jun 26, 2018 •

edited

Loading