Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
12 changes: 12 additions & 0 deletions goldens/public-api/platform-browser/index.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ export class By {
// @public
export function createApplication(options?: ApplicationConfig, context?: BootstrapContext): Promise<ApplicationRef>;

// @public
export class CssVarNamespacer {
namespace(name: string): string;
// (undocumented)
static ɵfac: i0.ɵɵFactoryDeclaration<CssVarNamespacer, never>;
// (undocumented)
static ɵprov: i0.ɵɵInjectableDeclaration<CssVarNamespacer>;
}

// @public
export function disableDebugTools(): void;

Expand Down Expand Up @@ -198,6 +207,9 @@ export const platformBrowser: (extraProviders?: StaticProvider[]) => PlatformRef
// @public
export function provideClientHydration(...features: HydrationFeature<HydrationFeatureKind>[]): EnvironmentProviders;

// @public
export function provideCssVarNamespacing(namespace: string): EnvironmentProviders;

// @public
export function provideProtractorTestingSupport(): Provider[];

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions packages/compiler/src/render3/view/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import * as core from '../../core';
import {CssSelector} from '../../directive_matching';
import * as o from '../../output/output_ast';
import {ParseError, ParseSourceSpan} from '../../parse_util';
import {ShadowCss} from '../../shadow_css';
import {namespaceCssVariables, ShadowCss} from '../../shadow_css';
import {CompilationJobKind, TemplateCompilationMode} from '../../template/pipeline/src/compilation';
import {emitHostBindingFunction, emitTemplateFn, transform} from '../../template/pipeline/src/emit';
import {ingestComponent, ingestHostBinding} from '../../template/pipeline/src/ingest';
Expand Down Expand Up @@ -300,10 +300,11 @@ export function compileComponentFromMetadata(
let hasStyles = !!meta.externalStyles?.length;
// e.g. `styles: [str1, str2]`
if (meta.styles && meta.styles.length) {
const namespacedStyles = meta.styles.map((s) => namespaceCssVariables(s));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apologies if we've talked about this already and I'm forgetting, but have we considered making this a build-time transform rather than compile-time? Could we even just generate a purely unique ID at build time and use it for the entire app, then skip the runtime replacement with %NS% entirely?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

have we considered making this a build-time transform rather than compile-time

I assume you mean "rather than runtime"? I did look into this, and it would probably work in the 3P environment. However, in 1P we have a few extra constraints.

  1. Changing variable names is a breaking change (hence the need for DISABLE_NAMESPACING), so we can't enable this unconditionally, we need some kind of opt-in, whether that's a compiler flag or a runtime flag.
  2. The namespace may not be known at build time. In the MicA case, app ID is probably known, but binary ID is not known until the release is published, and that would probably be needed to be truly isolated.
  3. We're generally limited to having a single set of build flags throughout g3. That's not strictly true, but making this conditional means that you either need to say enable_css_namepsacing = "my-app" on every ng_module (which would break libraries used by multiple apps who can't choose a single namespace) or you need a Bazel transition and the build story gets a lot more complicated.

I do agree with the general desire that this should be done at build time, but practically speaking that makes the feature much more complicated and I think it would be unlikely to be feasible in google3 based on the way our build tooling works. I'm inclined to start with a runtime transform and then potentially switch to a build-time transform later if/when we have a clearer need and path towards it in google3.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume you mean "rather than runtime"?

👍

Changing variable names is a breaking change (hence the need for DISABLE_NAMESPACING), so we can't enable this unconditionally, we need some kind of opt-in, whether that's a compiler flag or a runtime flag.

No dispute with the need for a flag.

The namespace may not be known at build time. In the MicA case, app ID is probably known, but binary ID is not known until the release is published, and that would probably be needed to be truly isolated.

Does the namespace need to be the app/binary ID, or is any ID sufficient so long as it's unique to a single build of the app? Maybe we could just use an ID we generate at build time?

you need a Bazel transition and the build story gets a lot more complicated

I agree that this is the tricky part. I'm not sure how we'd define a custom ID in g3 that would be available across the whole build. You're much more knowledgable here than I am, so I trust your thoughts on it.

I'm inclined to start with a runtime transform

Seems totally reasonable to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does the namespace need to be the app/binary ID, or is any ID sufficient so long as it's unique to a single build of the app? Maybe we could just use an ID we generate at build time?

MicA has to account for SSR-ing two versions of the same application at the same time while in the middle of a deployment (one request happens to go to app version X, another request happens to go app version X + 1). In that scenario, I don't think any information known at build time would be sufficient. That's a very MicA-specific concern, but it's the constraints we're working within.

I agree that this is the tricky part. I'm not sure how we'd define a custom ID in g3 that would be available across the whole build. You're much more knowledgable here than I am, so I trust your thoughts on it.

I forgot to link to the relevant part of the internal doc which discussed this. A transition is definitely plausible (we did it for Ivy), but also very much complexity we'd rather avoid. If we receive sufficient push back from g3 customers, then that's a sign that the performance impact is significant enough to consider re-evaluating the complexity here.

const styleValues =
meta.encapsulation == core.ViewEncapsulation.Emulated
? compileStyles(meta.styles, CONTENT_ATTR, HOST_ATTR)
: meta.styles;
? compileStyles(namespacedStyles, CONTENT_ATTR, HOST_ATTR)
: namespacedStyles;
const styleNodes = styleValues.reduce((result, style) => {
if (style.trim().length > 0) {
result.push(constantPool.getConstLiteral(o.literal(style)));
Expand Down
23 changes: 23 additions & 0 deletions packages/compiler/src/shadow_css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1130,13 +1130,36 @@ const _cssCommaInPlaceholderReGlobal = new RegExp(COMMA_IN_PLACEHOLDER, 'g');
const _cssSemiInPlaceholderReGlobal = new RegExp(SEMI_IN_PLACEHOLDER, 'g');
const _cssColonInPlaceholderReGlobal = new RegExp(COLON_IN_PLACEHOLDER, 'g');

// Matches `/* DISABLE_NAMESPACING */` optionally followed by any characters
// that are not a semicolon or closing brace (e.g. whitespace or a property name),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Semicolon, closing brace, or opening brace.

// up until it hits a CSS variable `--some-name`.
const _cssNamespaceRe = new RegExp(
String.raw`(?:(/\*\s*DISABLE_NAMESPACING\s*\*/)[^;{}]*?)?(--[a-zA-Z0-9_-]+)`,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Voicing my preference for --global-some-name one more time. I haven't come up with an example of how this regex might break, but it just feels brittle to me to be scanning the CSS file in this way. With --global-foovar/ we have a very clear picture of what to search for.

I also think using a comment as a pragma will make big blocks of variable references uglier if every other line requires a comment node.

This certainly isn't a hill I'll die on and I understand the concern of having variable references not syntactically matching their declaration (if declared in a global stylesheet), but I feel like the --global- prefix makes more sense to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, you're envisioning that --some-name would get renamed to --%NS%some-name while --global-some-name would get renamed to --some-name?

I don't mind the global-* prefix as an indicator of opt-out, but I do think it's confusing that we're "not namespacing" the value while still renaming it. If we left it alone as --global-some-name, then that would be intuitive to me, but that requires all affected variables to be able to be renamed, which I don't think is a safe assumption given many libraries define their own variables like --primary-color without thinking through this use case.

This can also be confusing if I want to find usages of --primary-color and grep for --primary-color and conclude it's not used only to later find that --global-primary-color was used in an Angular app somewhere.

I agree /* DISABLE_NAMESPACING */ is awkward, but I think the benefit of it is that it still allows you to use the intended name variable name directly in source, which is useful for clarity and "Find references"-style use cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to run this by the broader team just to get general opinions on the syntax.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Totally hear you on the concern for tooling. I'm definitely not in love with either approach, and I'm happy to go with whatever the team prefers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Closing the loop here, turns out /* DISABLE_NAMESPACING */ isn't viable because Sass generally wants you to use // DISABLE_NAMESPACING comments which then get elided by the compiler. It's possible to keep a comment like this, but you're generally fighting against the tooling rather than working with it.

We considered a few options, but ultimately landed on --global--* as the best immediate option, so it looks like:

:host {
  --global--foo: red;

  color: var(--global--foo);
}
  • This aligns with :global selector syntax most frameworks use and -global-* syntax in particular used by Svelte for CSS names.
  • It's short and straightforward.
  • It's local to the usage site of the symbol (no "spooky action at a distance").
  • The extra -- between the prefix and variable name makes this still searchable. So grepping for --foo will still find --global--foo.
  • We'll emit an error for --global-foo as that seems like a very easy mistake to make.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI, attribute and class names can contain --. I'm not sure how much this is used in practice or if there are other , but this will break selectors like [--foo] and .--foo. Maybe this is also a problem in keyframes names?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AGENT: The current regex (--[a-zA-Z0-9_-]+) will match -- anywhere in the CSS string, not just at the start of a variable declaration or var() call. This means it will incorrectly match and replace BEM style CSS classes (e.g., .block--modifier becomes .block--%NS%modifier which breaks the styles). We should ensure the -- is bounded appropriately (e.g., not preceded by another word character or hyphen). Also, adding a test case in shadow_css_spec.ts for classes with -- in the middle would be a great way to prevent this from regressing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feels unlikely to be a big problem imo. One more reason I'm investigating using a real parser, though 👀

'g',
);

export class CssRule {
constructor(
public selector: string,
public content: string,
) {}
}

/**
* Transforms CSS variables within a stylesheet to include a namespace placeholder.
*
* E.g. `--foo: bar;` becomes `--%NS%foo: bar;`
* E.g. `color: var(--foo);` becomes `color: var(--%NS%foo);`
*
* If a declaration or usage is preceded by `/* DISABLE_NAMESPACING *\/`, it is NOT transformed.
*/
export function namespaceCssVariables(cssText: string): string {
return cssText.replace(_cssNamespaceRe, (match, disableComment, varName) => {
if (disableComment) return match;
return `--%NS%${varName.substring('--'.length)}`;
Comment thread
dgp1130 marked this conversation as resolved.
Resolved
});
}

export function processRules(input: string, ruleCallback: (rule: CssRule) => CssRule): string {
const escaped = escapeInStrings(input);
const inputWithEscapedBlocks = escapeBlocks(escaped, CONTENT_PAIRS, BLOCK_PLACEHOLDER);
Expand Down
88 changes: 88 additions & 0 deletions packages/compiler/test/shadow_css/shadow_css_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {namespaceCssVariables} from '../../src/shadow_css';
import {shim} from './utils';

describe('ShadowCss', () => {
Expand Down Expand Up @@ -397,4 +398,91 @@ describe('ShadowCss', () => {
);
});
});

describe('CSS variable namespacing', () => {
it('should inject `%NS%` placeholder into CSS variable declarations and usages', () => {
const input = `
.foo {
--my-color: red;
color: var(--my-color, blue);
}
`.trim();

const expected = `
.foo {
--%NS%my-color: red;
color: var(--%NS%my-color, blue);
}
`.trim();

expect(namespaceCssVariables(input)).toEqualCss(expected);
});

it('should not inject `%NS%` when `/* DISABLE_NAMESPACING */` comment is present', () => {
const input = `
.foo {
/* DISABLE_NAMESPACING */
--my-color: green;
background: var(
/* DISABLE_NAMESPACING */
--my-color
);
}
`.trim();

const expected = `
.foo {
/* DISABLE_NAMESPACING */
--my-color: green;
background: var(
/* DISABLE_NAMESPACING */
--my-color
);
}
`.trim();

expect(namespaceCssVariables(input)).toEqualCss(expected);
});
it('should not inject `%NS%` when `/* DISABLE_NAMESPACING */` comment is present on the property declaration', () => {
const input = `
.foo {
/* DISABLE_NAMESPACING */
color: var(--my-color, blue);
}
`.trim();

const expected = `
.foo {
/* DISABLE_NAMESPACING */
color: var(--my-color, blue);
}
`.trim();

expect(namespaceCssVariables(input)).toEqualCss(expected);
});

it('should correctly handle multiple `var()` functions with isolated `/* DISABLE_NAMESPACING */` comments', () => {
const input = `
.foo {
border: var(/* DISABLE_NAMESPACING */ --border-size) solid var(--border-color);
box-shadow:
var(--shadow-1),
var(/* DISABLE_NAMESPACING */ --shadow-2),
var(--shadow-3);
}
`.trim();

const expected = `
.foo {
border: var(/* DISABLE_NAMESPACING */ --border-size) solid var(--%NS%border-color);
box-shadow:
var(--%NS%shadow-1),
var(/* DISABLE_NAMESPACING */ --shadow-2),
var(--%NS%shadow-3);
}
`.trim();

expect(namespaceCssVariables(input)).toEqualCss(expected);
});
});
});
2 changes: 1 addition & 1 deletion packages/core/test/acceptance/csp_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('CSP integration', () => {
for (let i = 0; i < styles.length; i++) {
const style = styles[i];
const nonce = style.getAttribute('nonce');
if (nonce && style.textContent?.includes('--csp-test-var')) {
if (nonce && style.textContent?.includes('csp-test-var')) {
nonces.push(nonce);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
"CONTENT_ATTR",
"CONTEXT",
"CSP_NONCE",
"CSS_VAR_NAMESPACE",
"ChainedInjector",
"ChangeDetectionScheduler",
"ChangeDetectionSchedulerImpl",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"CONTENT_ATTR",
"CONTEXT",
"CSP_NONCE",
"CSS_VAR_NAMESPACE",
"ChainedInjector",
"ChangeDetectionScheduler",
"ChangeDetectionSchedulerImpl",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"COMPONENT_REGEX",
"COMPONENT_VARIABLE",
"CONTENT_ATTR",
"CSS_VAR_NAMESPACE",
"DefaultDomRenderer2",
"DomAdapter",
"DomEventsPlugin",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"CONTENT_ATTR",
"CONTEXT",
"CSP_NONCE",
"CSS_VAR_NAMESPACE",
"ChainedInjector",
"ChangeDetectionScheduler",
"ChangeDetectionSchedulerImpl",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"CONTENT_ATTR",
"CONTEXT",
"CSP_NONCE",
"CSS_VAR_NAMESPACE",
"ChainedInjector",
"ChangeDetectionScheduler",
"ChangeDetectionSchedulerImpl",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"CONTENT_ATTR",
"CONTEXT",
"CSP_NONCE",
"CSS_VAR_NAMESPACE",
"ChainedInjector",
"ChangeDetectionScheduler",
"ChangeDetectionSchedulerImpl",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"CONTEXT",
"CREATE_VIEW_TRANSITION",
"CSP_NONCE",
"CSS_VAR_NAMESPACE",
"CanActivate",
"CanDeactivate",
"ChainedInjector",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"CONTENT_ATTR",
"CONTEXT",
"CSP_NONCE",
"CSS_VAR_NAMESPACE",
"ChainedInjector",
"ChangeDetectionScheduler",
"ChangeDetectionSchedulerImpl",
Expand Down
47 changes: 47 additions & 0 deletions packages/platform-browser/src/dom/css_var_namespacer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.dev/license
*/

import {inject, Injectable} from '@angular/core';
import {CSS_VAR_NAMESPACE} from './dom_renderer';

/**
* A service that can be used to manually namespace CSS variable names at runtime.
* This is useful when reading or setting CSS variables dynamically in JavaScript that
* were transformed by the compiler during the build.
*
* @publicApi
*/
@Injectable({providedIn: 'root'})
export class CssVarNamespacer {
private readonly namespacePrefix = inject(CSS_VAR_NAMESPACE, {optional: true}) ?? '';

/**
* Prepends the namespace prefix to a CSS variable name.
*
* @param name The CSS variable name to namespace, including the leading `--`.
* @returns The namespaced CSS variable name, including the leading `--`. Returns the input
* unchanged if no namespace is configured.
*/
namespace(name: string): string {
// Validate that the whole `--foo` variable is passed in.
if (typeof ngDevMode === 'undefined' || ngDevMode) {
if (!name.startsWith('--')) {
throw new Error(
`CSS variable names passed to \`CssVarNamespacer\` must start with '--', got: '${name}'`,
);
}
}

// We want to support libraries which might be used by applications which do and don't
// namespace variables. Therefore the library always needs to use `CssVarNamespacer`, even
// though the application may not actually be namespacing anything.
if (!this.namespacePrefix) return name;

return `--${this.namespacePrefix}${name.substring('--'.length)}`;
}
}
Loading