Skip to content

fix(@angular-devkit/build-angular): handle load event for multiple stylesheets and CSP nonces #26941

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 24, 2024
Merged

fix(@angular-devkit/build-angular): handle load event for multiple stylesheets and CSP nonces #26941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d39c8b1
fix(@angular-devkit/build-angular): handle handle load event for mult…
alan-agius4 Jan 24, 2024
86ff051
fix(@angular/ssr): handle handle load event for multiple stylesheets …
alan-agius4 Jan 24, 2024
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
56 changes: 36 additions & 20 deletions packages/angular/ssr/src/inline-css-processor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,34 @@ const CSP_MEDIA_ATTR = 'ngCspMedia';

/**
* Script text used to change the media value of the link tags.
*
* NOTE:
* We do not use `document.querySelectorAll('link').forEach((s) => s.addEventListener('load', ...)`
* because this does not always fire on Chome.
* See: https://github.com/angular/angular-cli/issues/26932 and https://crbug.com/1521256
*/
const LINK_LOAD_SCRIPT_CONTENT = [
`(() => {`,
// Save the `children` in a variable since they're a live DOM node collection.
// We iterate over the direct descendants, instead of going through a `querySelectorAll`,
// because we know that the tags will be directly inside the `head`.
` const children = document.head.children;`,
// Declare `onLoad` outside the loop to avoid leaking memory.
// Can't be an arrow function, because we need `this` to refer to the DOM node.
` function onLoad() {this.media = this.getAttribute('${CSP_MEDIA_ATTR}');}`,
// Has to use a plain for loop, because some browsers don't support
// `forEach` on `children` which is a `HTMLCollection`.
` for (let i = 0; i < children.length; i++) {`,
` const child = children[i];`,
` child.hasAttribute('${CSP_MEDIA_ATTR}') && child.addEventListener('load', onLoad);`,
` }`,
`})();`,
'(() => {',
` const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';`,
' const documentElement = document.documentElement;',
' const listener = (e) => {',
' const target = e.target;',
` if (!target || target.tagName !== 'LINK' || !target.hasAttribute(CSP_MEDIA_ATTR)) {`,
' return;',
' }',

' target.media = target.getAttribute(CSP_MEDIA_ATTR);',
' target.removeAttribute(CSP_MEDIA_ATTR);',

// Remove onload listener when there are no longer styles that need to be loaded.
' if (!document.head.querySelector(`link[${CSP_MEDIA_ATTR}]`)) {',
` documentElement.removeEventListener('load', listener);`,
' }',
' };',

// We use an event with capturing (the true parameter) because load events don't bubble.
` documentElement.addEventListener('load', listener, true);`,
'})();',
].join('\n');

