fix: skip-link rule now checks if a target exists

BREAKING CHANGE: Skip-link rule no longer requires skip lins with a focusable target.
dequelabs · Nov 13, 2017 · f7f9cf3 · f7f9cf3
1 parent 18263eb
commit f7f9cf3
Show file tree

Hide file tree

Showing 13 changed files with 56 additions and 124 deletions.
diff --git a/doc/rule-descriptions.md b/doc/rule-descriptions.md
@@ -23,15 +23,14 @@
 | empty-heading | Ensures headings have discernible text | cat.name-role-value, best-practice | true |
 | frame-title-unique | Ensures &lt;iframe&gt; and &lt;frame&gt; elements contain a unique title attribute | cat.text-alternatives, best-practice | true |
 | frame-title | Ensures &lt;iframe&gt; and &lt;frame&gt; elements contain a non-empty title attribute | cat.text-alternatives, wcag2a, wcag241, section508, section508.22.i | true |
-| heading-order | Ensures the order of headings is semantically correct | cat.semantics, best-practice | false |
-| hidden-content | Informs users about hidden content. | experimental, review-item | false |
-| href-no-hash | Ensures that href values are valid link references to promote only using anchors as links | cat.semantics, best-practice | false |
+| heading-order | Ensures the order of headings is semantically correct | cat.semantics, best-practice | true |
+| hidden-content | Informs users about hidden content. | experimental, review-item | true |
 | html-has-lang | Ensures every HTML document has a lang attribute | cat.language, wcag2a, wcag311 | true |
 | html-lang-valid | Ensures the lang attribute of the &lt;html&gt; element has a valid value | cat.language, wcag2a, wcag311 | true |
 | image-alt | Ensures &lt;img&gt; elements have alternate text or a role of none or presentation | cat.text-alternatives, wcag2a, wcag111, section508, section508.22.a | true |
 | image-redundant-alt | Ensure button and link text is not repeated as image alternative | cat.text-alternatives, best-practice | true |
 | input-image-alt | Ensures &lt;input type=&quot;image&quot;&gt; elements have alternate text | 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 | cat.forms, best-practice | false |
+| label-title-only | Ensures that every form element is not solely labeled using the title or aria-describedby attributes | cat.forms, best-practice | true |
 | label | Ensures every form element has a label | cat.forms, wcag2a, wcag332, wcag131, section508, section508.22.n | true |
 | layout-table | Ensures presentational &lt;table&gt; elements do not use &lt;th&gt;, &lt;caption&gt; elements or the summary attribute | cat.semantics, wcag2a, wcag131 | true |
 | link-in-text-block | Links can be distinguished without relying on color | cat.color, experimental, wcag2a, wcag141 | true |
@@ -45,10 +44,10 @@
 | object-alt | Ensures &lt;object&gt; elements have alternate text | cat.text-alternatives, wcag2a, wcag111, section508, section508.22.a | true |
 | p-as-heading | Ensure p elements are not used to style headings | cat.semantics, wcag2a, wcag131, experimental | true |
 | radiogroup | Ensures related &lt;input type=&quot;radio&quot;&gt; elements have a group and that the group designation is consistent | cat.forms, best-practice | true |
-| region | Ensures all content is contained within a landmark region | cat.keyboard, best-practice | false |
+| region | Ensures all content is contained within a landmark region | cat.keyboard, best-practice | true |
 | scope-attr-valid | Ensures the scope attribute is used correctly on tables | cat.tables, best-practice | true |
 | server-side-image-map | Ensures that server-side image maps are not used | cat.text-alternatives, wcag2a, wcag211, section508, section508.22.f | true |
-| skip-link | Ensures the first link on the page is a skip link | cat.keyboard, best-practice | false |
+| skip-link | Ensure all skip links have a focusable target | cat.keyboard, best-practice | true |
 | tabindex | Ensures tabindex attribute values are not greater than 0 | cat.keyboard, best-practice | true |
 | table-duplicate-name | Ensure that tables do not have the same summary and caption | cat.tables, best-practice | true |
 | table-fake-caption | Ensure that tables with a caption use the &lt;caption&gt; element. | cat.tables, experimental, wcag2a, wcag131, section508, section508.22.g | true |

diff --git a/lib/checks/navigation/skip-link-after.js b/lib/checks/navigation/skip-link-after.js
diff --git a/lib/checks/navigation/skip-link.js b/lib/checks/navigation/skip-link.js
@@ -1 +1,5 @@
-return axe.commons.dom.isFocusable(axe.commons.dom.getElementByReference(node, 'href'));
+const target = axe.commons.dom.getElementByReference(node, 'href');
+if (target) {
+  return axe.commons.dom.isVisible(target, true) || undefined;
+}
+return false;
diff --git a/lib/checks/navigation/skip-link.json b/lib/checks/navigation/skip-link.json
@@ -1,12 +1,12 @@
 {
   "id": "skip-link",
   "evaluate": "skip-link.js",
-  "after": "skip-link-after.js",
   "metadata": {
     "impact": "moderate",
     "messages": {
-      "pass": "Valid skip link found",
-      "fail": "No valid skip link found"
+      "pass": "Skip link target exists",
+      "incomplete": "Skip link target should become visible on activation",
+      "fail": "No skip link target"
     }
   }
 }
