-
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(preload): cssom assets #958
Changes from 33 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 |
---|---|---|
|
@@ -324,7 +324,7 @@ The options parameter is flexible way to configure how `axe.run` operates. The d | |
Additionally, there are a number or properties that allow configuration of different options: | ||
|
||
| Property | Default | Description | | ||
|-----------------|:-------:|:----------------------------:| | ||
|-----------------|:-------|:----------------------------| | ||
| `runOnly` | n/a | Limit which rules are executed, based on names or tags | ||
| `rules` | n/a | Allow customizing a rule's properties (including { enable: false }) | ||
| `reporter` | `v1` | Which reporter to use (see [Configuration](#api-name-axeconfigure)) | ||
|
@@ -335,6 +335,7 @@ Additionally, there are a number or properties that allow configuration of diffe | |
| `elementRef` | `false` | Return element references in addition to the target | ||
| `restoreScroll` | `false` | Scrolls elements back to before axe started | ||
| `frameWaitTime` | `60000` | How long (in milliseconds) axe waits for a response from embedded frames before timing out | ||
| `preload` | `false` | Any additional assets (eg: cssom) to preload before running rules. [See here for configuration details](#preload-configuration-details) | ||
|
||
###### Options Parameter Examples | ||
|
||
|
@@ -456,6 +457,24 @@ Additionally, there are a number or properties that allow configuration of diffe | |
``` | ||
This example will process all of the "violations", "incomplete", and "inapplicable" result types. Since "passes" was not specified, it will only process the first pass for each rule, if one exists. As a result, the results object's `passes` array will have a length of either `0` or `1`. On a series of extremely large pages, this would improve performance considerably. | ||
|
||
###### <a id='preload-configuration-details'></a> Preload Configuration in Options Parameter | ||
|
||
The preload attribute in options parameter accepts a `boolean` or an `object` where an array of assets can be specified. | ||
|
||
1. Specifying a `boolean` | ||
|
||
```js | ||
preload: true | ||
``` | ||
|
||
2. Specifying an `object` | ||
```js | ||
preload: { assets: ['cssom'], timeout: 50000 } | ||
``` | ||
The `assets` attribute expects an array of preload(able) constraints to be fetched. | ||
|
||
The `timeout` attribute in the object configuration is `optional` and has a fallback default value. The `timeout` is essential for any network dependent assets that are preloaded. | ||
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. Specify what timeout does - mostly that it's total time used for loading - not time per resource, correct? |
||
|
||
##### Callback Parameter | ||
|
||
The callback parameter is a function that will be called when the asynchronous `axe.run` function completes. The callback function is passed two parameters. The first parameter will be an error thrown inside of aXe if axe.run could not complete. If axe completed correctly the first parameter will be null, and the second parameter will be the results object. | ||
|
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 |
---|---|---|
|
@@ -31,7 +31,9 @@ | |
results: [], | ||
resultGroups: [], | ||
resultGroupMap: {}, | ||
impact: Object.freeze(['minor', 'moderate', 'serious', 'critical']) | ||
impact: Object.freeze(['minor', 'moderate', 'serious', 'critical']), | ||
preloadAssets: Object.freeze(['cssom']), | ||
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 this for? Why do we need to Maybe add a comment here explaining "why" we need this (frozen) array? |
||
preloadAssetsTimeout: 10000 | ||
}; | ||
|
||
definitions.forEach(function(definition) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
/** | ||
* 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) { | ||
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. You should add 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. 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 commentThe reason will be displayed to describe this comment to others. Learn more. Although I have refactored slightly |
||
return; | ||
} | ||
|
||
try { | ||
// attempt to resolve if sheet is relative/ same domain | ||
sheet.cssRules && q.defer(resolve => resolve(sheet)); | ||
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 looks very "clever". Why not use an
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. Cannot use |
||
} catch (e) { | ||
const deferredSheet = (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. Probably just inline this |
||
axe.utils | ||
.xhrQ({ | ||
url: sheet.href, | ||
timeout | ||
}) | ||
.then(xhrResponse => { | ||
if ( | ||
xhrResponse && | ||
Array.isArray(xhrResponse) && | ||
xhrResponse.length | ||
) { | ||
xhrResponse.forEach(r => { | ||
const text = r.responseText ? r.responseText : r.response; | ||
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. In what case(s) would 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. Ignore the above - likely will change, once axios is in place - #990 |
||
const href = r.responseURL; | ||
const sheet = getSheetFromTextFn(text, href); | ||
resolve(sheet); | ||
}); | ||
} | ||
}) | ||
.catch(reject); | ||
}; | ||
// external sheet -> make an xhr and q the response | ||
q.defer(deferredSheet); | ||
} | ||
}); | ||
|
||
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({ asset, timeout, treeRoot = axe._tree[0] }) { | ||
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.
|
||
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 getSheetFromTextFn = (function() { | ||
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 IIFE? It looks like we are trying to do some scoping tricks with 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. Aim is to re-use the same 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 think we can avoid the IIFE tho: const dynamicDoc = document.implementation.createHTMLDocument()
const getSheetFromTextFn = cssText => {
// ...
} |
||
let htmlHead = document.implementation.createHTMLDocument().head; | ||
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.
|
||
return (cssText, href) => { | ||
// create style node with css text | ||
let style = document.createElement('style'); | ||
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.
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 broken. It is using a different document object to create the elements that it is then attempting to insert in the htmlHead's document. You need to store and use the document that gets created on current line 93 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. Good spot! |
||
style.type = 'text/css'; | ||
style.href = href; | ||
style.appendChild(document.createTextNode(cssText)); | ||
// added style to temporary document | ||
htmlHead.appendChild(style); | ||
return style.sheet; | ||
}; | ||
})(); // invoke immediately | ||
|
||
documents.forEach(doc => { | ||
q.defer((resolve, reject) => { | ||
loadCssom(doc, timeout, getSheetFromTextFn) | ||
.then(sheets => | ||
resolve({ | ||
[asset]: sheets | ||
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 should just be |
||
}) | ||
) | ||
.catch(reject); | ||
}); | ||
}); | ||
|
||
return q; | ||
} | ||
axe.utils.preloadCssom = preloadCssom; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
/** | ||
* 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) && | ||
preload.assets.length | ||
); | ||
} | ||
|
||
/** | ||
* 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 === typeof 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. Please use |
||
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 | ||
let out = { | ||
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. Doesn't look like |
||
assets: axe.constants.preloadAssets, | ||
timeout: axe.constants.preloadAssetsTimeout | ||
}; | ||
|
||
// if type is boolean | ||
if (typeof options.preload === typeof 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. This is a very strange condition. Why not just do:
|
||
return out; | ||
} | ||
|
||
// if type is object - ensure an array of assets to load is specified | ||
if (isPreloadValidObject(options.preload)) { | ||
const requestedAssets = []; | ||
options.preload.assets.forEach(asset => { | ||
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. Looks like we're constructing a new array of assets in this loop. Maybe we should IMO the usage of 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. Came up with a different workflow, so can throw earlier, than in an iteration, also keeps the line of sight in the code to the left. |
||
const a = asset.toLowerCase(); | ||
if (axe.constants.preloadAssets.includes(a)) { | ||
// unique assets to load, in case user had requested same asset type many times. | ||
if (!requestedAssets.includes(a)) { | ||
requestedAssets.push(a); | ||
} | ||
return a; | ||
} else { | ||
const e = | ||
`Requested asset: ${a}, not supported by aXe.` + | ||
`Supported assets are: ${axe.constants.preloadAssets | ||
.map(_ => _) | ||
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 |
||
.join(', ')}.`; | ||
throw new Error(e); | ||
} | ||
}); | ||
out.assets = requestedAssets; | ||
|
||
if (options.preload.timeout) { | ||
if ( | ||
typeof options.preload.timeout === 'number' && | ||
!Number.isNaN(options.preload.timeout) | ||
) { | ||
out.timeout = options.preload.timeout; | ||
} else { | ||
throw new Error(`preload timeout specified is not of type number`); | ||
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 this sort of validation be done sooner? It seems like we've already performed a lot of instructions so far, and now we're just going to throw and halt the program. 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. Given this a thought, should not throw here, fallback to defaults instead. |
||
} | ||
} | ||
|
||
return out; | ||
} else { | ||
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 block is unnecessary, since we 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. Also, I suggest doing the inverse of this and de-denting the happy path above. For example:
|
||
throw new Error( | ||
'No assets configured for preload in aXe run configuration' | ||
); | ||
} | ||
} | ||
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 => { | ||
resolve(results[0]); | ||
}) | ||
.catch(reject); | ||
}); | ||
}); | ||
|
||
return q; | ||
} | ||
axe.utils.preload = preload; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/** | ||
* Returns a then(able) queue of XHR's | ||
* @param {Object} config configuration for XMLHttpRequest | ||
* @return {Object} | ||
*/ | ||
axe.utils.xhrQ = (config) => { | ||
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. Writing your own XHR utility scares me. HTTP requests are complicated things and servers do not always behave in reasonable ways. I know there's not currently a way to Maybe we could do something like this:
There's possibly even a Grunt plugin or something that could do this for us. 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 - #990 |
||
'use strict'; | ||
|
||
const request = new XMLHttpRequest(); // IE7+ friendly | ||
|
||
const q = axe.utils.queue(); | ||
|
||
q.defer((resolve, reject) => { | ||
// wire up timeout | ||
request.timeout = config.timeout; | ||
|
||
// listen for timeout | ||
request.ontimeout = () => { | ||
reject({ | ||
status: request.status, | ||
statusText: request.statusText | ||
}); | ||
} | ||
|
||
// monitor ready state | ||
request.onreadystatechange = () => { | ||
// request is not complete. | ||
if (request.readyState !== 4) { | ||
return; | ||
} | ||
// process the response | ||
if (request.status >= 200 && request.status <= 300) { | ||
// success | ||
resolve(request); | ||
} else { | ||
// failure | ||
reject({ | ||
status: request.status, | ||
statusText: request.statusText | ||
}); | ||
} | ||
}; | ||
|
||
// setup request | ||
request.open(config.method || 'GET', config.url, true); | ||
|
||
// add headers if any | ||
if (config.headers) { | ||
Object | ||
.keys(config.headers) | ||
.forEach((k) => { | ||
request | ||
.setRequestHeader(k, config.headers[k]); | ||
}); | ||
} | ||
|
||
// enumerate and construct params | ||
let params = config.params; | ||
if (params && | ||
typeof params === 'object') { | ||
params = Object.keys(params) | ||
.map((k) => { | ||
return `${encodeURIComponent(k)}=${encodeURIComponent(params[k])}`; | ||
}) | ||
.join('&'); | ||
} | ||
|
||
// send | ||
request.send(params); | ||
}); | ||
|
||
return q; | ||
} |
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.
Specify what CSSOM will do.