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

Jump to bottom

feat(aria-label): deprecate Element arg; use virtualNode #1922

Merged
merged 3 commits into from
Dec 12, 2019
Merged

feat(aria-label): deprecate Element arg; use virtualNode #1922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec496ba
feat(aria-label): deprecate Element arg; use virtualNode
WilcoFiers Nov 28, 2019
1121533
chore: fix typo
WilcoFiers Dec 4, 2019
1b6fe5a
chore: use serial over virtual in tests
WilcoFiers Dec 11, 2019
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
2 changes: 1 addition & 1 deletion lib/checks/shared/aria-label.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
const { text, aria } = axe.commons;
return !!text.sanitize(aria.arialabelText(node));
return !!text.sanitize(aria.arialabelText(virtualNode));
11 changes: 7 additions & 4 deletions lib/commons/aria/arialabel-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@
/**
* Get the text value of aria-label, if any
*
* @deprecated Do not use Element directly. Pass VirtualNode instead
* @param {VirtualNode|Element} element
* @return {string} ARIA label
*/
aria.arialabelText = function arialabelText(node) {
node = node.actualNode || node;
if (node.nodeType !== 1) {
return '';
if (node instanceof axe.AbstractVirtualNode === false) {
if (node.nodeType !== 1) {
return '';
}
node = axe.utils.getNodeFromTree(node);
}
return node.getAttribute('aria-label') || '';
return node.attr('aria-label') || '';
};
2 changes: 2 additions & 0 deletions lib/core/base/virtual-node/serial-virtual-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,5 @@ function normaliseAttrs({ attributes = {} }) {
return attrs;
}, {});
}

axe.SerialVirtualNode = SerialVirtualNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to add this to the axe namespace. All our files are loaded directly into the test, so SerialVirtualNode should be available as a global, just like our Audit is in test/core/base/audit.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not correct. core and commons are in an isolated scope. Only files in test/core have access to the scope of src/core. Files in test/commons, tests/checks and tests/integrations don't have access. Same with src/commons. Only files in tests/commons have access to the src/commons scope.

Axe-core isn't just one big global scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I didn't know that. Other than for the test, would there be a reason to have 'VirtualNode' and 'SerialVirtualNode' on the public API?

2 changes: 2 additions & 0 deletions lib/core/base/virtual-node/virtual-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,5 @@ class VirtualNode extends axe.AbstractVirtualNode {
return this._cache.boundingClientRect;
}
}

axe.VirtualNode = VirtualNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto for not adding this to the axe namespace.

2 changes: 1 addition & 1 deletion lib/rules/label-content-name-mismatch-matches.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ if (!rolesWithNameFromContents.includes(role)) {
* if no `aria-label` or `aria-labelledby` attribute - ignore `node`
*/
if (
!text.sanitize(aria.arialabelText(node)) &&
!text.sanitize(aria.arialabelText(virtualNode)) &&
!text.sanitize(aria.arialabelledbyText(node))
) {
return false;
Expand Down
30 changes: 11 additions & 19 deletions test/checks/shared/aria-label.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,39 +2,31 @@ describe('aria-label', function() {
'use strict';

var fixture = document.getElementById('fixture');
var checkSetup = axe.testUtils.checkSetup;

afterEach(function() {
fixture.innerHTML = '';
});

it('should return true if an aria-label is present', function() {
var node = document.createElement('div');
node.setAttribute('aria-label', 'woohoo');
fixture.appendChild(node);

assert.isTrue(checks['aria-label'].evaluate(node));
var checkArgs = checkSetup('<div id="target" aria-label="woohoo"></div>');
assert.isTrue(checks['aria-label'].evaluate.apply(null, checkArgs));
});

it('should return false if an aria-label is not present', function() {
var node = document.createElement('div');
fixture.appendChild(node);

assert.isFalse(checks['aria-label'].evaluate(node));
var checkArgs = checkSetup('<div id="target"></div>');
assert.isFalse(checks['aria-label'].evaluate.apply(null, checkArgs));
});

it('should return false if an aria-label is present, but empty', function() {
var node = document.createElement('div');
node.setAttribute('aria-label', ' ');
fixture.appendChild(node);

assert.isFalse(checks['aria-label'].evaluate(node));
var checkArgs = checkSetup('<div id="target" aria-label=" "></div>');
assert.isFalse(checks['aria-label'].evaluate.apply(null, checkArgs));
});

it('should collapse whitespace', function() {
var node = document.createElement('div');
node.setAttribute('aria-label', ' \t \n \r \t \t\r\n ');
fixture.appendChild(node);

assert.isFalse(checks['aria-label'].evaluate(node));
var checkArgs = checkSetup(
'<div id="target" aria-label=" \t \n \r \t \t\r\n "></div>'
);
assert.isFalse(checks['aria-label'].evaluate.apply(null, checkArgs));
});
});
31 changes: 17 additions & 14 deletions test/commons/aria/arialabel-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,31 @@ describe('aria.arialabelText', function() {
var aria = axe.commons.aria;

it('returns "" if there is no aria-label', function() {
var node = document.createElement('div');
assert.equal(aria.arialabelText(node), '');
});

it('returns the aria-label attribute of nodes', function() {
var node = document.createElement('div');
var label = ' my label ';
node.setAttribute('aria-label', label);
assert.equal(aria.arialabelText(node), label);
var vNode = new axe.SerialVirtualNode({ nodeName: 'div' });
straker marked this conversation as resolved.
Show resolved Hide resolved
assert.equal(aria.arialabelText(vNode), '');
});

it('returns the aria-label attribute of virtual nodes', function() {
var node = document.createElement('div');
it('returns the aria-label attribute', function() {
var label = ' my label ';
node.setAttribute('aria-label', label);
var vNode = { actualNode: node };
var vNode = new axe.SerialVirtualNode({
nodeName: 'div',
attributes: { 'aria-label': label }
});
assert.equal(aria.arialabelText(vNode), label);
});

it('returns "" if there is no aria-label', function() {
var vNode = new axe.SerialVirtualNode({ nodeName: 'div' });
assert.equal(aria.arialabelText(vNode), '');
});

it('looks up the node in the flat tree', function() {
var label = 'harambe';
var node = document.createElement('div');
assert.equal(aria.arialabelText(node), '');
node.setAttribute('aria-label', label);

axe.utils.getFlattenedTree(node);
assert.equal(aria.arialabelText(node), label);
});

it('returns "" if the node is not an element', function() {
Expand Down