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.

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

Merged
merged 60 commits into from
Aug 8, 2018
Merged
Show file tree
Hide file tree
Changes from 57 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
321f4f1
feat(rule): css-orientation-lock raw structure
jeeyyy Jun 5, 2018
33205ec
feat: adding preload config object.
jeeyyy Jun 6, 2018
5e4f463
fix: merge > from develop
jeeyyy Jun 6, 2018
a3ea7c1
chore: merge from > develop
jeeyyy Jun 7, 2018
7119f2d
fix: wip new rule - css orientation
jeeyyy Jun 7, 2018
3782006
chore: merge from > develop
jeeyyy Jun 7, 2018
508c795
fix: wip - cssom work
jeeyyy Jun 7, 2018
b78ac30
style: update eslint to allow spread operator for object spreading.
jeeyyy Jun 19, 2018
4e590a5
feat: cssom preloading async
jeeyyy Jun 19, 2018
18b87e3
refactor: remove css-orientation-lock work from cssom preloading.
jeeyyy Jun 19, 2018
771bd88
refactor: comments and clean-up
jeeyyy Jun 19, 2018
eac06ac
feat: audit run queue to await cssom fetching.
jeeyyy Jun 20, 2018
cad3bb6
refactor: change promise to q implementation.
jeeyyy Jun 20, 2018
d9ab58f
refactor: q plumbing from preloading to audit run.
jeeyyy Jun 20, 2018
858710f
feat: refactor data marshalling of preloadedAssets.
jeeyyy Jun 20, 2018
678da39
style: fix linting errors.
jeeyyy Jun 20, 2018
f763dec
refactor: seperate preload configuration method to utils for better c…
jeeyyy Jun 25, 2018
1b847ea
test: additional tests for cssom.
jeeyyy Jun 25, 2018
50229a0
style: formamtting updates.
jeeyyy Jun 25, 2018
8426fab
style: update tests to not have any es6 keywords
jeeyyy Jun 25, 2018
46ae47c
fix: valid-lang integration tests.
jeeyyy Jun 25, 2018
b03981b
docs: add preload configuration to api documentation
jeeyyy Jun 25, 2018
0ba0ef9
docs: fix markdown lint.
jeeyyy Jun 25, 2018
ba00d6b
refactor: lint, test & docs updates.
jeeyyy Jun 26, 2018
4fa9d6a
fix: merge from develop
jeeyyy Jul 1, 2018
2074cc9
docs: update api documentation for preload configuration
jeeyyy Jul 1, 2018
9f57cdb
chore: remove merge redundant files
jeeyyy Jul 1, 2018
f18db7f
refactor: revert preload changes to run/ audit/ checks
jeeyyy Jul 2, 2018
1434751
fix: preload changes based on review
jeeyyy Jul 2, 2018
b64309c
refactor: revert formatting changes
jeeyyy Jul 2, 2018
5ea88d5
fix: update shadown dom test to not pollute fixture
jeeyyy Jul 2, 2018
28bdcdd
fix: markdown lint
jeeyyy Jul 2, 2018
87820da
Merge branch 'develop' into preload-cssom
jeeyyy Jul 2, 2018
ae59586
fix: refactor based on review
jeeyyy Jul 4, 2018
15d71f1
fix: merge from origin
jeeyyy Jul 4, 2018
3a10d63
Merge branch 'develop' into preload-cssom
jeeyyy Jul 12, 2018
222f05b
fix: implement axios against xhrQ
jeeyyy Jul 12, 2018
7f061ae
fix: refactor based on comments/ review
jeeyyy Jul 12, 2018
0fca2d1
refactor: try catch block for stylesheet cssRules
jeeyyy Jul 12, 2018
2155df8
refactor: changes based on code review
jeeyyy Jul 17, 2018
0923c36
style: revert formatting changes
jeeyyy Jul 17, 2018
29e3c61
docs: update documentation for preload configuration
jeeyyy Jul 17, 2018
9ecba9e
test: add integration tests for preload-cssom
jeeyyy Jul 17, 2018
0b9d62d
docs: fix markdown lint
jeeyyy Jul 17, 2018
f06701b
Merge branch 'develop' into preload-cssom
jeeyyy Jul 17, 2018
20e6f7d
fix: code review based refactor
jeeyyy Jul 30, 2018
06f6dfc
Merge branch 'develop' into preload-cssom
jeeyyy Jul 30, 2018
9e28b8a
Merge branch 'develop' into preload-cssom
jeeyyy Jul 30, 2018
483bcd8
test: update tests based on code review
jeeyyy Jul 30, 2018
a3ecee7
Merge branch 'develop' into preload-cssom
jeeyyy Jul 30, 2018
3436cc0
fix: updates based on review.
jeeyyy Jul 31, 2018
12008e7
Merge branch 'develop' into preload-cssom
jeeyyy Jul 31, 2018
8da721e
fix: shadowDOM support and review updates
jeeyyy Jul 31, 2018
30a0819
Merge branch 'develop' into preload-cssom
jeeyyy Jul 31, 2018
027b3a1
chore: merge from develop
jeeyyy Jul 31, 2018
567ef3c
fix: updates based on review
jeeyyy Aug 6, 2018
b5c3f4f
Merge branch 'develop' into preload-cssom
jeeyyy Aug 6, 2018
a8c74ad
Merge branch 'develop' into preload-cssom
jeeyyy Aug 8, 2018
63cc5b4
fix: update preload function and tests
jeeyyy Aug 8, 2018
247a7cb
Merge branch 'develop' into preload-cssom
jeeyyy Aug 8, 2018
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
25 changes: 24 additions & 1 deletion doc/API.md
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,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))
Expand All @@ -339,6 +339,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

