From e3b1e1d8d1aa87c9d3e9a60c742a540deacce948 Mon Sep 17 00:00:00 2001
From: Wilco Fiers <wilco.fiers@deque.com>
Date: Fri, 20 Jul 2018 19:52:45 +0200
Subject: [PATCH] fix: Ignore abstracts in determining element roles

---
 doc/rule-descriptions.md             | 14 ++---
 lib/checks/aria/invalidrole.js       |  4 +-
 lib/checks/lists/dlitem.js           |  8 +--
 lib/checks/lists/listitem.js         | 18 +++----
 lib/commons/aria/roles.js            | 12 ++---
 test/checks/lists/dlitem.js          | 81 +++++++++++++++++++---------
 test/checks/lists/listitem.js        | 67 +++++++++++++++--------
 test/commons/aria/roles.js           | 10 ++++
 test/commons/table/is-data-cell.js   | 13 +++++
 test/rule-matches/heading-matches.js |  6 +++
 10 files changed, 157 insertions(+), 76 deletions(-)

diff --git a/doc/rule-descriptions.md b/doc/rule-descriptions.md
index 4ecd46a484..5331228f9d 100644
--- a/doc/rule-descriptions.md
+++ b/doc/rule-descriptions.md
@@ -36,12 +36,12 @@
 | input-image-alt | Ensures &lt;input type=&quot;image&quot;&gt; elements have alternate text | Critical | cat.text-alternatives, wcag2a, wcag111, section508, section508.22.a | true |
 | label-title-only | Ensures that every form element is not solely labeled using the title or aria-describedby attributes | Serious | cat.forms, best-practice | true |
 | label | Ensures every form element has a label | Minor, Serious, Critical | cat.forms, wcag2a, wcag332, wcag131, section508, section508.22.n | true |
-| landmark-banner-is-top-level | The banner landmark should not be contained in another landmark | Moderate | cat.semantics, best-practice | true |
-| landmark-contentinfo-is-top-level | The contentinfo landmark should not be contained in another landmark | Moderate | cat.semantics, best-practice | true |
-| landmark-main-is-top-level | The main landmark should not be contained in another landmark | Moderate | cat.semantics, best-practice | true |
-| landmark-no-duplicate-banner | Ensures the document has no more than one banner landmark | Moderate | cat.semantics, best-practice | true |
-| landmark-no-duplicate-contentinfo | Ensures the document has no more than one contentinfo landmark | Moderate | cat.semantics, best-practice | true |
-| landmark-one-main | Ensures a navigation point to the primary content of the page. If the page contains iframes, each iframe should contain either no main landmarks or just one | Moderate | cat.semantics, best-practice | true |
+| landmark-banner-is-top-level | Ensures the banner landmark is at top level | Moderate | cat.semantics, best-practice | true |
+| landmark-contentinfo-is-top-level | Ensures the contentinfo landmark is at top level | Moderate | cat.semantics, best-practice | true |
+| landmark-main-is-top-level | Ensures the main landmark is at top level | Moderate | cat.semantics, best-practice | true |
+| landmark-no-duplicate-banner | Ensures the page has at most one banner landmark | Moderate | cat.semantics, best-practice | true |
+| landmark-no-duplicate-contentinfo | Ensures the page has at most one contentinfo landmark | Moderate | cat.semantics, best-practice | true |
+| landmark-one-main | Ensures the page has only one main landmark and each iframe in the page has at most one main landmark | Moderate | cat.semantics, best-practice | true |
 | layout-table | Ensures presentational &lt;table&gt; elements do not use &lt;th&gt;, &lt;caption&gt; elements or the summary attribute | Serious | cat.semantics, wcag2a, wcag131 | true |
 | link-in-text-block | Links can be distinguished without relying on color | Serious | cat.color, experimental, wcag2a, wcag141 | true |
 | link-name | Ensures links have discernible text | Serious | cat.name-role-value, wcag2a, wcag412, wcag244, section508, section508.22.a | true |
