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: Improve perf of axe.run [WWD-1821] #1503

Merged
merged 18 commits into from
Apr 30, 2019
Merged

feat: Improve perf of axe.run [WWD-1821] #1503

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f513ee8
feat(core): add WeakMap between HTMLElements and virtual nodes
straker Apr 9, 2019
fde7556
update to use new global cache
straker Apr 17, 2019
9aecd10
Merge branch 'develop' into improvePerf
straker Apr 17, 2019
a50b91e
clean up elsint errors
straker Apr 17, 2019
0324acb
remove redundant idRefsCached
straker Apr 17, 2019
b6adc56
use const
straker Apr 17, 2019
6698717
add deprecation comment to getNodeFromTree, move dep to dev, add test…
straker Apr 18, 2019
4286459
remove deprecation comment
straker Apr 22, 2019
3d1db89
Merge branch 'develop' into improvePerf
straker Apr 22, 2019
d30dc6c
run fmt
straker Apr 22, 2019
dc8c4df
fix declarations
straker Apr 25, 2019
c01046f
fix and run prettier
straker Apr 26, 2019
3e28ae9
Merge branch 'develop' into improvePerf
straker Apr 26, 2019
f2ba02e
fix the test that broke due to prietter formatting dom...
straker Apr 26, 2019
f4f116b
revert prettierignore
straker Apr 26, 2019
52b22f1
Merge branch 'develop' into improvePerf
straker Apr 29, 2019
ee98db4
fix broken test
straker Apr 29, 2019
cae9e26
fix another breaking test
straker Apr 29, 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
71 changes: 27 additions & 44 deletions doc/examples/mocha/test/test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,50 +1,33 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<title>Example Mocha Test</title>
<link
rel="stylesheet"
href="../node_modules/mocha/mocha.css"
type="text/css"
charset="utf-8"
/>
</head>
<body>
<!-- Required for browser reporter -->
<div id="mocha"></div>
<head>
<meta charset="utf-8">
<title>Example Mocha Test</title>
<link rel="stylesheet" href="../node_modules/mocha/mocha.css" type="text/css" charset="utf-8" />
</head>
<body>
<!-- Required for browser reporter -->
<div id="mocha"></div>

<script
src="../node_modules/mocha/mocha.js"
type="text/javascript"
charset="utf-8"
></script>
<script
src="../node_modules/chai/chai.js"
type="text/javascript"
charset="utf-8"
></script>
<script
src="../node_modules/axe-core/axe.min.js"
type="text/javascript"
charset="utf-8"
></script>
<script type="text/javascript" charset="utf-8">
mocha.setup('bdd');
var expect = chai.expect;
</script>
<script src="../node_modules/mocha/mocha.js" type="text/javascript" charset="utf-8"></script>
<script src="../node_modules/chai/chai.js" type="text/javascript" charset="utf-8"></script>
<script src="../node_modules/axe-core/axe.min.js" type="text/javascript" charset="utf-8"></script>
<script type="text/javascript" charset="utf-8">
mocha.setup('bdd');
var expect = chai.expect;
</script>

<!-- Include axe -->
<!-- Include axe -->

