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 4 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
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.idRefsCached) {
straker marked this conversation as resolved.
Show resolved Hide resolved
axe._cache.idRefsCached = true;
// 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);
};
2 changes: 1 addition & 1 deletion lib/commons/dom/find-up.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*/
dom.findUp = function(element, target) {
return dom.findUpVirtual(
axe.utils.getNodeFromTree(axe._tree[0], element),
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
7 changes: 7 additions & 0 deletions lib/core/public/run-rules.js
View file
Open in desktop
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,12 @@ function cleanup() {
function runRules(context, options, resolve, reject) {
'use strict';
try {
axe._cache = {
nodeMap: new WeakMap(),
idRefs: {},
idRefsCached: false
};

context = new Context(context);
axe._tree = context.flatTree;
axe._selectorData = axe.utils.getSelectorData(context.flatTree);
Expand Down
28 changes: 9 additions & 19 deletions lib/core/utils/flattened-tree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ var axe = axe || { utils: {} };
*/
function virtualDOMfromNode(node, shadowId) {
const vNodeCache = {};
return {
const vNode = {
shadowId: shadowId,
children: [],
actualNode: node,
Expand All @@ -47,6 +47,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 +146,14 @@ 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 {Object} vNode The flattened, virtual DOM tree (optional)
* @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;
// TODO: removing vNode is a breaking change, but we can skirt that
// for now by ignoring it if passed in
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it isn't better to keep getNodeFromTree as is and deprecate it, and have a replacement method that uses cache? Would like to discuss.

Copy link
Member

Choose a reason for hiding this comment

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

👎 from me. I see this as an opportunity for people (us) to continue using the "wrong method". Additionally, as discussed yesterday, there are 3 different methods for figuring out if something is "hidden". Let's not create the same problem here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change getNodeFromTree is something that rules can use. Why is this change necessary?

let 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.

};
16 changes: 13 additions & 3 deletions lib/core/utils/is-hidden.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
*/
axe.utils.isHidden = function isHidden(el, recursed) {
'use strict';
var parent;
const node = axe.utils.getNodeFromTree(el);
let parent, isHidden;
straker marked this conversation as resolved.
Show resolved Hide resolved

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

var style = window.getComputedStyle(el, null);
if (node && typeof node.isHidden !== 'undefined') {
return node.isHidden;
}

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

if (
!style ||
Expand All @@ -35,6 +40,11 @@ axe.utils.isHidden = function isHidden(el, recursed) {
}

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

if (node) {
node.isHidden = isHidden;
straker marked this conversation as resolved.
Show resolved Hide resolved
}

return axe.utils.isHidden(parent, true);
return isHidden;
};
5 changes: 1 addition & 4 deletions lib/rules/color-contrast-matches.js
View file
Open in desktop
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
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@
"typescript": "^2.9.2",
"uglify-js": "^3.4.4"
},
"dependencies": {},
"dependencies": {
"weakmap-polyfill": "^2.0.0"
straker marked this conversation as resolved.
Show resolved Hide resolved
},
"lint-staged": {
"*.{md,json,ts}": [
"prettier --write",
Expand Down
Loading