@@ -55,7 +55,7 @@
 | p-as-heading | Ensure p elements are not used to style headings | Serious | cat.semantics, wcag2a, wcag131, experimental | true |
 | page-has-heading-one | Ensure that the page, or at least one of its frames contains a level-one heading | Moderate | cat.semantics, best-practice | true |
 | radiogroup | Ensures related &lt;input type=&quot;radio&quot;&gt; elements have a group and that the group designation is consistent | Critical | cat.forms, best-practice | true |
-| region | Ensures all content is contained within a landmark region | Moderate | cat.keyboard, best-practice | true |
+| region | Ensures all page content is contained by landmarks | Moderate | cat.keyboard, best-practice | true |
 | scope-attr-valid | Ensures the scope attribute is used correctly on tables | Moderate, Critical | cat.tables, best-practice | true |
 | server-side-image-map | Ensures that server-side image maps are not used | Minor | cat.text-alternatives, wcag2a, wcag211, section508, section508.22.f | true |
 | skip-link | Ensure all skip links have a focusable target | Moderate | cat.keyboard, best-practice | true |
diff --git a/lib/checks/aria/invalidrole.js b/lib/checks/aria/invalidrole.js
index 1ce02fc082..86a649e87f 100644
--- a/lib/checks/aria/invalidrole.js
+++ b/lib/checks/aria/invalidrole.js
@@ -1 +1,3 @@
-return !axe.commons.aria.isValidRole(node.getAttribute('role'));
+return !axe.commons.aria.isValidRole(node.getAttribute('role'), {
+	allowAbstract: true
+});
diff --git a/lib/checks/lists/dlitem.js b/lib/checks/lists/dlitem.js
index b4f11e5315..a750334627 100644
--- a/lib/checks/lists/dlitem.js
+++ b/lib/checks/lists/dlitem.js
@@ -1,20 +1,16 @@
 const parent = axe.commons.dom.getComposedParent(node);
 const parentTagName = parent.nodeName.toUpperCase();
 
+// 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) {
+if (!parentRole || !axe.commons.aria.isValidRole(parentRole)) {
 	return true;
 }
 
-if (!axe.commons.aria.isValidRole(parentRole)) {
-	return false;
-}
-
 if (parentRole === 'list') {
 	return true;
 }
diff --git a/lib/checks/lists/listitem.js b/lib/checks/lists/listitem.js
index 3270c8b4e3..a53deec328 100644
--- a/lib/checks/lists/listitem.js
+++ b/lib/checks/lists/listitem.js
@@ -1,18 +1,18 @@
 const parent = axe.commons.dom.getComposedParent(node);
 if (!parent) {
-	return false;
+	// Can only happen with detached DOM nodes and roots:
+	return undefined;
 }
 
-const ALLOWED_TAGS = ['UL', 'OL'];
 const parentTagName = parent.nodeName.toUpperCase();
-
 const parentRole = (parent.getAttribute('role') || '').toLowerCase();
