From 9772f39f547e1a7c74c8a3aba55e6173e542cadf Mon Sep 17 00:00:00 2001 From: jkodu Date: Tue, 23 Apr 2019 12:32:09 +0100 Subject: [PATCH] fix: update based on reviews --- doc/rule-descriptions.md | 2 +- lib/checks/keyboard/focusable-content.json | 3 +- lib/checks/keyboard/focusable-element.js | 5 ++-- lib/core/utils/get-scroll.js | 28 +++++++++++++++---- .../scrollable-region-focusable-matches.js | 14 +++++++--- lib/rules/scrollable-region-focusable.json | 5 ++-- test/checks/keyboard/focusable-content.js | 2 +- test/checks/keyboard/focusable-element.js | 2 +- test/playground.html | 2 +- 9 files changed, 41 insertions(+), 22 deletions(-) diff --git a/doc/rule-descriptions.md b/doc/rule-descriptions.md index 6363e26a72..0af6f09479 100644 --- a/doc/rule-descriptions.md +++ b/doc/rule-descriptions.md @@ -66,7 +66,7 @@ | radiogroup | Ensures related <input type="radio"> 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 index f4afb1e49c..7998b4ba3d 100644 --- 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 index 250e6ecb84..eacb898168 100644 --- 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 index 80f4039d28..b617757aa7 100644 --- 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 index f5aac93b99..9f1de463a2 100644 --- 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 index 38196eaf57..ff7bdf0472 100644 --- 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 index a099cb2875..7a2e396ec4 100644 --- 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( '
' + '

' + diff --git a/test/checks/keyboard/focusable-element.js b/test/checks/keyboard/focusable-element.js index 49cc3e6ab9..179936c6dc 100644 --- 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('

I hold some text

'); var actual = check.evaluate.apply(checkContext, params); assert.isFalse(actual); diff --git a/test/playground.html b/test/playground.html index afcbbf9e76..ebe81946d7 100644 --- a/test/playground.html +++ b/test/playground.html @@ -2,7 +2,7 @@ O hai -
foo
+

Content