<!-- Spec files -->
<script src="a11y.js" type="text/javascript" charset="utf-8"></script>
<div id="working">
<label for="labelfld">Label for this text field.</label>
<input type="text" id="labelfld" />
</div>
<div id="broken">
<p>Label for this text field.</p>
<input type="text" id="nolabelfld" />
</div>
</body>
<!-- Spec files -->
<script src="a11y.js" type="text/javascript" charset="utf-8"></script>
<div id="working">
<label for="labelfld">Label for this text field.</label>
<input type="text" id="labelfld">
</div>
<div id="broken">
<p>Label for this text field.</p>
<input type="text" id="nolabelfld">
</div>
</body>
</html>
50 changes: 24 additions & 26 deletions doc/examples/qunit/test/test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,27 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<title>Basic Test Suite</title>
<!-- Load local QUnit. -->
<link
rel="stylesheet"
href="../node_modules/qunitjs/qunit/qunit.css"
media="screen"
/>
<script src="../node_modules/qunitjs/qunit/qunit.js"></script>
<!-- Load local lib and tests. -->
<script src="../node_modules/axe-core/axe.min.js"></script>
<script src="a11y.js"></script>
</head>
<body>
<div id="qunit"></div>
<div id="qunit-fixture"></div>
<div id="working">
<label for="labelfld">Label for this text field.</label>
<input type="text" id="labelfld" />
</div>
<div id="broken">
<p>Label for this text field.</p>
<input type="text" id="nolabelfld" />
</div>
</body>
<head>
<meta charset="utf-8">
<title>Basic Test Suite</title>
<!-- Load local QUnit. -->
<link rel="stylesheet" href="../node_modules/qunitjs/qunit/qunit.css" media="screen">
<script src="../node_modules/qunitjs/qunit/qunit.js"></script>
<!-- Load local lib and tests. -->
<script src="../node_modules/axe-core/axe.min.js"></script>
<script src="a11y.js"></script>
</head>
<body>
<div id="qunit"></div>
<div id="qunit-fixture">
</div>
<div id="working">
<label for="labelfld">Label for this text field.</label>
<input type="text" id="labelfld">
</div>
<div id="broken">
<p>Label for this text field.</p>
<input type="text" id="nolabelfld">
</div>

