-
Notifications
You must be signed in to change notification settings - Fork 779
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(preload): cssom assets #958
Changes from 45 commits
321f4f1
33205ec
5e4f463
a3ea7c1
7119f2d
3782006
508c795
b78ac30
4e590a5
18b87e3
771bd88
eac06ac
cad3bb6
d9ab58f
858710f
678da39
f763dec
1b847ea
50229a0
8426fab
46ae47c
b03981b
0ba0ef9
ba00d6b
4fa9d6a
2074cc9
9f57cdb
f18db7f
1434751
b64309c
5ea88d5
28bdcdd
87820da
ae59586
15d71f1
3a10d63
222f05b
7f061ae
0fca2d1
2155df8
0923c36
29e3c61
9ecba9e
0b9d62d
f06701b
20e6f7d
06f6dfc
9e28b8a
483bcd8
a3ecee7
3436cc0
12008e7
8da721e
30a0819
027b3a1
567ef3c
b5c3f4f
a8c74ad
63cc5b4
247a7cb
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 |
---|---|---|
|
@@ -8,4 +8,4 @@ | |
"fail": "aria-hidden=true should not be present on the document body" | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
/** | ||
* Returns a then(able) queue of CSSStyleSheet(s) | ||
* @param {Object} ownerDocument document object to be inspected for stylesheets | ||
* @param {number} timeout on network request for stylesheet that need to be externally fetched | ||
* @param {Function} getSheetFromTextFn a utility function to generate a style sheet from text | ||
* @return {Object} queue | ||
* @private | ||
*/ | ||
function loadCssom(ownerDocument, timeout, getSheetFromTextFn) { | ||
const q = axe.utils.queue(); | ||
|
||
Array.from(ownerDocument.styleSheets).forEach(sheet => { | ||
if (sheet.disabled) { | ||
return; | ||
} | ||
|
||
try { | ||
if (sheet.cssRules) { | ||
// accessing .cssRules throws for external (cross-domain) sheets, which is handled in the catch | ||
sheet.isExternal = false; // isExternal is a flag to ascertain if a network request is necessary for cssom building | ||
q.defer(resolve => resolve(sheet)); | ||
} | ||
} catch (e) { | ||
// external sheet -> make an xhr and q the response | ||
q.defer((resolve, reject) => { | ||
axe.imports | ||
.axios({ | ||
method: 'get', | ||
url: sheet.href, | ||
timeout | ||
}) | ||
.then(({ data }) => { | ||
const sheet = getSheetFromTextFn(data, true); //second argument acts as > isExternal - true | ||
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. That second argument is useful. It's always external. More importantly, 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. Solved! |
||
resolve(sheet); | ||
}) | ||
.catch(reject); | ||
}); | ||
} | ||
}); | ||
|
||
return q; | ||
} | ||
|
||
/** | ||
* Returns an array of documents with a given root node/ tree | ||
* @param {Object} treeRoot - the DOM tree to be inspected | ||
* @return {Array} documents | ||
* @private | ||
*/ | ||
function getDocumentsFromTreeRoot(treeRoot) { | ||
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. Should we make this available on 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. Leaving it here for now, can migrate if other cssom rules need this, I don't think so, because preload should be the only place to construct this data. |
||
let ids = []; | ||
const documents = axe.utils | ||
.querySelectorAllFilter(treeRoot, '*', node => { | ||
if (ids.includes(node.shadowId)) { | ||
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. since shadowId can be undefined, might be safer if we test that explicitly, so 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 understand the thinking here, but node.shadowId is undefined in many cases, and this returns false correctly as expected. Adding a checked if null or undefined, is not essential here in my opinion. Can chat about this if necessary. 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. Fine, but you owe me a beer when we get this reported. 😉 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. That is cool by me 🍺 |
||
return false; | ||
} | ||
ids.push(node.shadowId); | ||
return true; | ||
}) | ||
.map(node => { | ||
return node.actualNode.ownerDocument; | ||
}); | ||
return documents; | ||
} | ||
|
||
/** | ||
* @method preloadCssom | ||
* @memberof axe.utils | ||
* @instance | ||
* @param {Object} object argument which is a composite object, with attributes asset, timeout, treeRoot(optional), resolve & reject | ||
* asset - type of asset being loaded, in this case cssom | ||
* timeout - timeout for any network calls made | ||
* treeRoot - the DOM tree to be inspected | ||
* resolve/ reject - promise chainable methods | ||
* @return {Object} | ||
*/ | ||
function preloadCssom({ timeout, treeRoot = axe._tree[0] }) { | ||
const documents = getDocumentsFromTreeRoot(treeRoot); | ||
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. If we're not doing shadow DOM in this PR, than we need to replace this expression. Otherwise we're just committing buggy shadow DOM code to develop, rather than not having it supported. 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 have just solved for shadowDOM, and it would make sense to fold all of that into the same PR. So yes, shadowDOM is in the same PR. |
||
const q = axe.utils.queue(); | ||
|
||
if (!documents.length) { | ||
return q; | ||
} | ||
|
||
const dynamicDoc = document.implementation.createHTMLDocument(); | ||
function getSheetFromTextFn(cssText, isExternal) { | ||
// create style node with css text | ||
const style = dynamicDoc.createElement('style'); | ||
style.type = 'text/css'; | ||
style.appendChild(dynamicDoc.createTextNode(cssText)); | ||
// added style to temporary document | ||
dynamicDoc.head.appendChild(style); | ||
// add attribute to differentiate if it is a fetched sheet | ||
style.sheet.isExternal = isExternal; | ||
return style.sheet; | ||
} | ||
|
||
documents.forEach(doc => { | ||
q.defer((resolve, reject) => { | ||
loadCssom(doc, timeout, getSheetFromTextFn) | ||
.then(resolve) | ||
.catch(reject); | ||
}); | ||
}); | ||
|
||
return q; | ||
} | ||
axe.utils.preloadCssom = preloadCssom; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
/** | ||
* Validated the preload object | ||
* @param {Object | boolean} preload configuration object or boolean passed via the options parameter to axe.run | ||
* @return {boolean} | ||
* @private | ||
*/ | ||
function isPreloadValidObject(preload) { | ||
return ( | ||
typeof preload === 'object' && | ||
preload.hasOwnProperty('assets') && | ||
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 remove the |
||
Array.isArray(preload.assets) | ||
); | ||
} | ||
|
||
/** | ||
* Returns a boolean which decides if preload is configured | ||
* @param {Object} options run configuration options (or defaults) passed via axe.run | ||
* @return {boolean} | ||
*/ | ||
function shouldPreload(options) { | ||
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. It looks like this entire function could be simplified into a few LOC: const shouldPreload = options => {
return options && isValidPreloadObject(options.preload)
} 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. Nah, keeping this function as preload can be configured in various ways & this function keeps it clear to check. No harm. Eg: 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. Yeah, this is really overdoing the avoid complex logic branches thing. I agree with Stephen, this his solution is much more readable. |
||
if (!options) { | ||
return false; | ||
} | ||
|
||
if (!options.preload) { | ||
return false; | ||
} | ||
|
||
if (typeof options.preload === 'boolean') { | ||
return options.preload; | ||
} | ||
|
||
if (isPreloadValidObject(options.preload)) { | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
axe.utils.shouldPreload = shouldPreload; | ||
|
||
/** | ||
* Constructs a configuration object representing the preload requested assets & timeout | ||
* @param {Object} options run configuration options (or defaults) passed via axe.run | ||
* @return {Object} | ||
*/ | ||
function getPreloadConfig(options) { | ||
// default fallback configuration | ||
const config = { | ||
assets: axe.constants.preloadAssets, | ||
timeout: axe.constants.preloadAssetsTimeout | ||
}; | ||
|
||
// if type is boolean | ||
if (typeof options.preload === 'boolean') { | ||
return config; | ||
} | ||
|
||
// if type is object - ensure an array of assets to load is specified | ||
if (!isPreloadValidObject(options.preload)) { | ||
throw new Error( | ||
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. This code will never run because if it isn't, your |
||
'No assets configured for preload in aXe run configuration' | ||
); | ||
} | ||
|
||
// check if requested assets to preload are valid items | ||
const areRequestedAssetsValid = options.preload.assets.every(a => | ||
axe.constants.preloadAssets.includes(a.toLowerCase()) | ||
); | ||
|
||
if (!areRequestedAssetsValid) { | ||
throw new Error( | ||
`Requested assets, not supported by aXe.` + | ||
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. IIRC we're not capitalizing the "x" anymore. 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. to avoid |
||
`Supported assets are: ${axe.constants.preloadAssets.join(', ')}.` | ||
); | ||
} | ||
|
||
// unique assets to load, in case user had requested same asset type many times. | ||
config.assets = axe.utils.uniqueArray( | ||
options.preload.assets.map(a => a.toLowerCase()), | ||
[] | ||
); | ||
|
||
if ( | ||
options.preload.timeout && | ||
typeof options.preload.timeout === 'number' && | ||
!Number.isNaN(options.preload.timeout) | ||
) { | ||
config.timeout = options.preload.timeout; | ||
} | ||
return config; | ||
} | ||
axe.utils.getPreloadConfig = getPreloadConfig; | ||
|
||
/** | ||
* Returns a then(able) queue with results of all requested preload(able) assets. Eg: ['cssom']. | ||
* If preload is set to false, returns an empty queue. | ||
* @param {Object} options run configuration options (or defaults) passed via axe.run | ||
* @return {Object} queue | ||
*/ | ||
function preload(options) { | ||
const preloadFunctionsMap = { | ||
cssom: axe.utils.preloadCssom | ||
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 suggest you expose this so that it can be extended and you can test that this function is actually called when you call preload. 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. This is just an object map of various preload options/ types and the respective functions to execute. The tests for cssom, already test the execution of the respective function & any further additions to this hash, will have their own tests. So do not see the need to abstract any more. 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. See my comment about testing that preloadCssom is called and returned correctly. We'll need to do something to prove that this works. 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. Ignored as per discussion. |
||
}; | ||
|
||
const q = axe.utils.queue(); | ||
|
||
const shouldPreload = axe.utils.shouldPreload(options); | ||
if (!shouldPreload) { | ||
return q; | ||
} | ||
|
||
const preloadConfig = axe.utils.getPreloadConfig(options); | ||
|
||
preloadConfig.assets.forEach(asset => { | ||
q.defer((resolve, reject) => { | ||
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. What is the purpose of this top-level
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. prefer to keep the top level 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. The queue isn't chainable, so that wouldn't work. It only gives you one "then". 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. Oh, weird. I figured it worked the same way as a native Promise. 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. Still keeping this one, as is. Appreciate the comments. |
||
preloadFunctionsMap[asset]({ | ||
asset, | ||
timeout: preloadConfig.timeout | ||
}) | ||
.then(results => { | ||
const sheets = results[0]; | ||
resolve({ | ||
[asset]: sheets | ||
}); | ||
}) | ||
.catch(reject); | ||
}); | ||
}); | ||
|
||
return q; | ||
} | ||
axe.utils.preload = preload; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,4 +16,4 @@ | |
"aria-hidden-body" | ||
], | ||
"none": [] | ||
} | ||
} |
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 should add
&& sheet.cssRules.length <= 0
, rather than add an if statement in the try/catch block. Currently the code looks like it's missing an "else".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 cannot move .cssRules to here, as we need the catch to trigger for external stylesheets. Trying to read a .cssRules on external resource throws a SecurityError, which flows into the catch block. This is documented below.
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.
Although I have refactored slightly