-if (parentRole && !axe.commons.aria.isValidRole(parentRole)) {
+
+if (parentRole === 'list') {
+	return true;
+}
+
+if (parentRole && axe.commons.aria.isValidRole(parentRole)) {
 	return false;
 }
 
-const isListRole = parentRole === 'list';
-return (
-	(ALLOWED_TAGS.includes(parentTagName) && (!parentRole || isListRole)) ||
-	isListRole
-);
+return ['UL', 'OL'].includes(parentTagName);
diff --git a/lib/commons/aria/roles.js b/lib/commons/aria/roles.js
index 3f2daec812..33e25cdbe2 100644
--- a/lib/commons/aria/roles.js
+++ b/lib/commons/aria/roles.js
@@ -6,15 +6,15 @@
  * @memberof axe.commons.aria
  * @instance
  * @param  {String}  role The role to check
+ * @param  {Object}  options Use `allowAbstract` if you want abstracts
  * @return {Boolean}
  */
-aria.isValidRole = function(role) {
-	'use strict';
-	if (aria.lookupTable.role[role]) {
-		return true;
+aria.isValidRole = function(role, { allowAbstract } = {}) {
+	const roleDefinition = aria.lookupTable.role[role];
+	if (!roleDefinition) {
+		return false;
 	}
-
-	return false;
+	return allowAbstract ? true : roleDefinition.type !== 'abstract';
 };
 
 /**
diff --git a/test/checks/lists/dlitem.js b/test/checks/lists/dlitem.js
index 77e7f144b8..b6b5cbc7a7 100644
--- a/test/checks/lists/dlitem.js
+++ b/test/checks/lists/dlitem.js
@@ -1,60 +1,91 @@
-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 dt element 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));
 	});
 
-	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>');
+	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>');
+	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>');
+	it('should fail if the dt element has a parent <div> with role="list"', function() {
+		var checkArgs = checkSetup(
+			'<div role="list"><dt id="target">My list item</div>'
+		);
 		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');
+	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>'
+		);
 		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>';
+	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>'
+		);
+		assert.isTrue(checks.dlitem.evaluate.apply(null, checkArgs));
+	});
 
-		var checkArgs = checkSetup(node, 'dt');
+	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));
+		}
+	);
+});
diff --git a/test/checks/lists/listitem.js b/test/checks/lists/listitem.js
index 751bd6b6e9..72c3e52f64 100644
--- a/test/checks/lists/listitem.js
+++ b/test/checks/lists/listitem.js
@@ -1,55 +1,78 @@
-describe('listitem', function () {
+describe('listitem', 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 listitem has a parent <ol>', function () {
+	it('should pass if the listitem has a parent <ol>', function() {
 		var checkArgs = checkSetup('<ol><li id="target">My list item</li></ol>');
 		assert.isTrue(checks.listitem.evaluate.apply(null, checkArgs));
 	});
 
-	it('should pass if the listitem has a parent <ul>', function () {
+	it('should pass if the listitem has a parent <ul>', function() {
 		var checkArgs = checkSetup('<ul><li id="target">My list item</li></ul>');
 		assert.isTrue(checks.listitem.evaluate.apply(null, checkArgs));
 	});
 
-	it('should pass if the listitem has a parent role=list', function () {
-		var checkArgs = checkSetup('<div role="list"><li id="target">My list item</li></div>');
+	it('should pass if the listitem has a parent role=list', function() {
+		var checkArgs = checkSetup(
+			'<div role="list"><li id="target">My list item</li></div>'
+		);
 		assert.isTrue(checks.listitem.evaluate.apply(null, checkArgs));
 	});
 
-	it('should fail if the listitem has an incorrect parent', function () {
+	it('should fail if the listitem has an incorrect parent', function() {
 		var checkArgs = checkSetup('<div><li id="target">My list item</li></div>');
 		assert.isFalse(checks.listitem.evaluate.apply(null, checkArgs));
 	});
 
-	it('should fail if the listitem has a parent <ol> with changed role', function () {
-		var checkArgs = checkSetup('<ol role="menubar"><li id="target">My list item</li></ol>');
+	it('should fail if the listitem has a parent <ol> with changed role', function() {
+		var checkArgs = checkSetup(
+			'<ol role="menubar"><li id="target">My list item</li></ol>'
+		);
 		assert.isFalse(checks.listitem.evaluate.apply(null, checkArgs));
 	});
 
-	(shadowSupport.v1 ? it : xit)('should return true in a shadow DOM pass', function () {
-		var node = document.createElement('div');
-		node.innerHTML = '<li>My list item </li>';
-		var shadow = node.attachShadow({ mode: 'open' });
-		shadow.innerHTML = '<ul><slot></slot></ul>';
-		var checkArgs = checkSetup(node, 'li');
+	it('should pass if the listitem has a parent <ol> with an invalid role', function() {
+		var checkArgs = checkSetup(
+			'<ol role="invalid-role"><li id="target">My list item</li></ol>'
+		);
 		assert.isTrue(checks.listitem.evaluate.apply(null, checkArgs));
 	});
 
-	(shadowSupport.v1 ? it : xit)('should return false in a shadow DOM fail', function () {
-		var node = document.createElement('div');
-		node.innerHTML = '<li>My list item </li>';
-		var shadow = node.attachShadow({ mode: 'open' });
-		shadow.innerHTML = '<div><slot></slot></div>';
-		var checkArgs = checkSetup(node, 'li');
-		assert.isFalse(checks.listitem.evaluate.apply(null, checkArgs));
+	it('should pass if the listitem has a parent <ol> with an abstract role', function() {
+		var checkArgs = checkSetup(
+			'<ol role="section"><li id="target">My list item</li></ol>'
+		);
+		assert.isTrue(checks.listitem.evaluate.apply(null, checkArgs));
 	});
 
+	(shadowSupport.v1 ? it : xit)(
+		'should return true in a shadow DOM pass',
+		function() {
+			var node = document.createElement('div');
+			node.innerHTML = '<li>My list item </li>';
+			var shadow = node.attachShadow({ mode: 'open' });
+			shadow.innerHTML = '<ul><slot></slot></ul>';
+			var checkArgs = checkSetup(node, 'li');
+			assert.isTrue(checks.listitem.evaluate.apply(null, checkArgs));
+		}
+	);
+
+	(shadowSupport.v1 ? it : xit)(
+		'should return false in a shadow DOM fail',
+		function() {
+			var node = document.createElement('div');
+			node.innerHTML = '<li>My list item </li>';
+			var shadow = node.attachShadow({ mode: 'open' });
+			shadow.innerHTML = '<div><slot></slot></div>';
+			var checkArgs = checkSetup(node, 'li');
+			assert.isFalse(checks.listitem.evaluate.apply(null, checkArgs));
+		}
+	);
 });
diff --git a/test/commons/aria/roles.js b/test/commons/aria/roles.js
index c7a2093ec1..65452695c7 100644
--- a/test/commons/aria/roles.js
+++ b/test/commons/aria/roles.js
@@ -13,6 +13,16 @@ describe('aria.isValidRole', function() {
 	it('should return false if role is not found in the lookup table', function() {
 		assert.isFalse(axe.commons.aria.isValidRole('cats'));
 	});
+
+	it('returns false for abstract roles by default', function() {
+		assert.isFalse(axe.commons.aria.isValidRole('input'));
+	});
+
+	it('returns true for abstract roles with { allowAbstract: true } ', function() {
+		assert.isTrue(
+			axe.commons.aria.isValidRole('input', { allowAbstract: true })
+		);
+	});
 });
 
 describe('aria.getRolesWithNameFromContents', function() {
diff --git a/test/commons/table/is-data-cell.js b/test/commons/table/is-data-cell.js
index 358b5df117..84f87c39c5 100644
--- a/test/commons/table/is-data-cell.js
+++ b/test/commons/table/is-data-cell.js
@@ -78,4 +78,17 @@ describe('table.isDataCell', function() {
 		assert.isTrue(axe.commons.table.isDataCell(target1));
 		assert.isFalse(axe.commons.table.isDataCell(target2));
 	});
+
+	it('should ignore abstract roles', function() {
+		fixture.innerHTML =
+			'<table>' +
+			'<tr><td id="target1" role="section">heading</td></tr>' +
+			'<tr><th id="target2" role="section">heading</th></tr>' +
+			'</table>';
+
+		var target1 = $id('target1');
+		var target2 = $id('target2');
+		assert.isTrue(axe.commons.table.isDataCell(target1));
+		assert.isFalse(axe.commons.table.isDataCell(target2));
+	});
 });
diff --git a/test/rule-matches/heading-matches.js b/test/rule-matches/heading-matches.js
index b04190718f..8ea4e56699 100644
--- a/test/rule-matches/heading-matches.js
+++ b/test/rule-matches/heading-matches.js
@@ -52,4 +52,10 @@ describe('heading-matches', function() {
 		h1.setAttribute('role', 'bruce');
 		assert.isTrue(rule.matches(h1));
 	});
+
+	it('should return true on headings with their role changes to an abstract role', function() {
+		var h1 = document.createElement('h1');
+		h1.setAttribute('role', 'widget');
+		assert.isTrue(rule.matches(h1));
+	});
 });