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

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

Merged
merged 18 commits into from
Apr 30, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion lib/checks/aria/required-children.js
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]);
const virtualTree = axe.utils.getNodeFromTree(nodes[index]);
if (owns(nodes[index], virtualTree, role, true)) {
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/checks/aria/required-parent.js
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
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
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')] = true;
}

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] = true;
});
});
}

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] === true;
};
4 changes: 2 additions & 2 deletions lib/commons/aria/label-virtual.js
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
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
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
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
22 changes: 16 additions & 6 deletions lib/commons/dom/is-visible.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ 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' : '');

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

style = window.getComputedStyle(el, null);
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 :)

}

const style = window.getComputedStyle(el, null);
if (style === null) {
return false;
}

nodeName = el.nodeName.toUpperCase();
const nodeName = el.nodeName.toUpperCase();

if (
style.getPropertyValue('display') === 'none' ||
Expand All @@ -66,10 +71,15 @@ dom.isVisible = function(el, screenReader, recursed) {
return false;
}

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

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

return isVisible;
};
2 changes: 1 addition & 1 deletion lib/commons/text/accessible-text-virtual.js
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
const virtualNode = axe.utils.getNodeFromTree(element); // throws an exception on purpose if axe._tree not correct
return text.accessibleTextVirtual(virtualNode, context);
};

Expand Down
2 changes: 1 addition & 1 deletion lib/commons/text/label-virtual.js
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
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
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
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
5 changes: 5 additions & 0 deletions lib/core/public/run-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

// Clean up after resolve / reject
function cleanup() {
axe._cache = undefined;
axe._tree = undefined;
axe._selectorData = undefined;
}
Expand All @@ -18,6 +19,10 @@ function cleanup() {
function runRules(context, options, resolve, reject) {
'use strict';
try {
axe._cache = {
nodeMap: new WeakMap()
};

context = new Context(context);
axe._tree = context.flatTree;
axe._selectorData = axe.utils.getSelectorData(context.flatTree);
Expand Down
25 changes: 7 additions & 18 deletions lib/core/utils/flattened-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ var axe = axe || { utils: {} };
*/
function virtualDOMfromNode(node, shadowId) {
const vNodeCache = {};
return {
const vNode = {
shadowId: shadowId,
children: [],
actualNode: node,
_isHidden: null, // will be populated by axe.utils.isHidden
jeeyyy marked this conversation as resolved.
Show resolved Hide resolved
get isFocusable() {
if (!vNodeCache._isFocusable) {
vNodeCache._isFocusable = axe.commons.dom.isFocusable(node);
Expand All @@ -47,6 +48,8 @@ function virtualDOMfromNode(node, shadowId) {
return vNodeCache._tabbableElements;
}
};
axe._cache.nodeMap.set(node, vNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Verify in a test that all virtual nodes are put in cache.

return vNode;
}

/**
Expand Down Expand Up @@ -144,26 +147,12 @@ axe.utils.getFlattenedTree = function(node, shadowId) {
};

/**
* Recursively return a single node from a virtual dom tree
* Return a single node from the virtual dom tree
*
* @param {Object} vNode The flattened, virtual DOM tree
* @param {Node} node The HTML DOM node
*/
axe.utils.getNodeFromTree = function(vNode, node) {
var found;

if (vNode.actualNode === node) {
return vNode;
}
vNode.children.forEach(candidate => {
if (found) {
return;
}
if (candidate.actualNode === node) {
found = candidate;
} else {
found = axe.utils.getNodeFromTree(candidate, node);
}
});
return found;
const el = node || vNode;
return axe._cache.nodeMap.get(el);
Copy link
Contributor

Choose a reason for hiding this comment

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

Create a test to show this is accessed from cache.

};
20 changes: 16 additions & 4 deletions lib/core/utils/is-hidden.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/
axe.utils.isHidden = function isHidden(el, recursed) {
'use strict';
var parent;
const node = axe.utils.getNodeFromTree(el);

// 9 === Node.DOCUMENT
if (el.nodeType === 9) {
Expand All @@ -20,7 +20,11 @@ axe.utils.isHidden = function isHidden(el, recursed) {
el = el.host; // grab the host Node
}

var style = window.getComputedStyle(el, null);
if (node && node._isHidden !== null) {
return node._isHidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

As before, please add tests for the caching.

}

const style = window.getComputedStyle(el, null);

if (
!style ||
Expand All @@ -34,7 +38,15 @@ axe.utils.isHidden = function isHidden(el, recursed) {
return true;
}

parent = el.assignedSlot ? el.assignedSlot : el.parentNode;
const parent = el.assignedSlot ? el.assignedSlot : el.parentNode;
const isHidden = axe.utils.isHidden(parent, true);

// cache the results of the isHidden check on the parent tree
// so we don't have to look at the parent tree again for all its
// descendants
if (node) {
node._isHidden = isHidden;
}

return axe.utils.isHidden(parent, true);
return isHidden;
};
5 changes: 1 addition & 4 deletions lib/rules/color-contrast-matches.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@ if (nodeName === 'LABEL' || nodeParentLabel) {
if (nodeParentLabel) {
relevantNode = nodeParentLabel;
// we need an input candidate from a parent to account for label children
relevantVirtualNode = axe.utils.getNodeFromTree(
axe._tree[0],
nodeParentLabel
);
relevantVirtualNode = axe.utils.getNodeFromTree(nodeParentLabel);
}
// explicit label of disabled input
let doc = axe.commons.dom.getRootNode(relevantNode);
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@
"sri-toolbox": "^0.2.0",
"standard-version": "^5.0.0",
"typescript": "^2.9.2",
"uglify-js": "^3.4.4"
"uglify-js": "^3.4.4",
"weakmap-polyfill": "^2.0.0"
straker marked this conversation as resolved.
Show resolved Hide resolved
},
"dependencies": {},
"lint-staged": {
Expand Down
Loading