From f7f9cf358dc0fb4632d4073759f09ddf61fdfcd7 Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Wed, 25 Oct 2017 22:48:12 +0200 Subject: [PATCH] fix: skip-link rule now checks if a target exists BREAKING CHANGE: Skip-link rule no longer requires skip lins with a focusable target. --- doc/rule-descriptions.md | 11 ++++---- lib/checks/navigation/skip-link-after.js | 1 - lib/checks/navigation/skip-link.js | 6 ++++- lib/checks/navigation/skip-link.json | 6 ++--- lib/rules/skip-link-matches.js | 2 ++ lib/rules/skip-link.json | 6 ++--- test/checks/navigation/skip-link.js | 27 ++++++++++++------- .../full/skip-link/skip-link-fail.html | 23 ---------------- .../full/skip-link/skip-link-fail.js | 27 ------------------- .../full/skip-link/skip-link-pass.html | 24 ----------------- .../full/skip-link/skip-link-pass.js | 27 ------------------- .../rules/skip-link/skip-link.html | 14 ++++++++++ .../rules/skip-link/skip-link.json | 6 +++++ 13 files changed, 56 insertions(+), 124 deletions(-) delete mode 100644 lib/checks/navigation/skip-link-after.js create mode 100644 lib/rules/skip-link-matches.js delete mode 100644 test/integration/full/skip-link/skip-link-fail.html delete mode 100644 test/integration/full/skip-link/skip-link-fail.js delete mode 100644 test/integration/full/skip-link/skip-link-pass.html delete mode 100644 test/integration/full/skip-link/skip-link-pass.js create mode 100644 test/integration/rules/skip-link/skip-link.html create mode 100644 test/integration/rules/skip-link/skip-link.json diff --git a/doc/rule-descriptions.md b/doc/rule-descriptions.md index 044af174d7..f4541397ef 100644 --- 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 <iframe> and <frame> elements contain a unique title attribute | cat.text-alternatives, best-practice | true | | frame-title | Ensures <iframe> and <frame> 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 <html> element has a valid value | cat.language, wcag2a, wcag311 | true | | image-alt | Ensures <img> 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 <input type="image"> 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 <table> elements do not use <th>, <caption> 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 <object> 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 <input type="radio"> 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 <caption> 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 deleted file mode 100644 index c60b163cf3..0000000000 --- a/lib/checks/navigation/skip-link-after.js +++ /dev/null @@ -1 +0,0 @@ -return [results[0]]; diff --git a/lib/checks/navigation/skip-link.js b/lib/checks/navigation/skip-link.js index 92eb8cf509..24e8c95456 100644 --- 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 index 8e4f9baff2..2078210fb1 100644 --- 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 new file mode 100644 index 0000000000..7ee5bcbd17 --- /dev/null +++ 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 index 5883fd7a8c..8ae283cf65 100644 --- 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 index 9f8da0fb68..34e2348419 100644 --- 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 = 'Click Here

Introduction

'; + it('should return true if the href points to an element with an ID', function () { + fixture.innerHTML = 'Click Here

Introduction

'; 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 = 'Click Here'; + 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 = 'Click Here' + + '

Introduction

'; + 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 = 'Click Here

Introduction

'; + it('should return undefined if the target has aria-hidden=true', function () { + fixture.innerHTML = 'Click Here' + + '

Introduction

'; 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 deleted file mode 100644 index 97718da4c1..0000000000 --- a/test/integration/full/skip-link/skip-link-fail.html +++ /dev/null @@ -1,23 +0,0 @@ - - - -Skip-Link Test - - - - - - - -

This document is going to fail the skip-link test.

-
- - - - diff --git a/test/integration/full/skip-link/skip-link-fail.js b/test/integration/full/skip-link/skip-link-fail.js deleted file mode 100644 index cf4b18f9c1..0000000000 --- a/test/integration/full/skip-link/skip-link-fail.js +++ /dev/null @@ -1,27 +0,0 @@ -describe('skip link fail test', function () { - 'use strict'; - var results; - before(function (done) { - axe.run({ runOnly: { type: 'rule', values: ['skip-link'] } }, function (err, r) { - assert.isNull(err); - results = r; - done(); - }); - }); - - describe('violations', function () { - it('should find one', function () { - assert.lengthOf(results.violations[0].nodes, 1); - }); - - it('should find html', function () { - assert.deepEqual(results.violations[0].nodes[0].target, ['#firstlink']); - }); - }); - - describe('passes', function () { - it('should find none', function () { - assert.lengthOf(results.passes, 0); - }); - }); -}); diff --git a/test/integration/full/skip-link/skip-link-pass.html b/test/integration/full/skip-link/skip-link-pass.html deleted file mode 100644 index 799c39a81d..0000000000 --- a/test/integration/full/skip-link/skip-link-pass.html +++ /dev/null @@ -1,24 +0,0 @@ - - - -Skip-Link Test - - - - - - - -

This document is going to pass the skip-link test.

-

Focusable header

-
- - - - diff --git a/test/integration/full/skip-link/skip-link-pass.js b/test/integration/full/skip-link/skip-link-pass.js deleted file mode 100644 index ef0420d36f..0000000000 --- a/test/integration/full/skip-link/skip-link-pass.js +++ /dev/null @@ -1,27 +0,0 @@ -describe('skip link pass test', function () { - 'use strict'; - var results; - before(function (done) { - axe.run({ runOnly: { type: 'rule', values: ['skip-link'] } }, function (err, r) { - assert.isNull(err); - results = r; - done(); - }); - }); - - describe('passes', function () { - it('should find one', function () { - assert.lengthOf(results.passes[0].nodes, 1); - }); - - it('should find html', function () { - assert.deepEqual(results.passes[0].nodes[0].target, ['#firstlink']); - }); - }); - - describe('violations', function () { - it('should find none', function () { - assert.lengthOf(results.violations, 0); - }); - }); -}); diff --git a/test/integration/rules/skip-link/skip-link.html b/test/integration/rules/skip-link/skip-link.html new file mode 100644 index 0000000000..f42b69d3e3 --- /dev/null +++ b/test/integration/rules/skip-link/skip-link.html @@ -0,0 +1,14 @@ + +link +link + +
+Link + + +Link + + +Link + +bad link 1 diff --git a/test/integration/rules/skip-link/skip-link.json b/test/integration/rules/skip-link/skip-link.json new file mode 100644 index 0000000000..d93ad4013c --- /dev/null +++ b/test/integration/rules/skip-link/skip-link.json @@ -0,0 +1,6 @@ +{ + "rule": "skip-link", + "violations": [["#fail1"]], + "incomplete": [["#canttell1"]], + "passes": [["#pass1"], ["#pass2"]] +}