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.

Sign up for GitHub

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(frame-focusable-content): don't fail for elements with negative tabindex #3493

Merged
merged 4 commits into from
Jun 3, 2022
+61 −29
Merged

fix(frame-focusable-content): don't fail for elements with negative tabindex #3493

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39e32b1
fix(frame-focsuable-content): don't fail for elements with negative t…
straker Jun 1, 2022
2ea0dea
revert playground'
straker Jun 1, 2022
48fd1d7
remove test case
straker Jun 1, 2022
0afb521
Update lib/checks/keyboard/frame-focusable-content-evaluate.js
straker Jun 3, 2022
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
37 changes: 20 additions & 17 deletions lib/checks/keyboard/frame-focusable-content-evaluate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,26 @@
import isFocusable from '../../commons/dom/is-focusable';

export default function frameFocusableContentEvaluate(
node,
options,
virtualNode
) {
if (!virtualNode.children) {
return undefined;
}

try {
return !virtualNode.children.some(child => {
return focusableDescendants(child);
});
} catch (e) {
return undefined;
}
}

function focusableDescendants(vNode) {
if (isFocusable(vNode)) {
const tabIndex = parseInt(vNode.attr('tabindex'), 10);
if (isFocusable(vNode) && (isNaN(tabIndex) || tabIndex > -1)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

Expand All @@ -17,19 +36,3 @@ function focusableDescendants(vNode) {
return focusableDescendants(child);
});
}

function frameFocusableContentEvaluate(node, options, virtualNode) {
if (!virtualNode.children) {
return undefined;
}

try {
return !virtualNode.children.some(child => {
return focusableDescendants(child);
});
} catch (e) {
return undefined;
}
}

export default frameFocusableContentEvaluate;
7 changes: 2 additions & 5 deletions lib/commons/dom/is-focusable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@ import AbstractVirtualNode from '../../core/base/virtual-node/abstract-virtual-n
import { getNodeFromTree } from '../../core/utils';

/**
* Determines if an element is focusable
* Determines if an element is keyboard or programmatically focusable.
* @method isFocusable
* @memberof axe.commons.dom
* @instance
* @param {HTMLElement} el The HTMLElement
* @return {Boolean} The element's focusability status
*/

function isFocusable(el) {
export default function isFocusable(el) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if maybe we should add some option here to say isFocusable(el, { noNegativeTabindex: true })?

This seem like sort of a common occurance.

Copy link
Contributor Author

@straker straker Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Though maybe the option could be called programmatically? Either way we should do it in a separate pr.

const focusable = isFocusable(el, { programmatically: false })

Copy link
Contributor Author

@straker straker Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #3500

const vNode = el instanceof AbstractVirtualNode ? el : getNodeFromTree(el);

if (vNode.props.nodeType !== 1) {
Expand All @@ -32,5 +31,3 @@ function isFocusable(el) {

return false;
}

export default isFocusable;
25 changes: 19 additions & 6 deletions test/checks/keyboard/frame-focusable-content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,32 +10,45 @@ describe('frame-focusable-content tests', function() {
});

it('should return true if element has no focusable content', function() {
var vNode = queryFixture('<button id="target"><span>Hello</span></button>');
var vNode = queryFixture('<div id="target"><span>Hello</span></div>');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you have to change these?

Copy link
Contributor Author

@straker straker Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having the frame-focusable-content test have all the elements wrapped in buttons was strange so changed them to divs

The <button><a></a></button> setup was the one that was the most strange

assert.isTrue(frameFocusableContent(null, null, vNode));
});

it('should return true if element is empty', function() {
var vNode = queryFixture('<button id="target"></button>');
var vNode = queryFixture('<div id="target"></div>');
assert.isTrue(frameFocusableContent(null, null, vNode));
});

it('should return true if element only has text content', function() {
var vNode = queryFixture('<button id="target">Hello</button>');
var vNode = queryFixture('<div id="target">Hello</div>');
assert.isTrue(frameFocusableContent(null, null, vNode));
});

it('should return false if element has focusable content', function() {
var vNode = queryFixture(
'<button id="target"><span tabindex="0">Hello</span></button>'
'<div id="target"><span tabindex="0">Hello</span></div>'
);
assert.isFalse(frameFocusableContent(null, null, vNode));
});

it('should return false if element has natively focusable content', function() {
var vNode = queryFixture(
'<button id="target"><a href="foo.html">Hello</a></button>'
'<div id="target"><a href="foo.html">Hello</a></div>'
);
assert.isFalse(frameFocusableContent(null, null, vNode));
});

it('should return true if element is natively focusable but has tabindex=-1', function() {
var vNode = queryFixture(
'<div id="target"><button tabindex="-1">Hello</button></div>'
);
assert.isTrue(frameFocusableContent(null, null, vNode));
});

it('should return false if element is natively focusable but has tabindex=0', function() {
var vNode = queryFixture(
'<div id="target"><button tabindex="0">Hello</button></div>'
);
assert.isFalse(frameFocusableContent(null, null, vNode));
});

});
5 changes: 5 additions & 0 deletions test/integration/rules/frame-focusable-content/frame-focusable-content.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
tabindex="-1"
id="pass1"
></iframe>
<iframe
src="/integration/rules/frame-focusable-content/frames/focusable-negative-tabindex.html"
tabindex="-1"
id="pass2"
></iframe>

<iframe
src="/integration/rules/frame-focusable-content/frames/focusable.html"
Expand Down
5 changes: 4 additions & 1 deletion test/integration/rules/frame-focusable-content/frame-focusable-content.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,8 @@
["#fail1", "#focusable"],
["#fail2", "#focusable"]
],
"passes": [["#pass1", "#not-focusable"]]
"passes": [
["#pass1", "#not-focusable"],
["#pass2", "#focusable"]
]
}
11 changes: 11 additions & 0 deletions test/integration/rules/frame-focusable-content/frames/focusable-negative-tabindex.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<html id="focusable">
<head>
<title>Hello</title>
<meta charset="utf8" />
<script src="/axe.js"></script>
</head>
<body>
<button tabindex="-1">Click</button>
</body>
</html>