Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: consistently parse tabindex, following HTML 5 spec #4637

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/checks/keyboard/focusable-no-name-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { isFocusable } from '../../commons/dom';
import { accessibleTextVirtual } from '../../commons/text';
import { parseTabindex } from '../../core/utils';
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved

function focusableNoNameEvaluate(node, options, virtualNode) {
const tabIndex = virtualNode.attr('tabindex');
const inFocusOrder = isFocusable(virtualNode) && tabIndex > -1;
const tabIndex = parseTabindex(virtualNode.attr('tabindex'));
const inFocusOrder =
isFocusable(virtualNode) && tabIndex !== null && tabIndex > -1;
if (!inFocusOrder) {
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/checks/keyboard/no-focusable-content-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import isFocusable from '../../commons/dom/is-focusable';
import { getRoleType } from '../../commons/aria';
import { parseTabindex } from '../../core/utils';

export default function noFocusableContentEvaluate(node, options, virtualNode) {
if (!virtualNode.children) {
Expand Down Expand Up @@ -51,6 +52,6 @@ function getFocusableDescendants(vNode) {
}

function usesUnreliableHidingStrategy(vNode) {
const tabIndex = parseInt(vNode.attr('tabindex'), 10);
const tabIndex = parseTabindex(vNode.attr('tabindex'));
return !isNaN(tabIndex) && tabIndex < 0;
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
}
4 changes: 3 additions & 1 deletion lib/checks/keyboard/tabindex-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { parseTabindex } from '../../core/utils';

function tabindexEvaluate(node, options, virtualNode) {
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
const tabIndex = parseInt(virtualNode.attr('tabindex'), 10);
const tabIndex = parseTabindex(virtualNode.attr('tabindex'));

// an invalid tabindex will either return 0 or -1 (based on the element) so
// will never be above 0
Expand Down
5 changes: 2 additions & 3 deletions lib/commons/dom/get-tabbable-elements.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { querySelectorAll } from '../../core/utils';
import { parseTabindex } from '../../core/utils';

/**
* Get all elements (including given node) that are part of the tab order
Expand All @@ -13,9 +14,7 @@ function getTabbableElements(virtualNode) {

const tabbableElements = nodeAndDescendents.filter(vNode => {
const isFocusable = vNode.isFocusable;
let tabIndex = vNode.actualNode.getAttribute('tabindex');
tabIndex =
tabIndex && !isNaN(parseInt(tabIndex, 10)) ? parseInt(tabIndex) : null;
const tabIndex = parseTabindex(vNode.actualNode.getAttribute('tabindex'));

return tabIndex ? isFocusable && tabIndex >= 0 : isFocusable;
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
});
Expand Down
3 changes: 2 additions & 1 deletion lib/commons/dom/inserted-into-focus-order.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import isFocusable from './is-focusable';
import isNativelyFocusable from './is-natively-focusable';
import { parseTabindex } from '../../core/utils';

/**
* Determines if an element is in the focus order, but would not be if its
Expand All @@ -12,7 +13,7 @@ import isNativelyFocusable from './is-natively-focusable';
* if its tabindex were removed. Else, false.
*/
function insertedIntoFocusOrder(el) {
const tabIndex = parseInt(el.getAttribute('tabindex'), 10);
const tabIndex = parseTabindex(el.getAttribute('tabindex'));

// an element that has an invalid tabindex will return 0 or -1 based on
// if it is natively focusable or not, which will always be false for this
Expand Down
5 changes: 3 additions & 2 deletions lib/commons/dom/is-focusable.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import focusDisabled from './focus-disabled';
import isNativelyFocusable from './is-natively-focusable';
import { nodeLookup } from '../../core/utils';
import { parseTabindex } from '../../core/utils';

/**
* Determines if an element is keyboard or programmatically focusable.
Expand All @@ -23,6 +24,6 @@ export default function isFocusable(el) {
return true;
}
// check if the tabindex is specified and a parseable number
const tabindex = vNode.attr('tabindex');
return tabindex && /^\s*([-+]?\d+)/.test(tabindex);
const tabindex = parseTabindex(vNode.attr('tabindex'));
return tabindex !== null;
}
3 changes: 2 additions & 1 deletion lib/commons/dom/is-in-tab-order.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { nodeLookup } from '../../core/utils';
import isFocusable from './is-focusable';
import { parseTabindex } from '../../core/utils';

/**
* Determines if an element is focusable and able to be tabbed to.
Expand All @@ -16,7 +17,7 @@ export default function isInTabOrder(el) {
return false;
}

const tabindex = parseInt(vNode.attr('tabindex', 10));
const tabindex = parseTabindex(vNode.attr('tabindex'));
if (tabindex <= -1) {
return false; // Elements with tabindex=-1 are never in the tab order
}
Expand Down
4 changes: 3 additions & 1 deletion lib/core/base/context/create-frame-context.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { parseTabindex } from '../../utils';

export function createFrameContext(frame, { focusable, page }) {
return {
node: frame,
Expand All @@ -11,7 +13,7 @@ export function createFrameContext(frame, { focusable, page }) {
}

function frameFocusable(frame) {
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
const tabIndex = frame.getAttribute('tabindex');
const tabIndex = parseTabindex(frame.getAttribute('tabindex'));
if (!tabIndex) {
return true;
}
Expand Down
1 change: 1 addition & 0 deletions lib/core/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export { default as objectHasOwn } from './object-has-own';
export { default as parseCrossOriginStylesheet } from './parse-crossorigin-stylesheet';
export { default as parseSameOriginStylesheet } from './parse-sameorigin-stylesheet';
export { default as parseStylesheet } from './parse-stylesheet';
export { default as parseTabindex } from './parse-tabindex';
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
export { default as performanceTimer } from './performance-timer';
export { pollyfillElementsFromPoint } from './pollyfill-elements-from-point';
export { default as preloadCssom } from './preload-cssom';
Expand Down
22 changes: 22 additions & 0 deletions lib/core/utils/parse-tabindex.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* Parses a tabindex value to return an integer if valid, or null if invalid.
* @method parseTabindex
* @memberof axe.utils
* @param {string|null} str
* @return {number|null}
*/
function parseTabindex(value) {
if (value === null || typeof value !== 'string') {
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
return null;
}

// Trim whitespace and check if it’s a valid integer format
const match = value.trim().match(/^([-+]?\d+)$/);
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
if (match) {
return Number(match[1]);
}

return null;
}

export default parseTabindex;
3 changes: 2 additions & 1 deletion lib/rules/autocomplete-matches.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { sanitize } from '../commons/text';
import standards from '../standards';
import { isVisibleToScreenReaders, isVisibleOnScreen } from '../commons/dom';
import { parseTabindex } from '../core/utils';

function autocompleteMatches(node, virtualNode) {
const autocomplete = virtualNode.attr('autocomplete');
Expand Down Expand Up @@ -34,7 +35,7 @@ function autocompleteMatches(node, virtualNode) {
// The element has `tabindex="-1"` and has a [[semantic role]] that is
// not a [widget](https://www.w3.org/TR/wai-aria-1.1/#widget_roles)
const role = virtualNode.attr('role');
const tabIndex = virtualNode.attr('tabindex');
const tabIndex = parseTabindex(virtualNode.attr('tabindex'));
if (tabIndex === '-1' && role) {
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
const roleDef = standards.ariaRoles[role];
if (roleDef === undefined || roleDef.type !== 'widget') {
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/no-negative-tabindex-matches.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { parseTabindex } from '../core/utils';

function noNegativeTabindexMatches(node, virtualNode) {
const tabindex = parseInt(virtualNode.attr('tabindex'), 10);
const tabindex = parseTabindex(virtualNode.attr('tabindex'));
return isNaN(tabindex) || tabindex >= 0;
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
35 changes: 35 additions & 0 deletions test/core/utils/parse-tabindex.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
describe('axe.utils.parseTabindex', function () {
'use strict';

it('should return 0 for "0"', function () {
assert.strictEqual(axe.utils.parseTabindex('0'), 0);
});

it('should return 1 for "+1"', function () {
assert.strictEqual(axe.utils.parseTabindex('+1'), 1);
});

it('should return -1 for "-1"', function () {
assert.strictEqual(axe.utils.parseTabindex('-1'), -1);
});

it('should return null for null', function () {
assert.strictEqual(axe.utils.parseTabindex(null), null);
});

it('should return null for an empty string', function () {
assert.strictEqual(axe.utils.parseTabindex(''), null);
});

it('should return null for a whitespace string', function () {
assert.strictEqual(axe.utils.parseTabindex(' '), null);
});

it('should return null for non-numeric strings', function () {
assert.strictEqual(axe.utils.parseTabindex('abc'), null);
});

it('should return null for decimal numbers', function () {
assert.strictEqual(axe.utils.parseTabindex('2.5'), null);
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
});
ahmedhalac marked this conversation as resolved.
Show resolved Hide resolved
});