</body>
</html>
2 changes: 1 addition & 1 deletion lib/checks/aria/required-children.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ function ariaOwns(nodes, role) {
if (nodes[index] === null) {
continue;
}
let virtualTree = axe.utils.getNodeFromTree(axe._tree[0], nodes[index]);
let virtualTree = axe.utils.getNodeFromTree(nodes[index]);
straker marked this conversation as resolved.
Show resolved Hide resolved
if (owns(nodes[index], virtualTree, role, true)) {
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/checks/aria/required-parent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ var owners = getAriaOwners(node);
if (owners) {
for (var i = 0, l = owners.length; i < l; i++) {
missingParents = getMissingContext(
axe.utils.getNodeFromTree(axe._tree[0], owners[i]),
axe.utils.getNodeFromTree(owners[i]),
missingParents,
true
);
Expand Down
2 changes: 1 addition & 1 deletion lib/commons/aria/get-owned-virtual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ aria.getOwnedVirtual = function getOwned({ actualNode, children }) {

return dom.idrefs(actualNode, 'aria-owns').reduce((ownedElms, element) => {
if (element) {
const virtualNode = axe.utils.getNodeFromTree(axe._tree[0], element);
const virtualNode = axe.utils.getNodeFromTree(element);
ownedElms.push(virtualNode);
}
return ownedElms;
Expand Down
67 changes: 31 additions & 36 deletions lib/commons/aria/is-accessible-ref.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
/* global aria, axe, dom */
function findDomNode(node, functor) {
if (functor(node)) {
return node;
const idRefsRegex = /^idrefs?$/;

function cacheIdRefs(node, refAttrs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add tests for axe._cache.idRefs?

if (node.hasAttribute) {
if (node.nodeName.toUpperCase() === 'LABEL' && node.hasAttribute('for')) {
axe._cache.idRefs[node.getAttribute('for')] = 1;
straker marked this conversation as resolved.
Show resolved Hide resolved
}

refAttrs
.filter(attr => node.hasAttribute(attr))
jeeyyy marked this conversation as resolved.
Show resolved Hide resolved
.forEach(attr => {
const attrValue = node.getAttribute(attr);
axe.utils.tokenList(attrValue).forEach(id => {
axe._cache.idRefs[id] = 1;
straker marked this conversation as resolved.
Show resolved Hide resolved
});
});
}

for (let i = 0; i < node.children.length; i++) {
const out = findDomNode(node.children[i], functor);
if (out) {
return out;
}
cacheIdRefs(node.children[i], refAttrs);
}
}

Expand All @@ -22,34 +33,18 @@ aria.isAccessibleRef = function isAccessibleRef(node) {
root = root.documentElement || root; // account for shadow roots
const id = node.id;

// Get all idref(s) attributes on the lookup table
const refAttrs = Object.keys(aria.lookupTable.attributes).filter(attr => {
const { type } = aria.lookupTable.attributes[attr];
return /^idrefs?$/.test(type);
});
// because axe.commons is not available in axe.utils, we can't do
// this caching when we build up the virtual tree
if (!axe._cache.idRefs) {
axe._cache.idRefs = {};
// Get all idref(s) attributes on the lookup table
const refAttrs = Object.keys(aria.lookupTable.attributes).filter(attr => {
const { type } = aria.lookupTable.attributes[attr];
return idRefsRegex.test(type);
});

// Find the first element that IDREF(S) the node
let refElm = findDomNode(root, elm => {
if (elm.nodeType !== 1) {
// Elements only
return;
}
if (
elm.nodeName.toUpperCase() === 'LABEL' &&
elm.getAttribute('for') === id
) {
return true;
}
// See if there are any aria attributes that reference the node
return refAttrs
.filter(attr => elm.hasAttribute(attr))
.some(attr => {
const attrValue = elm.getAttribute(attr);
if (aria.lookupTable.attributes[attr].type === 'idref') {
return attrValue === id;
}
return axe.utils.tokenList(attrValue).includes(id);
});
});
return typeof refElm !== 'undefined';
cacheIdRefs(root, refAttrs);
}

return axe._cache.idRefs[id] === 1;
};
4 changes: 2 additions & 2 deletions lib/commons/aria/label-virtual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ aria.labelVirtual = function({ actualNode }) {
ref = dom.idrefs(actualNode, 'aria-labelledby');
candidate = ref
.map(function(thing) {
const vNode = axe.utils.getNodeFromTree(axe._tree[0], thing);
const vNode = axe.utils.getNodeFromTree(thing);
return vNode ? text.visibleVirtual(vNode, true) : '';
})
.join(' ')
Expand Down Expand Up @@ -49,6 +49,6 @@ aria.labelVirtual = function({ actualNode }) {
* @return {Mixed} String of visible text, or `null` if no label is found
*/
aria.label = function(node) {
node = axe.utils.getNodeFromTree(axe._tree[0], node);
node = axe.utils.getNodeFromTree(node);
return aria.labelVirtual(node);
};
5 changes: 1 addition & 4 deletions lib/commons/dom/find-up.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@
* @return {HTMLElement|null} Either the matching HTMLElement or `null` if there was no match
*/
dom.findUp = function(element, target) {
return dom.findUpVirtual(
axe.utils.getNodeFromTree(axe._tree[0], element),
target
);
return dom.findUpVirtual(axe.utils.getNodeFromTree(element), target);
};

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/commons/dom/has-content-virtual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ dom.hasContentVirtual = function(elm, noRecursion) {
* @return {Boolean}
*/
dom.hasContent = function hasContent(elm, noRecursion) {
elm = axe.utils.getNodeFromTree(axe._tree[0], elm);
elm = axe.utils.getNodeFromTree(elm);
return dom.hasContentVirtual(elm, noRecursion);
};

Expand Down
2 changes: 1 addition & 1 deletion lib/commons/dom/is-in-text-block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ function getBlockParent(node) {
while (parentBlock && !isBlock(parentBlock)) {
parentBlock = dom.getComposedParent(parentBlock);
}
return axe.utils.getNodeFromTree(axe._tree[0], parentBlock);
return axe.utils.getNodeFromTree(parentBlock);
}

/**
Expand Down
18 changes: 15 additions & 3 deletions lib/commons/dom/is-visible.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ function isClipped(clip) {
*/
dom.isVisible = function(el, screenReader, recursed) {
'use strict';
var style, nodeName, parent;
const node = axe.utils.getNodeFromTree(el);
const cacheName = 'isVisible' + (screenReader ? 'ScreenReader' : '');
straker marked this conversation as resolved.
Show resolved Hide resolved
let style, nodeName, parent, isVisible;
straker marked this conversation as resolved.
Show resolved Hide resolved

// 9 === Node.DOCUMENT
if (el.nodeType === 9) {
Expand All @@ -45,6 +47,10 @@ dom.isVisible = function(el, screenReader, recursed) {
el = el.host; // grab the host Node
}

if (node && typeof node[cacheName] !== 'undefined') {
return node[cacheName];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add 2 tests:

  1. that the cache is set after first accessing this method
  2. that the cache is used when accessing this a second time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to use this comment to group all the tests for caching comments.

We already have code that tests that all of these functions are working as expected. Is there a particular reason you would like the unit tests to be tied to how the function was implemented internally?

One of the nice things about these drastic code changes (especially for getNodeFromTree) is that I could completely swap out the internal implementation and I didn't have to update any of the tests. If we start adding tests that make sure a node is added to the cache, or retrieved from the cache, or that private cache properties are added, then we tie a lot of tests to the implementation of the internal code. We also force changes to that implementation to require changes to the unit tests.

IMO, we shouldn't test the internals of a function as the internals can easily change, creating fragile tests. We should only test that the function returns an expected set of outputs for a given set of inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough :)

}

style = window.getComputedStyle(el, null);
if (style === null) {
return false;
Expand All @@ -68,8 +74,14 @@ dom.isVisible = function(el, screenReader, recursed) {

parent = el.assignedSlot ? el.assignedSlot : el.parentNode;
if (parent) {
return dom.isVisible(parent, screenReader, true);
isVisible = dom.isVisible(parent, screenReader, true);
} else {
isVisible = false;
}

return false;
if (node) {
node[cacheName] = isVisible;
}

return isVisible;
};
2 changes: 1 addition & 1 deletion lib/commons/text/accessible-text-virtual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* @return {string}
*/
text.accessibleText = function accessibleText(element, context) {
let virtualNode = axe.utils.getNodeFromTree(axe._tree[0], element); // throws an exception on purpose if axe._tree not correct
let virtualNode = axe.utils.getNodeFromTree(element); // throws an exception on purpose if axe._tree not correct
straker marked this conversation as resolved.
Show resolved Hide resolved
return text.accessibleTextVirtual(virtualNode, context);
};

Expand Down
2 changes: 1 addition & 1 deletion lib/commons/text/label-virtual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@ text.labelVirtual = function(node) {
* @return {Mixed} String of visible text, or `null` if no label is found
*/
text.label = function(node) {
node = axe.utils.getNodeFromTree(axe._tree[0], node);
node = axe.utils.getNodeFromTree(node);
return text.labelVirtual(node);
};
2 changes: 1 addition & 1 deletion lib/commons/text/visible-virtual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ text.visibleVirtual = function(element, screenReader, noRecursing) {
* @return {String}
*/
text.visible = function(element, screenReader, noRecursing) {
element = axe.utils.getNodeFromTree(axe._tree[0], element);
element = axe.utils.getNodeFromTree(element);
return text.visibleVirtual(element, screenReader, noRecursing);
};
6 changes: 3 additions & 3 deletions lib/core/base/context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ function parseSelectorArray(context, type) {
//eslint no-loop-func:0
result = result.concat(
nodeList.map(node => {
return axe.utils.getNodeFromTree(context.flatTree[0], node);
return axe.utils.getNodeFromTree(node);
})
);
break;
Expand All @@ -146,15 +146,15 @@ function parseSelectorArray(context, type) {
//eslint no-loop-func:0
result = result.concat(
nodeList.map(node => {
return axe.utils.getNodeFromTree(context.flatTree[0], node);
return axe.utils.getNodeFromTree(node);
})
);
}
} else if (item instanceof Node) {
if (item.documentElement instanceof Node) {
result.push(context.flatTree[0]);
} else {
result.push(axe.utils.getNodeFromTree(context.flatTree[0], item));
result.push(axe.utils.getNodeFromTree(item));
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions lib/core/imports/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ if (!('Promise' in window)) {
require('es6-promise').polyfill();
}

/**
* Polyfill `WeakMap`
* Reference: https://github.com/polygonplanet/weakmap-polyfill
*/
require('weakmap-polyfill');
scurker marked this conversation as resolved.
Show resolved Hide resolved

/**
* Namespace `axe.imports` which holds required external dependencies
*
Expand Down
Loading