Expand Down Expand Up @@ -460,6 +461,28 @@ 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 current set of values supported for `assets` is listed in the following table:

| Asset Type | Description |
|:-----------|:------------|
| `cssom` | This asset type preloads all CSS Stylesheets rulesets specified in the page. The stylessheets can be an external cross-domain resource, a relative stylesheet or an inline style with in the head tag of the document. If the stylesheet is an external cross-domain a network request is made. An object representing the CSS Rules from each stylesheet is made available to the checks evaluate function as `preloadedAssets` at run-time |

The `timeout` attribute in the object configuration is `optional` and has a fallback default value (10000ms). The `timeout` is essential for any network dependent assets that are preloaded, where-in if a given request takes longer than the specified/ default value, the operation is aborted.

##### 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.
Expand Down
2 changes: 1 addition & 1 deletion lib/checks/aria/aria-hidden-body.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@
"fail": "aria-hidden=true should not be present on the document body"
}
}
}
}
10 changes: 2 additions & 8 deletions lib/commons/dom/get-root-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,6 @@
* @instance
* @param {Element} node
* @returns {DocumentFragment|Document}
* @deprecated use axe.utils.getRootNode
*/
dom.getRootNode = function(node) {
var doc = (node.getRootNode && node.getRootNode()) || document; // this is for backwards compatibility
if (doc === node) {
// disconnected node
doc = document;
}
return doc;
};
dom.getRootNode = axe.utils.getRootNode;
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a warning here when this method is called? Just aliasing it doesn't seem to give users any reason not to use it.

Something like:

let didWarn = false
dom.getRootNode = (...args) => {
  if (!didWarn) {
    console.warn('axe-core: dom.getRootNode has been deprecated; use utils.getRootNode instead.')
    didWarn = true
  }
  axe.utils.getRootNode(...args)
}

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 like this.

Reason I did not do this is right away is, we are bypassing the function to axe.utils for the time being.

The jsdoc @deprecated notice is a flag for the dev's (us) to take some action on a later date.

There are multiple places with in the code base still referring to axe.commons.dom.getRootNode, until we change all these references internally first, to axe.utils.getRootNode believe we should not warn on the console.

And before all the internal changes come be made, this PR needs to be merged.

Also reckon a deprecation notice should dictate when an API is going to be deprecated, so we need to agree on that, something like this function has been deprecated from v3.2.0, use newFunction.

Happy to add this, once we reach that consensus.

@marcysutton @WilcoFiers - thoughts?

Thanks @stephenmathieson

4 changes: 3 additions & 1 deletion lib/core/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -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']), // overtime this array will grow with other preload asset types, this constant is to verify if a requested preload type by the user via the configuration is supported by axe.
preloadAssetsTimeout: 10000
};