export interface InlineCriticalCssProcessOptions {
Expand All @@ -62,6 +73,7 @@ interface PartialHTMLElement {
hasAttribute(name: string): boolean;
removeAttribute(name: string): void;
appendChild(child: PartialHTMLElement): void;
insertBefore(newNode: PartialHTMLElement, referenceNode?: PartialHTMLElement): void;
remove(): void;
name: string;
textContent: string;
Expand Down Expand Up @@ -164,7 +176,7 @@ class CrittersExtended extends Critters {
// `addEventListener` to apply the media query instead.
link.removeAttribute('onload');
link.setAttribute(CSP_MEDIA_ATTR, crittersMedia[1]);
this.conditionallyInsertCspLoadingScript(document, cspNonce);
this.conditionallyInsertCspLoadingScript(document, cspNonce, link);
}

// Ideally we would hook in at the time Critters inserts the `style` tags, but there isn't
Expand Down Expand Up @@ -204,7 +216,11 @@ class CrittersExtended extends Critters {
* Inserts the `script` tag that swaps the critical CSS at runtime,
* if one hasn't been inserted into the document already.
*/
private conditionallyInsertCspLoadingScript(document: PartialDocument, nonce: string): void {
private conditionallyInsertCspLoadingScript(
document: PartialDocument,
nonce: string,
link: PartialHTMLElement,
): void {
if (this.addedCspScriptsDocuments.has(document)) {
return;
}
Expand All @@ -219,9 +235,9 @@ class CrittersExtended extends Critters {
const script = document.createElement('script');
script.setAttribute('nonce', nonce);
script.textContent = LINK_LOAD_SCRIPT_CONTENT;
// Append the script to the head since it needs to
// run as early as possible, after the `link` tags.
document.head.appendChild(script);
// Prepend the script to the head since it needs to
// run as early as possible, before the `link` tags.
document.head.insertBefore(script, link);
this.addedCspScriptsDocuments.add(document);
}
}
Expand Down
56 changes: 36 additions & 20 deletions packages/angular_devkit/build_angular/src/utils/index-file/inline-critical-css.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,34 @@ const CSP_MEDIA_ATTR = 'ngCspMedia';

/**
* Script text used to change the media value of the link tags.
*
* NOTE:
* We do not use `document.querySelectorAll('link').forEach((s) => s.addEventListener('load', ...)`
* because this does not always fire on Chome.
* See: https://github.com/angular/angular-cli/issues/26932 and https://crbug.com/1521256
*/
const LINK_LOAD_SCRIPT_CONTENT = [
`(() => {`,
// Save the `children` in a variable since they're a live DOM node collection.
// We iterate over the direct descendants, instead of going through a `querySelectorAll`,
// because we know that the tags will be directly inside the `head`.
` const children = document.head.children;`,
// Declare `onLoad` outside the loop to avoid leaking memory.
// Can't be an arrow function, because we need `this` to refer to the DOM node.
` function onLoad() {this.media = this.getAttribute('${CSP_MEDIA_ATTR}');}`,
// Has to use a plain for loop, because some browsers don't support
// `forEach` on `children` which is a `HTMLCollection`.
` for (let i = 0; i < children.length; i++) {`,
` const child = children[i];`,
` child.hasAttribute('${CSP_MEDIA_ATTR}') && child.addEventListener('load', onLoad);`,
` }`,
`})();`,
'(() => {',
` const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';`,
' const documentElement = document.documentElement;',
' const listener = (e) => {',
' const target = e.target;',
` if (!target || target.tagName !== 'LINK' || !target.hasAttribute(CSP_MEDIA_ATTR)) {`,
' return;',
' }',

' target.media = target.getAttribute(CSP_MEDIA_ATTR);',
' target.removeAttribute(CSP_MEDIA_ATTR);',

// Remove onload listener when there are no longer styles that need to be loaded.
' if (!document.head.querySelector(`link[${CSP_MEDIA_ATTR}]`)) {',
` documentElement.removeEventListener('load', listener);`,
' }',
' };',

// We use an event with capturing (the true parameter) because load events don't bubble.
` documentElement.addEventListener('load', listener, true);`,
'})();',
].join('\n');

export interface InlineCriticalCssProcessOptions {
Expand All @@ -57,6 +68,7 @@ interface PartialHTMLElement {
hasAttribute(name: string): boolean;
removeAttribute(name: string): void;
appendChild(child: PartialHTMLElement): void;
insertBefore(newNode: PartialHTMLElement, referenceNode?: PartialHTMLElement): void;
remove(): void;
name: string;
textContent: string;
Expand Down Expand Up @@ -155,7 +167,7 @@ class CrittersExtended extends Critters {
// `addEventListener` to apply the media query instead.
link.removeAttribute('onload');
link.setAttribute(CSP_MEDIA_ATTR, crittersMedia[1]);
this.conditionallyInsertCspLoadingScript(document, cspNonce);
this.conditionallyInsertCspLoadingScript(document, cspNonce, link);
}

// Ideally we would hook in at the time Critters inserts the `style` tags, but there isn't
Expand Down Expand Up @@ -195,17 +207,21 @@ class CrittersExtended extends Critters {
* Inserts the `script` tag that swaps the critical CSS at runtime,
* if one hasn't been inserted into the document already.
*/
private conditionallyInsertCspLoadingScript(document: PartialDocument, nonce: string): void {
private conditionallyInsertCspLoadingScript(
document: PartialDocument,
nonce: string,
link: PartialHTMLElement,
): void {
if (this.addedCspScriptsDocuments.has(document)) {
return;
}

const script = document.createElement('script');
script.setAttribute('nonce', nonce);
script.textContent = LINK_LOAD_SCRIPT_CONTENT;
// Append the script to the head since it needs to
// run as early as possible, after the `link` tags.
document.head.appendChild(script);
// Prepend the script to the head since it needs to
// run as early as possible, before the `link` tags.
document.head.insertBefore(script, link);
this.addedCspScriptsDocuments.add(document);
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/legacy-cli/e2e/tests/build/ssr/express-engine-csp-nonce.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export default async function () {
by.css('link[rel="stylesheet"]')
);
expect(await linkTag.getAttribute('media')).toMatch('all');
expect(await linkTag.getAttribute('ngCspMedia')).toMatch('all');
expect(await linkTag.getAttribute('ngCspMedia')).toBeNull();
expect(await linkTag.getAttribute('onload')).toBeNull();

// Make sure there were no client side errors.
Expand Down