fix: Use getAttribute(id) over .id (#374)

* fix implicit attribute us in duplicate-id * fix: Only access node id's through getAttribute * Minor build chores for whitespace and lockfile (#366) * chore: convert spaces to tabs in Gruntfile Align the file with the EditorConfig setting * chore: ignore package-lock.json This file is produced by NPM 5, but this repo didn't use the old shrinkwrap version so this should be excluded * fix: Set relatedNodes on color/link-in-block rules (#407) * chore: ignore growl in retire * Add instructions on debugging on CircleCI * chore: Fix spacing issues * fix implicit attribute us in duplicate-id * fix: Only access node id's through getAttribute * chore: Fix spacing issues * fix: JSHint issue
dequelabs · Jul 13, 2017 · 353b53f · 353b53f
1 parent 5324040
commit 353b53f
Show file tree

Hide file tree

Showing 15 changed files with 58 additions and 36 deletions.
diff --git a/lib/checks/aria/required-parent.js b/lib/checks/aria/required-parent.js
@@ -34,11 +34,12 @@ function getAriaOwners(element) {
 		o = null;
 
 	while (element) {
-		if (element.id) {
-			o = document.querySelector('[aria-owns~=' + axe.commons.utils.escapeSelector(element.id) + ']');
+		if (element.getAttribute('id')) {
+			const id = axe.commons.utils.escapeSelector(element.getAttribute('id'));
+			o = document.querySelector(`[aria-owns~=${id}]`);
 			if (o) { owners.push(o); }
 		}
-		element = element.parentNode;
+		element = element.parentElement;
 	}
 
 	return owners.length ? owners : null;

diff --git a/lib/checks/label/explicit.js b/lib/checks/label/explicit.js
@@ -1,5 +1,7 @@
-if (node.id) {
-	var label = document.querySelector('label[for="' + axe.commons.utils.escapeSelector(node.id) + '"]');
+if (node.getAttribute('id')) {
+	const id = axe.commons.utils.escapeSelector(node.getAttribute('id'));
+	const label = document.querySelector(`label[for="${id}"]`);
+
 	if (label) {
 		return !!axe.commons.text.accessibleText(label);
 	}

diff --git a/lib/checks/label/multiple-label.js b/lib/checks/label/multiple-label.js
@@ -1,6 +1,6 @@
-var labels = [].slice.call(document.querySelectorAll('label[for="' +
-	axe.commons.utils.escapeSelector(node.id) + '"]')),
-	parent = node.parentNode;
+const id = axe.commons.utils.escapeSelector(node.getAttribute('id'));
+let labels = Array.from(document.querySelectorAll(`label[for="${id}"]`));
+let parent = node.parentNode;
 
 if (labels.length) {
 	// filter out hidden labels because they're fine

diff --git a/lib/checks/shared/duplicate-id.js b/lib/checks/shared/duplicate-id.js
@@ -1,11 +1,13 @@
 // Since empty ID's are not meaningful and are ignored by Edge, we'll
 // let those pass.
-if (!node.id.trim()) {
+if (!node.getAttribute('id').trim()) {
 	return true;
 }
 
-var matchingNodes = document.querySelectorAll('[id="' + axe.commons.utils.escapeSelector(node.id) + '"]');
+const id = axe.commons.utils.escapeSelector(node.getAttribute('id'));
+var matchingNodes = document.querySelectorAll(`[id="${id}"]`);
 var related = [];
+
 for (var i = 0; i < matchingNodes.length; i++) {
 	if (matchingNodes[i] !== node) {
 		related.push(matchingNodes[i]);

diff --git a/lib/checks/tables/td-headers-attr.js b/lib/checks/tables/td-headers-attr.js
@@ -8,8 +8,8 @@ for (var rowIndex = 0, rowLength = node.rows.length; rowIndex < rowLength; rowIn
 }
 
 var ids = cells.reduce(function (ids, cell) {
-	if (cell.id) {
-		ids.push(cell.id);
+	if (cell.getAttribute('id')) {
+		ids.push(cell.getAttribute('id'));
 	}
 	return ids;
 }, []);
@@ -30,8 +30,8 @@ var badCells = cells.reduce(function (badCells, cell) {
 
 	if (headers.length !== 0) {
 		// Check if the cell's id is in this list
-		if (cell.id) {
-			isSelf = (headers.indexOf(cell.id.trim()) !== -1);
+		if (cell.getAttribute('id')) {
+			isSelf = (headers.indexOf(cell.getAttribute('id').trim()) !== -1);
 		}
 
 		// Check if the headers are of cells inside the table

diff --git a/lib/checks/tables/th-has-data-cells.js b/lib/checks/tables/th-has-data-cells.js
@@ -30,7 +30,8 @@ var tableGrid = tableUtils.toGrid(node);
 
 // Look for all the bad headers
 var out = headers.reduce(function (res, header) {
-	if (header.id && reffedHeaders.indexOf(header.id) !== -1) {
+	if (header.getAttribute('id') &&
+			reffedHeaders.includes(header.getAttribute('id'))) {
 		return (!res ? res : true);
 	}
 

diff --git a/lib/commons/table/is-header.js b/lib/commons/table/is-header.js
@@ -10,8 +10,9 @@ table.isHeader = function (cell) {
 		return true;
 	}
 
-	if (cell.id) {
-		return !!document.querySelector('[headers~="' + axe.utils.escapeSelector(cell.id) + '"]');
+	if (cell.getAttribute('id')) {
+		const id = axe.utils.escapeSelector(cell.getAttribute('id'));
+		return !!document.querySelector(`[headers~="${id}"]`);
 	}
 
 	return false;

diff --git a/lib/commons/text/accessible-text.js b/lib/commons/text/accessible-text.js
@@ -21,8 +21,9 @@ var phrasingElements = ['A', 'EM', 'STRONG', 'SMALL', 'MARK', 'ABBR', 'DFN', 'I'
  */
 function findLabel(element) {
 	var ref = null;
-	if (element.id) {
-		ref = document.querySelector('label[for="' + axe.utils.escapeSelector(element.id) + '"]');
+	if (element.getAttribute('id')) {
+		const id = axe.utils.escapeSelector(element.getAttribute('id'));
+		ref = document.querySelector(`label[for="${id}"]`);
 		if (ref) {
 			return ref;
 		}

diff --git a/lib/commons/text/label.js b/lib/commons/text/label.js
@@ -14,8 +14,9 @@ text.label = function (node) {
 	}
 
 	// explicit label
-	if (node.id) {
-		ref = document.querySelector('label[for="' + axe.utils.escapeSelector(node.id) + '"]');
+	if (node.getAttribute('id')) {
+		const id = axe.commons.utils.escapeSelector(node.getAttribute('id'));
+		ref = document.querySelector(`label[for="${id}"]`);
 		candidate = ref && text.visible(ref, true);
 		if (candidate) {
 			return candidate;

diff --git a/lib/core/utils/get-selector.js b/lib/core/utils/get-selector.js
@@ -49,10 +49,10 @@ function getNthChildString (elm, selector) {
 const createSelector = {
   // Get ID properties
   getElmId (elm) {
-  	if (!elm.id) {
+  	if (!elm.getAttribute('id')) {
   		return;
   	}
-  	const id = '#' + escapeSelector(elm.id || '');
+  	const id = '#' + escapeSelector(elm.getAttribute('id') || '');
     if (
     	// Don't include youtube's uid values, they change  on reload
     	!id.match(/player_uid_/) &&
@@ -88,7 +88,7 @@ const createSelector = {
   },
   // Has a name property, but no ID (Think input fields)
   getElmNameProp (elm) {
-    if (!elm.id && elm.name) {
+    if (!elm.hasAttribute('id') && elm.name) {
       return '[name="' + escapeSelector(elm.name) + '"]';
     }
   },

diff --git a/lib/core/utils/get-xpath.js b/lib/core/utils/get-xpath.js
@@ -46,16 +46,15 @@ function getXPathArray(node, path) {
 		} while (sibling);
 	}
 
-	if(node.nodeType === 1) {
+	if (node.nodeType === 1) {
 		var element = {};
 		element.str = node.nodeName.toLowerCase();
 		// add the id and the count so we can construct robust versions of the xpath
-		if(node.getAttribute && node.getAttribute('id') &&
-			node.ownerDocument.querySelectorAll('#' + axe.utils.escapeSelector(node.id)).length === 1) {
-
+		var id = node.getAttribute && axe.utils.escapeSelector(node.getAttribute('id'));
+		if (id && node.ownerDocument.querySelectorAll('#' + id).length === 1) {
 			element.id = node.getAttribute('id');
 		}
-		if(count > 1) {
+		if (count > 1) {
 			element.count = count;
 		}
 		path.push(element);

diff --git a/lib/rules/color-contrast-matches.js b/lib/rules/color-contrast-matches.js
@@ -54,9 +54,10 @@ if (nodeName === 'LABEL' || nodeParentLabel) {
 }
 
 // label of disabled control associated w/ aria-labelledby
-if (node.id) {
-	var candidate = doc.querySelector('[aria-labelledby~=' + axe.commons.utils.escapeSelector(node.id) + ']');
-	if (candidate && candidate.disabled) {
+if (node.getAttribute('id')) {
+	const id = axe.commons.utils.escapeSelector(node.getAttribute('id'));
+	const candidate = doc.querySelector(`[aria-labelledby~="${id}"]`);
+	if (candidate && candidate.hasAttribute('disabled')) {
 		return false;
 	}
 }

diff --git a/test/checks/shared/duplicate-id.js b/test/checks/shared/duplicate-id.js
@@ -49,4 +49,13 @@ describe('duplicate-id', function () {
 		assert.isTrue(checks['duplicate-id'].evaluate.call(checkContext, node));
 	});
 
+	it('should allow overwrote ids', function () {
+		fixture.innerHTML = '<form data-testelm="1" id="target"><label>mylabel' + 
+			'<input name="id">' +
+		'</label></form>';
+		var node = fixture.querySelector('[data-testelm="1"]');
+
+		assert.isTrue(checks['duplicate-id'].evaluate.call(checkContext, node));
+	});
+
 });
diff --git a/test/commons/table/is-header.js b/test/commons/table/is-header.js
@@ -5,9 +5,11 @@ describe('table.isHeader', function () {
 	}
 
 	var fixture = $id('fixture');
+	var cell;
 
 	afterEach(function () {
-		fixture.innerHTML = '';
+		fixture.innerHTML = '<table><tr><th id="cell"></th></tr></table>';
+		cell = $id('cell');
 	});
 
 	it('should return true if table.isColumnHeader return true', function () {
@@ -19,7 +21,7 @@ describe('table.isHeader', function () {
 		axe.commons.table.isRowHeader = function () {
 			return false;
 		};
-		assert.isTrue(axe.commons.table.isHeader({}));
+		assert.isTrue(axe.commons.table.isHeader(cell));
 
 		axe.commons.table.isColumnHeader = orig;
 		axe.commons.table.isRowHeader = orig2;
@@ -34,7 +36,7 @@ describe('table.isHeader', function () {
 		axe.commons.table.isColumnHeader = function () {
 			return false;
 		};
-		assert.isTrue(axe.commons.table.isHeader({}));
+		assert.isTrue(axe.commons.table.isHeader(cell));
 
 		axe.commons.table.isRowHeader = orig;
 		axe.commons.table.isColumnHeader = orig2;
@@ -49,7 +51,7 @@ describe('table.isHeader', function () {
 		axe.commons.table.isColumnHeader = function () {
 			return false;
 		};
-		assert.isFalse(axe.commons.table.isHeader({}));
+		assert.isFalse(axe.commons.table.isHeader(cell));
 
 		axe.commons.table.isRowHeader = orig;
 		axe.commons.table.isColumnHeader = orig2;

diff --git a/test/core/utils/get-selector.js b/test/core/utils/get-selector.js
@@ -181,9 +181,11 @@ describe('axe.utils.getSelector', function () {
 		var sel = axe.utils.getSelector({
 			nodeName: 'a',
 			classList: [],
+			getAttribute: function () { },
 			hasAttribute: function () { return false; },
 			parentNode: {
 				nodeName: 'b',
+				getAttribute: function () { },
 				hasAttribute: function () { return false; },
 				classList: []
 			}