-
Notifications
You must be signed in to change notification settings - Fork 776
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
fix(performance): significantly improve the performance of the dom.fi… #699
Conversation
5d3104e
to
8643930
Compare
@WilcoFiers @marcysutton I think this is ready for merging now |
lib/commons/dom/find-up.js
Outdated
/** | ||
* recusively walk up the DOM, checking for a node which matches a selector | ||
* | ||
* **WARNING:** this should be used sparingly, as it's not even close to being performant | ||
* @method findUp | ||
* @memberof axe.commons.dom | ||
* @instance | ||
* @param {HTMLElement|String} element The starting HTMLElement | ||
* @param {HTMLElement|VirtualNode} element The starting HTMLElement or its virtualNode equivalent | ||
* @param {String} target The selector for the HTMLElement | ||
* @return {HTMLElement|null} Either the matching HTMLElement or `null` if there was no match | ||
*/ | ||
dom.findUp = function (element, target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should be renamed to findUpVirtual
so that it follows the convention we set with other virtual methods. Otherwise this breaks compatibility with custom rules.
lib/commons/dom/find-up.js
Outdated
if (parent && parent.nodeType === 11) { | ||
matches = null; | ||
parent = parent.host; | ||
if (typeof element.actualNode === 'undefined') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do this in a separate findUp
method that calls findUpVirtual
.
lib/commons/dom/find-up.js
Outdated
@@ -1,34 +1,61 @@ | |||
/* global dom, axe */ | |||
dom.oldFindUp = function (element, target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this method if you use axe.utils.matchesSelector
instead of elm.matches
. This seems like the absolute worst way to do this... Absolutely no reason so rerun querySelectorAll. No wonder this is slow.
lib/commons/dom/find-up.js
Outdated
element = axe.utils.getNodeFromTree(axe._tree[0], element); | ||
} | ||
parent = element.actualNode; | ||
if (!element.shadowId && typeof element.actualNode.closest === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in assuming that any virtual node with a truthy shadowId is somewhere inside a shadow tree, either because it is a shadow element or because it's slotted somewhere? Please add a comment to clear that up.
lib/commons/dom/find-up.js
Outdated
return null; | ||
} | ||
// handle shadow DOM nodes and older browsers | ||
if (typeof parent.matches === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use axe.utils.matchesSelector
, it works in each browser we want to support. That way you don't need to keep the old method around at all.
lib/commons/dom/find-up.js
Outdated
// handle shadow DOM nodes and older browsers | ||
if (typeof parent.matches === 'function') { | ||
do {// recursively walk up the DOM, checking each parent node | ||
parent = (parent.assignedSlot ? parent.assignedSlot : parent.parentNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to use dom.getComposedParent
I think the way you've done this now, you could actually match the contents
element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we don't want to use getComposedParent because a rule may have to look for the <slot>
itself (for example)
Not sure I understand what you mean by "match the contents
element"?
})[0]; | ||
} else { | ||
label = dom.findUp(actualNode, 'label'); | ||
label = dom.findUp(virtualNode, 'label'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should all call findUpVirtual
.
…ndUp utility fixes #696
I also added a documentation section on performance |
|
||
Certain rules (like the color-contrast rule) look at almost every element on a page and some of these rules also perform somewhat expensive operations on these elements including looking up the hierarchy, looking at overlapping elements, calculating the computed styles etc. It also calculates a unique selector for each element in the results and also de-duplicates elements so that you do not get duplicate items in your results. | ||
|
||
If your page is very large (in terms of the number of Elements on the page) i.e. >50K elements on the page, then you will see analysis times that run over 10s on a relatively decent CPU. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also add a note here on turning up the iframe timeout option with very large iframes.
@@ -19,14 +19,14 @@ var phrasingElements = ['A', 'EM', 'STRONG', 'SMALL', 'MARK', 'ABBR', 'DFN', 'I' | |||
* @param {HTMLElement} element The HTMLElement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These docs look out-of-date.
…ke VirtualNode instances
…ndUp utility
Fixes #696