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

fix: CSSOM generation for shadowRoot in Safari #1113

Merged
merged 14 commits into from
Oct 9, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
248 changes: 190 additions & 58 deletions lib/core/utils/preload-cssom.js
Original file line number Diff line number Diff line change
@@ -1,48 +1,122 @@
/**
* Make an axios get request to fetch a given resource and resolve
* @method getExternalStylesheet
* @param {Object} arg an object with properties to configure the external XHR
* @property {Object} arg.resolve resolve callback on queue
* @property {Object} arg.reject reject callback on queue
* @property {String} arg.url string representing the url of the resource to load
* @property {Object} arg.rootDocument document or shadowDOM root document for which to process CSSOM
* @property {Number} arg.timeout timeout to about network call
* @property {Function} arg.getStyleSheet a utility function to generate a style sheet for a given text content
* @property {String} arg.shadowId an id if undefined denotes that given root is a shadowRoot
* @property {Number} arg.priority css applied priority
* @private
*/
function getExternalStylesheet({
resolve,
reject,
url,
rootDocument,
timeout,
getStyleSheet,
shadowId,
priority
}) {
axe.imports
.axios({
method: 'get',
url,
timeout
})
.then(({ data }) => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall off the top of my head. Does Axios treat non-2xx response codes as errors? If not, we might want to add something like:

.then(({ data, status }) => {
  if ((status / 100 | 0) !== 2) {
    throw new Error('non-2xx response')
  }
  // ...
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Docs say that it does.

const sheet = getStyleSheet({
data,
isExternal: true,
shadowId,
root: rootDocument,
priority
});
resolve(sheet);
})
.catch(reject);
}

/**
* 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
* @method loadCssom
* @private
* @param {Object} arg an object with projects essential to load CSSOM
* @property {Object} arg.rootDocument document or shadowDOM root document for which to process CSSOM
* @property {Number} arg.rootIndex a number representing the index of the document or shadowDOM, used for priority
* @property {String} arg.shadowId an id if undefined denotes that given root is a shadowRoot
* @property {Number} timeout abort duration for network request
* @param {Function} getStyleSheet a utility function to generate a style sheet for a given text content
* @return {Object} queue
* @private

*/
function loadCssom({ root, shadowId }, timeout, convertTextToStylesheetFn) {
function loadCssom({
rootDocument,
rootIndex,
shadowId,
timeout,
getStyleSheet
}) {
const q = axe.utils.queue();

let styleSheets = null;

/**
* 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
* For shadowDOM #document-fragment node, fragment.styleSheets is not reliable in most browsers (mostly Safari)
* (Issue: https://github.com/dequelabs/axe-core/issues/1082)
*
* For stable results of CSSOM inside document fragment of shadowRoot, it is best to parse the children of the root
* and extaract tags <style> and <link> to construct dynamic styleSheets
*/
function getExternalStylesheet({ resolve, reject, url }) {
axe.imports
.axios({
method: 'get',
url,
timeout
})
.then(({ data }) => {
const sheet = convertTextToStylesheetFn({
data,
isExternal: true,
shadowId,
root
if (
rootDocument.nodeName.toUpperCase() === '#DOCUMENT-FRAGMENT' &&
shadowId
) {
// retrieve shadowRoot style sheets as []
styleSheets = Array.from(rootDocument.children).reduce((out, node) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you only looking at child nodes, instead of doing getElementsByTagName for style and link elements? It might make sense to break up the function into different load methods depending on the elements. Reduces the complexity of this function.

const nodeName = node.nodeName.toUpperCase();

// ignore if node is not of type style or link
if (nodeName !== 'STYLE' && nodeName !== 'LINK') {
return out;
}

// if style tag
// the contents are written as cssText into a dynamically created stylesheet
// these may be @import and or inline styles.
if (nodeName === 'STYLE') {
const dynamicSheet = getStyleSheet({
// no need to pass other arguments
data: node.textContent
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this solution work with link elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works, just fine as shown here: https://github.com/dequelabs/axe-core/pull/1113/files#diff-b195c7e1a05c62e60347f579d8f7dddeR339

It was the style tag that needed special parsing as above.

});
resolve(sheet);
})
.catch(reject);
}
out.push(dynamicSheet.sheet);
}

const q = axe.utils.queue();
// if link tag
// href is parsed and written as @import 'href'
// this helps keep concurrency, rather than awaiting onload on link
if (nodeName === 'LINK' && !node.media.includes('print')) {
const dynamicSheet = getStyleSheet({
data: node,
isLink: true
});
out.push(dynamicSheet.sheet);
}

// handle .styleSheets non existent on certain shadowDOM root
const rootStyleSheets = root.styleSheets
? Array.from(root.styleSheets)
: null;
if (!rootStyleSheets) {
// return
return out;
}, []);
} else {
// retrieve stylesheets as an []
styleSheets = Array.from(rootDocument.styleSheets);
}

// if no root styleSheets then return
if (!styleSheets || !styleSheets.length) {
return q;
}

Expand All @@ -51,7 +125,7 @@ function loadCssom({ root, shadowId }, timeout, convertTextToStylesheetFn) {
let sheetHrefs = [];

// filter out sheets, that should not be accounted for...
const sheets = rootStyleSheets.filter(sheet => {
const sheets = styleSheets.filter(sheet => {
// FILTER > sheets with the same href (if exists)
let sheetAlreadyExists = false;
if (sheet.href) {
Expand All @@ -75,14 +149,24 @@ function loadCssom({ root, shadowId }, timeout, convertTextToStylesheetFn) {
});

// iterate to decipher multi-level nested sheets if any (this is essential to retrieve styles from shadowDOM)
sheets.forEach(sheet => {
sheets.forEach((sheet, sheetIndex) => {
/* eslint max-statements: ["error", 20] */

// basic priority
const sheetLevelPriority = `${rootIndex}${sheetIndex}`;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand what's happening here. It looks like we're concatenating (not adding) numbers together in a string to be used as a numerical value later on. Can you explain how this works?

I'm stuck on the following:

A "priority" string is created with a rootIndex of 1 and a sheetIndex of 3000. It is then coerced to the integer 13000.

Later on, another string is created with rootIndex of 2 and sheetIndex of 1. It's then coerced to 21.

13000 is larger than 21, but its "root" was higher. Is this OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there is multiple levels of complexity for calculating priority.

Before going into the details, let me just say the above can and is solved by introducing decimals rootIndex.sheetIndex resulting in 1.1, 1.200, 2.1 and so on, hence the priority remains ordered as expected. Good spot. I have now done that.

image

Also, note that this is just the first pass at computing priority.
There are few more issues to tackle like below:

Which I believe will introduce further complexity to the priority calculation.
This PR mainly handles shadowDOM stylesheets resolution, and introduces the concept of priority.


// 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);

// if no cssRules - return
if (!rules.length) {
return;
}

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

Expand All @@ -94,47 +178,74 @@ function loadCssom({ root, shadowId }, timeout, convertTextToStylesheetFn) {
sheet,
isExternal: false,
shadowId,
root
root: rootDocument,
priority: Number(sheetLevelPriority)
})
);
return;
}

// if any import rules exists, fetch via `href` which eventually constructs a sheet with results from resource
// 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 });
getExternalStylesheet({
resolve,
reject,
url: rule.href,
rootDocument,
timeout,
getStyleSheet,
shadowId,
priority: Number(sheetLevelPriority)
});
});
});

// 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);

if (!inlineRules.length) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

!!! OK It took me 10 minutes before I understood what this is doing. Can you add a comment here, something along the lines of: // Stylesheet only has @import rules in it

}

// concat all cssText into a string for inline rules
const inlineRulesCssText = inlineRules
.reduce((out, rule) => {
out.push(rule.cssText);
return out;
}, [])
.join();

// create and return a sheet with inline rules
q.defer(resolve =>
resolve(
convertTextToStylesheetFn({
getStyleSheet({
data: inlineRulesCssText,
shadowId,
root,
isExternal: false
root: rootDocument,
isExternal: false,
priority: Number(sheetLevelPriority)
})
)
);
} catch (e) {
// external sheet -> make an xhr and q the response
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a bit misleading. This is a cross-origin stylesheet.

q.defer((resolve, reject) => {
getExternalStylesheet({ resolve, reject, url: sheet.href });
getExternalStylesheet({
resolve,
reject,
url: sheet.href,
rootDocument,
timeout,
getStyleSheet,
shadowId,
priority: Number(sheetLevelPriority)
});
});
}
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

This empty array is passed into sheets.forEach((sheet, sheetIndex) => {...}, []).


// return
return q;
}
Expand All @@ -158,7 +269,7 @@ function getAllRootsInTree(tree) {
.map(node => {
return {
shadowId: node.shadowId,
root: axe.utils.getRootNode(node.actualNode)
rootDocument: axe.utils.getRootNode(node.actualNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe stick with rootNode? That's what we're calling it everywhere else.

};
});
return documents;
Expand Down Expand Up @@ -188,34 +299,55 @@ axe.utils.preloadCssom = function preloadCssom({

/**
* Convert text content to CSSStyleSheet
* @method convertTextToStylesheet
* @method getStyleSheet
* @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
* @param {Object} arg an object with properties to construct stylesheet
* @property {String} arg.data text content of the stylesheet
* @property {Boolean} arg.isExternal flag to notify if the resource was fetched from the network
* @property {String} arg.shadowId (Optional) shadowId if shadowDOM
* @property {Object} arg.root implementation document to create style elements
* @property {String} arg.priority a number indicating the loaded priority of CSS, to denote specificity of styles contained in the sheet.
*/
function convertTextToStylesheet({ data, isExternal, shadowId, root }) {
function getStyleSheet({
data,
isExternal,
shadowId,
root,
priority,
isLink = false
}) {
const style = dynamicDoc.createElement('style');
style.type = 'text/css';
style.appendChild(dynamicDoc.createTextNode(data));
if (isLink) {
// as creating a stylesheet as link will need to be awaited
// till `onload`, it is wise to convert link href to @import statement
const text = dynamicDoc.createTextNode(`@import "${data.href}"`);
style.appendChild(text);
} else {
style.appendChild(dynamicDoc.createTextNode(data));
}
dynamicDoc.head.appendChild(style);
return {
sheet: style.sheet,
isExternal,
shadowId,
root
root,
priority
};
}

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) => {
.reduce((out, root, index) => {
out.defer((resolve, reject) => {
loadCssom(root, timeout, convertTextToStylesheet)
loadCssom({
rootDocument: root.rootDocument,
rootIndex: index + 1, // we want index to start with 1 for priority calculation
shadowId: root.shadowId,
timeout,
getStyleSheet
})
.then(resolve)
.catch(reject);
});
Expand Down
8 changes: 7 additions & 1 deletion test/core/utils/preload-cssom.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,13 @@ describe('axe.utils.preloadCssom unit tests', function() {
var cssom = results[0];
assert.lengthOf(cssom, 2);
cssom.forEach(function(o) {
assert.hasAllKeys(o, ['root', 'shadowId', 'sheet', 'isExternal']);
assert.hasAllKeys(o, [
'root',
'shadowId',
'sheet',
'isExternal',
'priority'
]);
});
done();
})
Expand Down
Loading