fix implicit attribute use in duplicate-id #374

WilcoFiers · 2017-06-20T15:31:10Z

The id property can be overloaded, so we should use getAttribute()

dylanb · 2017-06-21T14:40:06Z

ok, I deleted the confusing comments and replies so I could clarify:

This only happens on form elements
When it happens, the id attribute changes from a string to a reference to an HTMLElement
There are 39 places in the code where we refer to the element.id - this changes only one of them

I am not sure whether this is documented browser behavior or a side-effect? Can you find a reference?

If it is documented, we might be better off detecting it and having a rule that raises this as a violation rather than trying to fix this everywhere.

marcysutton · 2017-06-24T21:28:48Z

lib/checks/aria/required-parent.js

+		if (element.getAttribute('id')) {
+			o = document.querySelector(
+				'[aria-owns~=' + 
+				axe.commons.utils.escapeSelector(element.getAttribute('id')) +


The line breaks don't make this much easier to read. Perhaps the escapeSelector line should be indented to indicate a group?

WilcoFiers · 2017-06-30T15:40:12Z

@dylanb @marcysutton Any further comments?

marcysutton · 2017-06-30T18:32:25Z

Looks good to me!

dylanb · 2017-06-30T19:08:39Z

Don't merge

…

--Dylan

On Jun 30, 2017, at 11:40 AM, Wilco Fiers ***@***.***> wrote: @dylanb @marcysutton Any further comments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

WilcoFiers · 2017-07-01T12:31:25Z

@dylanb Don't merge, because ... ?

dylanb · 2017-07-02T06:11:52Z

lib/checks/forms/labelledby.js

@@ -4,7 +4,11 @@ this.data({
 });

 var matchingNodes = document.querySelectorAll('input[type="' +
-	axe.commons.utils.escapeSelector(node.type) + '"][name="' + axe.commons.utils.escapeSelector(node.name) + '"]');
+	axe.commons.utils.escapeSelector(node.getAttribute('type')) +


Why these changes?

dylanb · 2017-07-02T06:12:56Z

lib/checks/navigation/unique-frame-title.js

@@ -1,3 +1,5 @@
-var title = axe.commons.text.sanitize(node.title).trim().toLowerCase();
+var title = axe.commons.text.sanitize(


dylanb · 2017-07-02T06:14:01Z

lib/commons/dom/is-in-text-block.js

@@ -75,7 +75,7 @@ dom.isInTextBlock = function isInTextBlock(node) {
 			return false;

 		// Don't walk links, we're only interested in what's not in them.
-		} else if ((nodeName === 'A' && currNode.href) ||
+		} else if ((nodeName === 'A' && currNode.hasAttribute('href')) ||


This is not the same - don't change this

dylanb · 2017-07-02T06:14:22Z

lib/commons/text/accessible-text.js

@@ -32,21 +33,23 @@ function findLabel(element) {
 }

 function isButton(element) {
-	return ['button', 'reset', 'submit'].indexOf(element.type) !== -1;
+	return ['button', 'reset', 'submit'].includes(
+		(element.getAttribute('type') || '').toLowerCase()


Don't change this

dylanb · 2017-07-02T06:14:49Z

lib/commons/text/accessible-text.js

 }

 function isInput(element) {
 	var nodeName = element.nodeName.toUpperCase();
 	return (nodeName === 'TEXTAREA' || nodeName === 'SELECT') ||
-		(nodeName === 'INPUT' && element.type.toLowerCase() !== 'hidden');
+		(nodeName === 'INPUT' && (element.getAttribute('type') || '').toLowerCase() !== 'hidden');


don't change this

dylanb · 2017-07-02T06:15:07Z

lib/commons/text/accessible-text.js

 		default:
 			return false;
 	}
 }

 function shouldCheckAlt(element) {
 	var nodeName = element.nodeName.toUpperCase();
-	return (nodeName === 'INPUT' && element.type.toLowerCase() === 'image') ||
-		['IMG', 'APPLET', 'AREA'].indexOf(nodeName) !== -1;
+	return (nodeName === 'INPUT' && (element.getAttribute('type') || '').toLowerCase() === 'image') ||


don't change this

dylanb · 2017-07-02T06:15:25Z

lib/commons/text/accessible-text.js

@@ -199,7 +202,12 @@ text.accessibleText = function(element, inLabelledByContext) {

 		if (isInput(element) && !inControlContext) {
 			if (isButton(element)) {
-				return element.value || element.title || defaultButtonValues[element.type] || '';
+				return (


don't change

dylanb · 2017-07-02T06:15:41Z

lib/core/utils/get-selector.js

@@ -81,15 +81,15 @@ const createSelector = {
    	nodeName = escapeSelector(nodeName);
      // Add [type] if nodeName is an input element
      if (nodeName === 'input' && elm.hasAttribute('type')) {
-        nodeName += '[type="' + elm.type + '"]';
+        nodeName += `[type="${ elm.getAttribute('type') }"]`;


don't change

dylanb · 2017-07-02T06:15:52Z

lib/core/utils/get-selector.js

      }
      return nodeName;
    }
  },
  // Has a name property, but no ID (Think input fields)
  getElmNameProp (elm) {
-    if (!elm.id && elm.name) {
-      return '[name="' + escapeSelector(elm.name) + '"]';
+    if (!elm.getAttribute('id') && elm.getAttribute('name')) {


don't change

dylanb · 2017-07-02T06:16:16Z