feat(preload): cssom assets #958

jeeyyy · 2018-06-20T11:46:37Z

This PR tackles the functionality of preloading assets, currently only cssom.
Only the boilerplate code to preload is built, this has not been injected into current run methods or checks.
The philosophy is such that, once this configuration is set, the audit.run calls a axe.utils.preload queue to retrieve any external assets, and parses the results and passes it back to audit.run.

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Has documentation updated, a DU ticket, or requires no documentation change
Includes new tests, or was unnecessary
Code is reviewed for security by: <Stephen

…overage.

WilcoFiers · 2018-07-31T11:48:48Z

test/integration/full/preload-cssom/preload-cssom.js

+		function start() {
+			// Stop messing with my tests Mocha!
+			document.querySelector('#mocha h1').outerHTML =
+				'<h2>preload cssom integration test</h2>';


What does that mean "otherwise there is no title generated"?

WilcoFiers · 2018-07-31T12:10:56Z

lib/core/utils/preload-cssom.js

+						timeout
+					})
+					.then(({ data }) => {
+						const sheet = getSheetFromTextFn(data, true); //second argument acts as > isExternal - true


That second argument is useful. It's always external. More importantly, isExternal shouldn't be set in getSheetFromTextFn at all. It belongs in the same function that you set isExternal = false (this one).

WilcoFiers · 2018-07-31T12:15:22Z

lib/core/utils/preload-cssom.js

+	const q = axe.utils.queue();
+
+	Array.from(ownerDocument.styleSheets).forEach(sheet => {
+		if (sheet.disabled) {


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

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.

Although I have refactored slightly

WilcoFiers · 2018-07-31T12:18:22Z

lib/core/utils/preload-cssom.js

+ * @return {Object}
+ */
+function preloadCssom({ timeout, treeRoot = axe._tree[0] }) {
+	const documents = getDocumentsFromTreeRoot(treeRoot);


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.

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.

WilcoFiers · 2018-07-31T12:30:11Z

lib/core/utils/preload.js

+ */
+function isValidPreloadObject(preload) {
+	if (typeof preload !== 'object') {
+		return preload;


preload=false shouldn't return false in isValidPreloadObject. This is misleading.

WilcoFiers · 2018-07-31T12:38:38Z

test/integration/full/preload-cssom/preload-cssom.js

+		return axe.utils.preloadCssom(config);
+	}
+
+	function commonTestsForRootAndFrame(root) {


One more thing we forgot to test is if cross domain sheets loaded through @import can be accessed. I suspect they might not, so there's still some work in there I think.

Indeed, you are right. I have come up with a solution for the same. Which is why shadowDOM work is going into this PR as against a new one.

Note:
Basic one-level deep nested @import is tackled here.
Have created an issue to tackle further enhancements - #1054

Changes done. Also supports shadowDOM

WilcoFiers · 2018-08-01T15:53:32Z

lib/core/utils/preload.js

+
+	// if type is object - ensure an array of assets to load is specified
+	if (!isValidPreloadObject(options.preload)) {
+		throw new Error(


This code will never run because if it isn't, your shouldPreload function causes utils.preload to exit early.

WilcoFiers · 2018-08-01T15:53:37Z

lib/core/utils/preload-cssom.js

+			// await loading all such documents assets, and concat results into one object
+			.then(assets => {
+				resolve(
+					assets.reduce((out, cssomSheets) => {


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

WilcoFiers · 2018-08-01T15:55:31Z

lib/core/utils/preload.js

+	if (typeof options.preload === 'boolean') {
+		return options.preload;
+	}
+	return isValidPreloadObject(options.preload);


I'd suggest we run this first and throw if its invalid, rather than do it in getPreloadConfig.

That is what we are doing.

const shouldPreload = axe.utils.shouldPreload(options); if (!shouldPreload) { return q; } const preloadConfig = axe.utils.getPreloadConfig(options);

WilcoFiers · 2018-08-01T16:03:33Z

test/integration/full/preload-cssom/preload-cssom.js

+		function start() {
+			// Stop messing with my tests Mocha!
+			document.querySelector('#mocha h1').outerHTML =
+				'<h2>preload cssom integration test</h2>';


It isn't. I put this in the page-has-heading-one tests to make sure I can test that page without mocha getting in the way. That's the only place its needed.

WilcoFiers · 2018-08-01T16:38:51Z

test/integration/full/preload-cssom/preload-cssom.js

+			}
+		);
+
+		shouldIt('should reject external stylesheets', function(done) {


Do you mean, "should reject if external sheet fails to load"?

WilcoFiers · 2018-08-01T16:52:44Z

lib/core/utils/preload-cssom.js

+
+			// there are external rules, best to filter non-external rules and create a new stylesheet to avoid duplication
+			const nonExternalRules = rules.filter(rule => !rule.href);
+			// concat all cssText into a string for non external rules


I'm having a very hard time understanding what's happening here. As best I can tell you're trying to turn same origin sheets and style elements with import statements into one long stylesheet... that doesn't seem like a good idea to me. This means we lose all context of how why and where a stylesheet got included. It also ignores that @import can have its own media value, and that they can be recursive. We also shouldn't ignore @import on cross-origin sheets.

Please be sure to include tests that covers all of these things.

Note: Paired on the above, refactored as discussed.

stephenmathieson · 2018-08-01T18:08:37Z

lib/core/utils/preload-cssom.js

+ * @param {Boolean} isExternal flag to specify if the stylesheet was fetched via xhr
+ * @param {String} shadowId (optional) string representing shadowId if style/ sheet was constructed from shadowDOM assets
+ */
+function getCssomSheet(sheet, isExternal, shadowId) {


This could be a 1-liner: const getCssomSheet = (sheet, isExternal, shadowId) => ({ sheet, isExternal, shadowId }).

Also, this function seems unnecessary.

lib/core/utils/preload-cssom.js

+	 * @private
+	 */
+	function getRootNode(node) {
+		var doc = (node.getRootNode && node.getRootNode()) || document; // this is for backwards compatibility


stephenmathieson · 2018-08-01T18:11:28Z

lib/core/utils/preload.js

+function isValidPreloadObject(preload) {
+	return (
+		typeof preload === 'object' &&
+		preload.hasOwnProperty('assets') &&


Can remove the .hasOwnProperty check, as Array.isArray(undefined) returns false.

stephenmathieson · 2018-08-01T18:12:31Z

lib/core/utils/preload.js

+ * @param {Object} options run configuration options (or defaults) passed via axe.run
+ * @return {boolean}
+ */
+axe.utils.shouldPreload = function shouldPreload(options) {


Could simplify this:

axe.utils.shouldPreload = options => options && isValidPreloadObject(options.preload)

Why was my comment marked as resolved? You didn't change anything or respond.....?

Simply because, I do not want to check if preload is a valid object until I am sure that it is an object. Hence a boolean check and an undefined check earlier.

Preload can take 3 values: true, false or { assets: ['assetKey'] } as defined in the docs.

Doing this options && isValidPreloadObject(options.preload) ignores and overrides the boolean values due to &&.

Scenarios (with your suggestion):

Preload Value options isValidPreloadObject output expected

options: undefined or null false false false false

options: { preload : true } true false false true (BOOM!)

options: { preload : false } true false false false

options: { preload : { assets: ['cssom'] } } true true true true

I am sure there is a clever way to shorten things further, but this keeps it easy to follow.
Prefer to keep this for that reason.

Appreciate the comments.

Discussed on Slack. Disregard my comment. This is good as-is!

stephenmathieson · 2018-08-01T18:13:23Z

lib/core/utils/preload.js

+
+	if (!areRequestedAssetsValid) {
+		throw new Error(
+			`Requested assets, not supported by aXe.` +


IIRC we're not capitalizing the "x" anymore.

to avoid i18n concerns removed aXe.

stephenmathieson · 2018-08-01T18:15:18Z

lib/core/utils/preload-cssom.js

+function getAllRootsInTree(tree) {
+	/**
+	 * Clone of axe.commons.dom.getRootNode
+	 * Note:


This seems like something that needs to be fixed first. Duplicating these functions will lead to maintenance nightmares.

Updated. Review again.

stephenmathieson · 2018-08-07T13:11:48Z

lib/commons/dom/get-root-node.js

-	}
-	return doc;
-};
+dom.getRootNode = axe.utils.getRootNode;


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) }

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

stephenmathieson · 2018-08-07T13:12:51Z

lib/core/utils/preload-cssom.js

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


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

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.

stephenmathieson

LGTM

jeeyyy added 21 commits June 5, 2018 14:55

feat(rule): css-orientation-lock raw structure

321f4f1

feat: adding preload config object.

33205ec

fix: merge > from develop

5e4f463

chore: merge from > develop

a3ea7c1

fix: wip new rule - css orientation

7119f2d

chore: merge from > develop

3782006

fix: wip - cssom work

508c795

style: update eslint to allow spread operator for object spreading.

b78ac30

feat: cssom preloading async

4e590a5

refactor: remove css-orientation-lock work from cssom preloading.

18b87e3

refactor: comments and clean-up

771bd88

feat: audit run queue to await cssom fetching.

eac06ac

refactor: change promise to q implementation.

cad3bb6

refactor: q plumbing from preloading to audit run.

d9ab58f

feat: refactor data marshalling of preloadedAssets.

858710f

style: fix linting errors.

678da39

refactor: seperate preload configuration method to utils for better c…

f763dec

…overage.

test: additional tests for cssom.

1b847ea

style: formamtting updates.

50229a0

style: update tests to not have any es6 keywords

8426fab

fix: valid-lang integration tests.

46ae47c

jeeyyy changed the title ~~[WIP] feat: preloading assets - cssom~~ feat: cssom - preloading assets Jun 25, 2018

jeeyyy added 2 commits June 25, 2018 22:37

docs: add preload configuration to api documentation

b03981b

docs: fix markdown lint.

0ba0ef9

jeeyyy requested review from WilcoFiers, isner, stephenmathieson and marcysutton June 25, 2018 22:03

jeeyyy mentioned this pull request Jun 25, 2018

Proposal: Need for loading external assets in axe-core #906

Closed

jeeyyy changed the title ~~feat: cssom - preloading assets~~ feat(preload): preloading cssom assets Jun 26, 2018

jeeyyy mentioned this pull request Jul 30, 2018

Support for preloading shadowDOM cssom assets #1031

Closed

jeeyyy added 2 commits July 31, 2018 09:58

fix: updates based on review.

3436cc0

Merge branch 'develop' into preload-cssom

12008e7

WilcoFiers reviewed Jul 31, 2018

View reviewed changes

WilcoFiers previously requested changes Jul 31, 2018

View reviewed changes

jeeyyy added 2 commits July 31, 2018 18:50

fix: shadowDOM support and review updates

8da721e

Merge branch 'develop' into preload-cssom

30a0819

chore: merge from develop

027b3a1

WilcoFiers previously requested changes Aug 1, 2018

View reviewed changes

stephenmathieson reviewed Aug 1, 2018

View reviewed changes

lib/core/utils/preload-cssom.js Outdated

* @private

*/

function getRootNode(node) {

var doc = (node.getRootNode && node.getRootNode()) || document; // this is for backwards compatibility

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

stephenmathieson reviewed Aug 1, 2018

View reviewed changes

jeeyyy changed the title ~~feat(preload): cssom assets~~ [WIP] feat(preload): cssom assets Aug 5, 2018

jeeyyy mentioned this pull request Aug 6, 2018

CSSOM: Tackle nested @import stylesheets #1054

Closed

3 tasks

fix: updates based on review

567ef3c

jeeyyy changed the title ~~[WIP] feat(preload): cssom assets~~ feat(preload): cssom assets Aug 6, 2018

Merge branch 'develop' into preload-cssom

b5c3f4f

stephenmathieson reviewed Aug 7, 2018

View reviewed changes

jeeyyy added 3 commits August 8, 2018 09:54

Merge branch 'develop' into preload-cssom

a8c74ad

fix: update preload function and tests

63cc5b4

Merge branch 'develop' into preload-cssom

247a7cb

stephenmathieson approved these changes Aug 8, 2018

View reviewed changes

jeeyyy merged commit 5d6c1fa into develop Aug 8, 2018

jeeyyy deleted the preload-cssom branch August 8, 2018 17:09

Preload Value	options	isValidPreloadObject	output	expected
`options: undefined or null`	false	false	false	false
`options: { preload : true }`	true	false	false	true (BOOM!)
`options: { preload : false }`	true	false	false	false
`options: { preload : { assets: ['cssom'] } }`	true	true	true	true

feat(preload): cssom assets #958

feat(preload): cssom assets #958

Conversation

jeeyyy commented Jun 20, 2018 • edited by stephenmathieson Loading

Reviewer checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenmathieson left a comment

Choose a reason for hiding this comment

jeeyyy commented Jun 20, 2018 •

edited by stephenmathieson

Loading

jeeyyy Jul 31, 2018 •

edited

Loading

jeeyyy Aug 8, 2018 •

edited

Loading