fix: update based on reviews

dequelabs · Apr 23, 2019 · 9772f39 · 9772f39
1 parent e99ffa1
commit 9772f39
Show file tree

Hide file tree

Showing 9 changed files with 41 additions and 22 deletions.
diff --git a/doc/rule-descriptions.md b/doc/rule-descriptions.md
@@ -66,7 +66,7 @@
 | 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 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               |
-| scrollable-region-focusable         | Ensure that scrollable region has keyboard access                                                                          | Moderate           | wcag2a, wcag211                                                              | true               |
+| scrollable-region-focusable         | Elements that have scrollable content should be accessible by keyboard                                                     | Moderate           | wcag2a, wcag211                                                              | 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               |
 | tabindex                            | Ensures tabindex attribute values are not greater than 0                                                                   | Serious            | cat.keyboard, best-practice                                                  | true               |

diff --git a/lib/checks/keyboard/focusable-content.json b/lib/checks/keyboard/focusable-content.json
@@ -5,8 +5,7 @@
 		"impact": "moderate",
 		"messages": {
 			"pass": "Element contains focusable elements",
-			"fail": "Element should have focusable content",
-			"incomplete": ""
+			"fail": "Element should have focusable content"
 		}
 	}
 }
diff --git a/lib/checks/keyboard/focusable-element.js b/lib/checks/keyboard/focusable-element.js
@@ -6,8 +6,7 @@
  */
 const isFocusable = virtualNode.isFocusable;
 
-let tabIndex = virtualNode.actualNode.getAttribute('tabindex');
-tabIndex =
-	tabIndex && !isNaN(parseInt(tabIndex, 10)) ? parseInt(tabIndex) : null;
+let tabIndex = parseInt(virtualNode.actualNode.getAttribute('tabindex'), 10);
+tabIndex = !isNaN(tabIndex) ? tabIndex : null;
 
 return tabIndex ? isFocusable && tabIndex >= 0 : isFocusable;
diff --git a/lib/core/utils/get-scroll.js b/lib/core/utils/get-scroll.js
@@ -1,16 +1,32 @@
 /**
  * Get the scroll position of given element
+ * @method getScroll
+ * @memberof axe.utils
+ * @param {Element} node
+ * @param {buffer} (Optional) allowed negligence in overflow
+ * @returns {Object | undefined}
  */
-axe.utils.getScroll = function getScroll(elm) {
+axe.utils.getScroll = function getScroll(elm, buffer = 0) {
+	if (
+		elm.scrollHeight < elm.clientHeight ||
+		elm.scrollWidth < elm.clientWidth
+	) {
+		return;
+	}
+
+	const overflowX = elm.scrollWidth - elm.clientWidth;
+	const overflowY = elm.scrollHeight - elm.clientHeight;
+	const maxOverflow = Math.max(overflowX, overflowY);
+	if (maxOverflow <= buffer) {
+		return;
+	}
+
 	const style = window.getComputedStyle(elm);
 	const visibleOverflowY = style.getPropertyValue('overflow-y') === 'visible';
 	const visibleOverflowX = style.getPropertyValue('overflow-x') === 'visible';
 
-	if (
-		// See if the element hides overflowing content
-		(!visibleOverflowY && elm.scrollHeight > elm.clientHeight) ||
-		(!visibleOverflowX && elm.scrollWidth > elm.clientWidth)
-	) {
+	// See if the element hides overflowing content
+	if (!visibleOverflowY || !visibleOverflowX) {
 		return {
 			elm,
 			top: elm.scrollTop,

diff --git a/lib/rules/scrollable-region-focusable-matches.js b/lib/rules/scrollable-region-focusable-matches.js
@@ -2,8 +2,14 @@
  * Note:
  * `excludeHidden=true` for this rule, thus considering only elements in the accessibility tree.
  */
-const nodeAndDescendents = axe.utils.querySelectorAll(virtualNode, '*');
-const scrollableElements = nodeAndDescendents.filter(
-	vNode => !!axe.utils.getScroll(vNode.actualNode)
+const { isHidden, querySelectorAll } = axe.utils;
+const nodeAndDescendents = querySelectorAll(virtualNode, '*');
+
+const hasVisibleChildren = nodeAndDescendents.filter(
+	({ actualNode }) => !isHidden(actualNode)
 );
-return scrollableElements.length > 0;
+if (!hasVisibleChildren) {
+	return false;
+}
+
+return !!axe.utils.getScroll(node, 13); //1em ~= 13px
diff --git a/lib/rules/scrollable-region-focusable.json b/lib/rules/scrollable-region-focusable.json
@@ -1,11 +1,10 @@
 {
 	"id": "scrollable-region-focusable",
-	"excludeHidden": true,
 	"matches": "scrollable-region-focusable-matches.js",
 	"tags": ["wcag2a", "wcag211"],
 	"metadata": {
-		"description": "Ensure that scrollable region has keyboard access",
-		"help": "Elements that have scrollable content should be accessible by keyboard"
+		"description": "Elements that have scrollable content should be accessible by keyboard",
+		"help": "Ensure that scrollable region has keyboard access"
 	},
 	"all": [],
 	"any": ["focusable-content", "focusable-element"],

diff --git a/test/checks/keyboard/focusable-content.js b/test/checks/keyboard/focusable-content.js
@@ -34,7 +34,7 @@ describe('focusable-content tests', function() {
 		assert.isFalse(actual);
 	});
 
-	it('returns false when parent element is focusable (only checks if contents are focusable)', function() {
+	it('returns false when element is focusable (only checks if contents are focusable)', function() {
 		var params = checkSetup(
 			'<div id="target" tabindex="0">' +
 				'<p style="height: 200px;"></p>' +

diff --git a/test/checks/keyboard/focusable-element.js b/test/checks/keyboard/focusable-element.js
@@ -30,7 +30,7 @@ describe('focusable-element tests', function() {
 		assert.isFalse(actual);
 	});
 
-	it('returns false when element is not focusable by virtue', function() {
+	it('returns false when element is not focusable by default', function() {
 		var params = checkSetup('<p id="target">I hold some text </p>');
 		var actual = check.evaluate.apply(checkContext, params);
 		assert.isFalse(actual);

diff --git a/test/playground.html b/test/playground.html
@@ -2,7 +2,7 @@
 <html lang="en">
 <title>O hai</title>
 
-<div class="foo" id="foo">foo</div>
+<div id="target" style="height: 200px; width: 200px; overflow: hidden"><div style="height: 2000px; width: 100px; background-color: pink;"><p> Content </p></div></div>
 
 <script src="/axe.js"></script>
 <script>