definitions.forEach(function(definition) {
Expand Down
18 changes: 18 additions & 0 deletions lib/core/utils/get-root-node.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/* global axe */

/**
* Return the document or document fragment (shadow DOM)
* @method getRootNode
* @memberof axe.utils
* @instance
* @param {Element} node
* @returns {DocumentFragment|Document}
*/
axe.utils.getRootNode = function getRootNode(node) {
var doc = (node.getRootNode && node.getRootNode()) || document; // this is for backwards compatibility
if (doc === node) {
// disconnected node
doc = document;
}
return doc;
};
205 changes: 205 additions & 0 deletions lib/core/utils/preload-cssom.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
/**
* 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} convertTextToStylesheetFn a utility function to generate a style sheet from text
* @return {Object} queue
* @private
*/
function loadCssom({ root, shadowId }, timeout, convertTextToStylesheetFn) {
/**
* Make an axios get request to fetch a given resource and resolve
* @method getExternalStylesheet
* @private
* @param {Object} param an object with properties to configure the external XHR
* @property {Object} param.resolve resolve callback on queue
* @property {Object} param.reject reject callback on queue
* @property {String} param.url string representing the url of the resource to load
* @property {Number} param.timeout timeout to about network call
*/
function getExternalStylesheet({ resolve, reject, url }) {
axe.imports
.axios({
method: 'get',
url,
timeout
})
.then(({ data }) => {
const sheet = convertTextToStylesheetFn({
data,
isExternal: true,
shadowId
});
resolve(sheet);
})
.catch(reject);
}

const q = axe.utils.queue();

// iterate to decipher multi-level nested sheets if any (this is essential to retrieve styles from shadowDOM)
Array.from(root.styleSheets).forEach(sheet => {
// ignore disabled sheets
if (sheet.disabled) {
Copy link
Contributor

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

Copy link
Contributor Author

@jeeyyy jeeyyy Jul 31, 2018

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.

Copy link
Contributor Author

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

return;
}
// attempt to retrieve cssRules, or for external sheets make a XMLHttpRequest
try {
// accessing .cssRules throws for external (cross-domain) sheets, which is handled in the catch
const cssRules = sheet.cssRules;
// read all css rules in the sheet
const rules = Array.from(cssRules);

// filter rules that are included by way of @import or nested link
const importRules = rules.filter(r => r.href);

// if no import or nested link rules, with in these cssRules
// return current sheet
if (!importRules.length) {
q.defer(resolve =>
resolve({
sheet,
isExternal: false,
shadowId
})
);
return;
}

// if any import rules exists, fetch via `href` which eventually constructs a sheet with results from resource
importRules.forEach(rule => {
q.defer((resolve, reject) => {
getExternalStylesheet({ resolve, reject, url: rule.href });
});
});

// in the same sheet - get inline rules in <style> tag or in a CSSStyleSheet excluding @import or nested link
const inlineRules = rules.filter(rule => !rule.href);

// concat all cssText into a string for inline rules
const inlineRulesCssText = inlineRules
.reduce((out, rule) => {
Copy link
Member

Choose a reason for hiding this comment

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

This .reduce() seems to be performing the function of .map(). Should we refactor to use the intended native method here?

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 am taking each CSSRule and appending its cssText in to an array - out. I intend to keep this as reduce, as is, because I believe there will be logic going into this iteration as I tackle for @import and media rules.

Appreciate the comment.

out.push(rule.cssText);
return out;
}, [])
.join();
// create and return a sheet with inline rules
q.defer(resolve =>
resolve(
convertTextToStylesheetFn({
data: inlineRulesCssText,
shadowId,
isExternal: false
})
)
);
} catch (e) {
// if no href, do not attempt to make an XHR, but this is preventive check
// NOTE: as further enhancements to resolve nested @imports are done, a decision to throw an Error if necessary here will be made.
if (!sheet.href) {
return;
}
// external sheet -> make an xhr and q the response
q.defer((resolve, reject) => {
getExternalStylesheet({ resolve, reject, url: sheet.href });
});
}
}, []);
// return
return q;
}

/**
* Returns an array of objects with root(document)
* @param {Object} treeRoot the DOM tree to be inspected
* @return {Array<Object>} array of objects, which each object containing a root (document) and an optional shadowId
* @private
*/
function getAllRootsInTree(tree) {
let ids = [];
const documents = axe.utils
.querySelectorAllFilter(tree, '*', node => {
if (ids.includes(node.shadowId)) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 node.shadowId && ids.includes(.... We've been burned by bad polyfills before.

Copy link
Contributor Author

@jeeyyy jeeyyy Jul 17, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine, but you owe me a beer when we get this reported. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
shadowId: node.shadowId,
root: axe.utils.getRootNode(node.actualNode)
};
});
return documents;
}

/**
* @method preloadCssom
* @memberof axe.utils
* @instance
* @param {Object} object argument which is a composite object, with attributes timeout, treeRoot(optional), resolve & reject
* @property {Number} timeout timeout for any network calls made
* @property {Object} treeRoot the DOM tree to be inspected
* @return {Object} a queue with results of cssom assets
*/
axe.utils.preloadCssom = function preloadCssom({
timeout,
treeRoot = axe._tree[0]
}) {
const roots = axe.utils.uniqueArray(getAllRootsInTree(treeRoot), []);
const q = axe.utils.queue();

if (!roots.length) {
return q;
}

const dynamicDoc = document.implementation.createHTMLDocument();

/**
* Convert text content to CSSStyleSheet
* @method convertTextToStylesheet
* @private
* @param {Object} param an object with properties to construct stylesheet
* @property {String} param.data text content of the stylesheet
* @property {Boolean} param.isExternal flag to notify if the resource was fetched from the network
* @property {Object} param.doc implementation document to create style elements
* @property {String} param.shadowId (Optional) shadowId if shadowDOM
*/
function convertTextToStylesheet({ data, isExternal, shadowId }) {
const style = dynamicDoc.createElement('style');
style.type = 'text/css';
style.appendChild(dynamicDoc.createTextNode(data));
dynamicDoc.head.appendChild(style);
return {
sheet: style.sheet,
isExternal,
shadowId
};
}

q.defer((resolve, reject) => {
// as there can be multiple documents (root document, shadow document fragments, and frame documents)
// reduce these into a queue
roots
.reduce((out, root) => {
out.defer((resolve, reject) => {
loadCssom(root, timeout, convertTextToStylesheet)
.then(resolve)
.catch(reject);
});
return out;
}, axe.utils.queue())
// await loading all such documents assets, and concat results into one object
.then(assets => {
resolve(
assets.reduce((out, cssomSheets) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could have done return out.concat(cssomSheets).

This comment was marked as resolved.

return out.concat(cssomSheets);
}, [])
);
})
.catch(reject);
});

return q;
};
Loading