-
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
feat: Improve perf of axe.run [WWD-1821] #1503
Changes from 15 commits
f513ee8
fde7556
9aecd10
a50b91e
0324acb
b6adc56
6698717
4286459
3d1db89
d30dc6c
dc8c4df
c01046f
3e28ae9
f2ba02e
f4f116b
52b22f1
ee98db4
cae9e26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
node_modules/ | ||
node_modules/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' : ''); | ||
straker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// 9 === Node.DOCUMENT | ||
if (el.nodeType === 9) { | ||
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add 2 tests:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' || | ||
|
@@ -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; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
@@ -47,6 +48,8 @@ function virtualDOMfromNode(node, shadowId) { | |
return vNodeCache._tabbableElements; | ||
} | ||
}; | ||
axe._cache.nodeMap.set(node, vNode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
/** | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Create a test to show this is accessed from cache. |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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 && node._isHidden !== null) { | ||
return node._isHidden; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 || | ||
|
@@ -35,6 +40,14 @@ axe.utils.isHidden = function isHidden(el, recursed) { | |
} | ||
|
||
parent = el.assignedSlot ? el.assignedSlot : el.parentNode; | ||
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; | ||
}; |
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.
Can you add tests for
axe._cache.idRefs
?