fix: CSSOM generation for shadowRoot in Safari #1113

jeeyyy · 2018-08-29T14:18:07Z

This PR handles the edge case where for a given shadowRoot, the root.styleSheets lookup is returned as undefined | null.

This meant evaluation of styles with in shadowDOM failed.

This fix, tackles the edge case by parsing the content of the shadowRoot, to reverse engineer styleSheets from style tag if any specified with in the shadowRoot.

Upon reverse engineering the sheets, it is lead to flow into the generic loadCssom function to do the necessary generation of CSSOM as intended.

The concept of priority in stylesheets loaded is also introduced. The priority computation will be enhanced when handling multiple nested @import stylesheets in a separate PR after this lands.

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: @WilcoFiers

WilcoFiers · 2018-08-30T10:19:01Z

lib/core/utils/preload-cssom.js

+
+	if (!root.styleSheets) {
+		/**
+		 * In Safari: (Issue: https://github.com/dequelabs/axe-core/issues/1082)


Please report this as a bug to Apple.

Reported, but do not have an issue/ bug reported link from them yet.

WilcoFiers · 2018-08-30T10:19:37Z

lib/core/utils/preload-cssom.js

+			})
+			.map(node => {
+				const sheet = convertTextToStylesheetFn({
+					data: node.textContent


How does this solution work with link elements?

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.

WilcoFiers · 2018-08-30T10:22:15Z

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

+		);
+
+		(shadowSupported ? it : xit)(
+			'should return styles from shadow dom',


Make sure to test link element stylesheets as well.

jeeyyy · 2018-08-31T04:26:05Z

@WilcoFiers
Updated with handling link as requested as well.
Have also introduced the concept of priority, but needs more work to tackle other scenarios and enhancing css-orientation-lock tests.

Please review this again.

jeeyyy · 2018-09-03T07:54:10Z

@dylanb - review appreciated.

stephenmathieson · 2018-09-04T14:33:50Z

lib/core/utils/preload-cssom.js

+		/* eslint max-statements: ["error", 20] */
+
+		// basic priority
+		const sheetLevelPriority = `${rootIndex}${sheetIndex}`;


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?

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.

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

CSSOM: Handle Priority of styles applied (PR #1113) #1078

CSSOM: Tackle nested @import stylesheets #1054

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

WilcoFiers

Couple minor points. I'd also like @dylanb to take a look.

WilcoFiers · 2018-09-19T12:46:08Z

lib/core/utils/preload-cssom.js

+		shadowId
+	) {
+		// retrieve shadowRoot style sheets as []
+		styleSheets = Array.from(rootDocument.children).reduce((out, node) => {


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.

WilcoFiers · 2018-09-19T12:51:17Z

lib/core/utils/preload-cssom.js

+		/* eslint max-statements: ["error", 20] */
+
+		// padding sheet index with root index to create priority
+		const sheetLevelPriority = Number(`${rootIndex}.${sheetIndex}`);


Doesn't this break if we've got more than 10 sheets? Is this priority something you came up with or is it part of a spec some place? This seems fragile.

WilcoFiers · 2018-09-19T12:54:17Z

lib/core/utils/preload-cssom.js

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


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

Updated. Please review again

WilcoFiers · 2018-10-03T09:11:09Z

lib/core/utils/preload-cssom.js

 */
-function loadCssom({ root, shadowId }, timeout, convertTextToStylesheetFn) {
+function loadCssom({ rootNode, rootIndex, shadowId, timeout, getStyleSheet }) {


Can you please break this function up into more readable chunks? It's a 200 line function that does half a dozen different things.

I've taken a stab at simplifying this. Here's a suggestion. Also notice this is readable without a comment on every line.

function loadCssom(options) { const { rootIndex, rootNode, shadowId } = options; const q = axe.utils.queue(); const styleSheets = (rootNode.nodeType === 7 && shadowId ? getSheetsFromShadowDom(options) : getSheetsFromLightDom(options) ); const filteredSheets = styleSheets .filter(isFirstHrefSheet) .filter(notPrintSheet); filteredSheets.forEach((sheet, sheetIndex) => { let sheetLoader; try { // The following line throws an error on cross-origin style sheets: const cssRules = sheet.cssRules; sheetLoader = getSameOriginSheet; } catch () { sheetLoader = getCrossOriginSheet; } q.defer((resolve, reject) => { sheetLoader({ priority: [rootIndex, sheetIndex] url: rule.href, resolve, reject, ...options }); }); }); return q; })

WilcoFiers · 2018-10-03T09:35:38Z

lib/core/utils/preload-cssom.js

+					getStyleSheet,
+					shadowId,
+					priority
+				});
 			});
 		}
 	}, []);


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

WilcoFiers · 2018-10-03T09:51:40Z

lib/core/utils/preload-cssom.js

 */
-function loadCssom({ root, shadowId }, timeout, convertTextToStylesheetFn) {
+function loadCssom({ rootNode, rootIndex, shadowId, timeout, getStyleSheet }) {


I've taken a stab at simplifying this. Here's a suggestion. Also notice this is readable without a comment on every line.

function loadCssom(options) { const { rootIndex, rootNode, shadowId } = options; const q = axe.utils.queue(); const styleSheets = (rootNode.nodeType === 7 && shadowId ? getSheetsFromShadowDom(options) : getSheetsFromLightDom(options) ); const filteredSheets = styleSheets .filter(isFirstHrefSheet) .filter(notPrintSheet); filteredSheets.forEach((sheet, sheetIndex) => { let sheetLoader; try { // The following line throws an error on cross-origin style sheets: const cssRules = sheet.cssRules; sheetLoader = getSameOriginSheet; } catch () { sheetLoader = getCrossOriginSheet; } q.defer((resolve, reject) => { sheetLoader({ priority: [rootIndex, sheetIndex] url: rule.href, resolve, reject, ...options }); }); }); return q; })

Updated.

jeeyyy · 2018-10-04T14:19:25Z

@WilcoFiers
Thanks for the refactor suggestions, embraced most of it.

The suggestion as per this snippet #1113 (comment), to use sheetLoader which then is deferred into the queue is not as simple, so left that section after minimal refactor.

Reviews welcome.

stephenmathieson · 2018-10-04T15:01:55Z

lib/core/utils/preload-cssom.js

+			url,
+			timeout
+		})
+		.then(({ data }) => {


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') } // ... })

Docs say that it does.

WilcoFiers

Editorials. You're welcome to merge and raise a new PR with the editorial changes.

WilcoFiers · 2018-10-08T16:32:51Z

lib/core/utils/preload-cssom.js

+ * @param {Array<Object>} styleSheets array of stylesheets
+ * @returns an filtered array of stylesheets
+ */
+function filterStyleSheets(styleSheets) {


Filter it based on what? This needs a better name.

WilcoFiers · 2018-10-08T16:34:53Z

lib/core/utils/preload-cssom.js

-						isExternal: false
+						root: rootNode,
+						isExternal: false,
+						priority
 					})
 				)
 			);
 		} catch (e) {
 			// external sheet -> make an xhr and q the response


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

WilcoFiers · 2018-10-08T16:49:45Z

lib/core/utils/preload-cssom.js

 				});
 			});

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


!!! 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

jeeyyy added 2 commits August 29, 2018 15:03

fix: cssom of shadowRoot in safari

4929809

fix: remove redundant check in test

d29ff17

jeeyyy requested a review from a team as a code owner August 29, 2018 14:18

test: add tests for multiple style in shadowDOM

e14fd3e

WilcoFiers reviewed Aug 30, 2018

View reviewed changes

jeeyyy added 3 commits August 30, 2018 19:08

fix: handle stylesheet priority and shadowRoot CSSOM

aa73cd3

fix: introduce priority for stylesheets

d33527a

Merge branch 'develop' into fix-safari-shadow-cssom

63e1b53

jeeyyy requested review from stephenmathieson, isner and a team August 31, 2018 04:26

jeeyyy mentioned this pull request Sep 3, 2018

CSSOM: Safari not returning shadowRoot stylesheets (PR #1113) #1082

Closed

stephenmathieson reviewed Sep 4, 2018

View reviewed changes

fix: padding sheet index for priority compuation

018af6e

jeeyyy mentioned this pull request Sep 6, 2018

CSSOM: Handle Priority of styles applied (PR #1113) #1078

Closed

Merge branch 'develop' into fix-safari-shadow-cssom

f79d038

jeeyyy mentioned this pull request Sep 10, 2018

CSSOM: Tackle nested @import stylesheets #1054

Closed

3 tasks

WilcoFiers previously requested changes Sep 19, 2018

View reviewed changes

jeeyyy changed the title ~~fix: CSSOM generation for shadowRoot in Safari~~ [WIP] fix: CSSOM generation for shadowRoot in Safari Sep 19, 2018

jeeyyy self-assigned this Sep 21, 2018

jeeyyy added 2 commits September 26, 2018 09:32

Merge branch 'develop' into fix-safari-shadow-cssom

fce6eef

refactor: updates based on code review

74ee4a4

jeeyyy changed the title ~~[WIP] fix: CSSOM generation for shadowRoot in Safari~~ fix: CSSOM generation for shadowRoot in Safari Sep 26, 2018

Merge branch 'develop' into fix-safari-shadow-cssom

8987a4a

WilcoFiers reviewed Oct 3, 2018

View reviewed changes

WilcoFiers previously requested changes Oct 3, 2018

View reviewed changes

Merge branch 'develop' into fix-safari-shadow-cssom

b60839b

refactor: changes based on review comments

ae5e59b

stephenmathieson reviewed Oct 4, 2018

View reviewed changes

WilcoFiers approved these changes Oct 8, 2018

View reviewed changes

Merge branch 'develop' into fix-safari-shadow-cssom

e945b7f

jeeyyy merged commit a51ae03 into develop Oct 9, 2018

jeeyyy deleted the fix-safari-shadow-cssom branch October 9, 2018 09:19

Danidude pushed a commit to tingtun/axe-core that referenced this pull request Nov 1, 2018

fix: CSSOM generation for shadowRoot in Safari (dequelabs#1113)

93d959b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: CSSOM generation for shadowRoot in Safari #1113

fix: CSSOM generation for shadowRoot in Safari #1113

jeeyyy commented Aug 29, 2018 •

edited by WilcoFiers

Loading

WilcoFiers Aug 30, 2018

jeeyyy Sep 6, 2018

WilcoFiers Aug 30, 2018

jeeyyy Sep 26, 2018

WilcoFiers Aug 30, 2018

jeeyyy commented Aug 31, 2018

jeeyyy commented Sep 3, 2018

stephenmathieson Sep 4, 2018

jeeyyy Sep 5, 2018

WilcoFiers left a comment

WilcoFiers Sep 19, 2018

WilcoFiers Sep 19, 2018

WilcoFiers Sep 19, 2018

WilcoFiers Oct 3, 2018

WilcoFiers Oct 3, 2018 •

edited

Loading

WilcoFiers Oct 3, 2018

WilcoFiers Oct 3, 2018 •

edited

Loading

jeeyyy commented Oct 4, 2018

stephenmathieson Oct 4, 2018

WilcoFiers Oct 8, 2018

WilcoFiers left a comment

WilcoFiers Oct 8, 2018

WilcoFiers Oct 8, 2018

WilcoFiers Oct 8, 2018

fix: CSSOM generation for shadowRoot in Safari #1113

fix: CSSOM generation for shadowRoot in Safari #1113

Conversation

jeeyyy commented Aug 29, 2018 • edited by WilcoFiers 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

Choose a reason for hiding this comment

jeeyyy commented Aug 31, 2018

jeeyyy commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers left a 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

WilcoFiers Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

jeeyyy commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers left a 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

jeeyyy commented Aug 29, 2018 •

edited by WilcoFiers

Loading

WilcoFiers Oct 3, 2018 •

edited

Loading

WilcoFiers Oct 3, 2018 •

edited

Loading