Skip to content

Commit eef1aa8

Browse files
committed
fix(@angular/ssr): handle handle load event for multiple stylesheets and CSP nonces
The `load` event for each stylesheet may not always be triggered by Google Chrome's handling. Refer to: https://crbug.com/1521256 This results in the media attribute persistently being set to print, leading to distorted styles in the UI. To address this issue, we substitute the onload logic by replacing `link.addEventListener('load', ...` with `document.documentElement.addEventListener('load', ...` and filtering for link tags. Closes #26932
1 parent c93ea15 commit eef1aa8

File tree

2 files changed

+37
-21
lines changed

2 files changed

+37
-21
lines changed

packages/angular/ssr/src/inline-css-processor.ts

Lines changed: 36 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,23 +21,34 @@ const CSP_MEDIA_ATTR = 'ngCspMedia';
2121

2222
/**
2323
* Script text used to change the media value of the link tags.
24+
*
25+
* NOTE:
26+
* We do not use `document.querySelectorAll('link').forEach((s) => s.addEventListener('load', ...)`
27+
* because this does not always fire on Chome.
28+
* See: https://github.com/angular/angular-cli/issues/26932 and https://crbug.com/1521256
2429
*/
2530
const LINK_LOAD_SCRIPT_CONTENT = [
26-
`(() => {`,
27-
// Save the `children` in a variable since they're a live DOM node collection.
28-
// We iterate over the direct descendants, instead of going through a `querySelectorAll`,
29-
// because we know that the tags will be directly inside the `head`.
30-
` const children = document.head.children;`,
31-
// Declare `onLoad` outside the loop to avoid leaking memory.
32-
// Can't be an arrow function, because we need `this` to refer to the DOM node.
33-
` function onLoad() {this.media = this.getAttribute('${CSP_MEDIA_ATTR}');}`,
34-
// Has to use a plain for loop, because some browsers don't support
35-
// `forEach` on `children` which is a `HTMLCollection`.
36-
` for (let i = 0; i < children.length; i++) {`,
37-
` const child = children[i];`,
38-
` child.hasAttribute('${CSP_MEDIA_ATTR}') && child.addEventListener('load', onLoad);`,
39-
` }`,
40-
`})();`,
31+
'(() => {',
32+
` const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';`,
33+
' const documentElement = document.documentElement;',
34+
' const listener = (e) => {',
35+
' const target = e.target;',
36+
` if (!target || target.tagName !== 'LINK' || !target.hasAttribute(CSP_MEDIA_ATTR)) {`,
37+
' return;',
38+
' }',
39+
40+
' target.media = target.getAttribute(CSP_MEDIA_ATTR);',
41+
' target.removeAttribute(CSP_MEDIA_ATTR);',
42+
43+
// Remove onload listener when there are no longer styles that need to be loaded.
44+
' if (!document.head.querySelector(`link[${CSP_MEDIA_ATTR}]`)) {',
45+
` documentElement.removeEventListener('load', listener);`,
46+
' }',
47+
' };',
48+
49+
// We use an event with capturing (the true parameter) because load events don't bubble.
50+
` documentElement.addEventListener('load', listener, true);`,
51+
'})();',
4152
].join('\n');
4253

4354
export interface InlineCriticalCssProcessOptions {
@@ -62,6 +73,7 @@ interface PartialHTMLElement {
6273
hasAttribute(name: string): boolean;
6374
removeAttribute(name: string): void;
6475
appendChild(child: PartialHTMLElement): void;
76+
insertBefore(newNode: PartialHTMLElement, referenceNode?: PartialHTMLElement): void;
6577
remove(): void;
6678
name: string;
6779
textContent: string;
@@ -164,7 +176,7 @@ class CrittersExtended extends Critters {
164176
// `addEventListener` to apply the media query instead.
165177
link.removeAttribute('onload');
166178
link.setAttribute(CSP_MEDIA_ATTR, crittersMedia[1]);
167-
this.conditionallyInsertCspLoadingScript(document, cspNonce);
179+
this.conditionallyInsertCspLoadingScript(document, cspNonce, link);
168180
}
169181

170182
// Ideally we would hook in at the time Critters inserts the `style` tags, but there isn't
@@ -204,7 +216,11 @@ class CrittersExtended extends Critters {
204216
* Inserts the `script` tag that swaps the critical CSS at runtime,
205217
* if one hasn't been inserted into the document already.
206218
*/
207-
private conditionallyInsertCspLoadingScript(document: PartialDocument, nonce: string): void {
219+
private conditionallyInsertCspLoadingScript(
220+
document: PartialDocument,
221+
nonce: string,
222+
link: PartialHTMLElement,
223+
): void {
208224
if (this.addedCspScriptsDocuments.has(document)) {
209225
return;
210226
}
@@ -219,9 +235,9 @@ class CrittersExtended extends Critters {
219235
const script = document.createElement('script');
220236
script.setAttribute('nonce', nonce);
221237
script.textContent = LINK_LOAD_SCRIPT_CONTENT;
222-
// Append the script to the head since it needs to
223-
// run as early as possible, after the `link` tags.
224-
document.head.appendChild(script);
238+
// Prepend the script to the head since it needs to
239+
// run as early as possible, before the `link` tags.
240+
document.head.insertBefore(script, link);
225241
this.addedCspScriptsDocuments.add(document);
226242
}
227243
}

tests/legacy-cli/e2e/tests/build/ssr/express-engine-csp-nonce.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ export default async function () {
111111
by.css('link[rel="stylesheet"]')
112112
);
113113
expect(await linkTag.getAttribute('media')).toMatch('all');
114-
expect(await linkTag.getAttribute('ngCspMedia')).toMatch('all');
114+
expect(await linkTag.getAttribute('ngCspMedia')).toBeNull();
115115
expect(await linkTag.getAttribute('onload')).toBeNull();
116116
117117
// Make sure there were no client side errors.

0 commit comments

Comments
 (0)