diff --git a/lib/rules/skip-link-matches.js b/lib/rules/skip-link-matches.js
@@ -0,0 +1,2 @@
+const href = node.getAttribute('href');
+return (href[0] === '#' && href.length > 1);
diff --git a/lib/rules/skip-link.json b/lib/rules/skip-link.json
@@ -1,14 +1,14 @@
 {
   "id": "skip-link",
   "selector": "a[href]",
-  "pageLevel": true,
+  "matches": "skip-link-matches.js",
   "tags": [
     "cat.keyboard",
     "best-practice"
   ],
   "metadata": {
-    "description": "Ensures the first link on the page is a skip link",
-    "help": "The page should have a skip link as its first link"
+    "description": "Ensure all skip links have a focusable target",
+    "help": "The skip-link target should exist and be focusable"
   },
   "all": [],
   "any": [

diff --git a/test/checks/navigation/skip-link.js b/test/checks/navigation/skip-link.js
@@ -19,20 +19,29 @@ describe('skip-link', function () {
 		assert.isFalse(checks['skip-link'].evaluate(node));
 	});
 
-	it('should return false if the href points to a non-focusable element', function () {
-		fixture.innerHTML = '<a href="#mainheader">Click Here</a><h1 id="mainheader">Introduction</h1>';
+	it('should return true if the href points to an element with an ID', function () {
+		fixture.innerHTML = '<a href="#target">Click Here</a><h1 id="target">Introduction</h1>';
 		var node = fixture.querySelector('a');
-		assert.isFalse(checks['skip-link'].evaluate(node));
+		assert.isTrue(checks['skip-link'].evaluate(node));
+	});
+
+	it('should return true if the href points to an element with an name', function () {
+		fixture.innerHTML = '<a href="#target">Click Here</a><a name="target"></a>';
+		var node = fixture.querySelector('a');
+		assert.isTrue(checks['skip-link'].evaluate(node));
 	});
 
-	it('should return first result of an array', function () {
-		var results = [1, 2, 3];
-		assert.equal(checks['skip-link'].after(results), 1);
+	it('should return undefined if the target has display:none', function () {
+		fixture.innerHTML = '<a href="#target">Click Here</a>' +
+			'<h1 id="target" style="display:none">Introduction</h1>';
+		var node = fixture.querySelector('a');
+		assert.isUndefined(checks['skip-link'].evaluate(node));
 	});
 
-	it('should return true if the href points to a focusable element', function () {
-		fixture.innerHTML = '<a href="#mainheader">Click Here</a><h1 id="mainheader" tabindex="0">Introduction</h1>';
+	it('should return undefined if the target has aria-hidden=true', function () {
+		fixture.innerHTML = '<a href="#target">Click Here</a>' +
+			'<h1 id="target" aria-hidden="true">Introduction</h1>';
 		var node = fixture.querySelector('a');
-		assert.isTrue(checks['skip-link'].evaluate(node));
+		assert.isUndefined(checks['skip-link'].evaluate(node));
 	});
 });
diff --git a/test/integration/full/skip-link/skip-link-fail.html b/test/integration/full/skip-link/skip-link-fail.html
diff --git a/test/integration/full/skip-link/skip-link-fail.js b/test/integration/full/skip-link/skip-link-fail.js
diff --git a/test/integration/full/skip-link/skip-link-pass.html b/test/integration/full/skip-link/skip-link-pass.html
diff --git a/test/integration/full/skip-link/skip-link-pass.js b/test/integration/full/skip-link/skip-link-pass.js
diff --git a/test/integration/rules/skip-link/skip-link.html b/test/integration/rules/skip-link/skip-link.html
@@ -0,0 +1,14 @@
+
+<a href="foo#bar" id="ignore1">link</a>
+<a href="#" id="ignore2">link</a>
+
+<div id="pass1-tgt"></div>
+<a href="#pass1-tgt" id="pass1">Link</a>
+
+<a name="pass2-tgt"></a>
+<a href="#pass2-tgt" id="pass2">Link</a>
+
+<div id="canttell1-tgt" style="display:none"></div>
+<a href="#canttell1-tgt" id="canttell1">Link</a>
+
+<a href="#fail1-tgt" id="fail1">bad link 1</a>
diff --git a/test/integration/rules/skip-link/skip-link.json b/test/integration/rules/skip-link/skip-link.json
@@ -0,0 +1,6 @@
+{
+  "rule": "skip-link",
+  "violations": [["#fail1"]],
+  "incomplete": [["#canttell1"]],
+  "passes": [["#pass1"], ["#pass2"]]
+}
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		const href = node.getAttribute('href');
		return (href[0] === '#' && href.length > 1);