Skip to content
Open
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
4 changes: 4 additions & 0 deletions packages/core/src/core_private_export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ export {
DeferBlockState as ɵDeferBlockState,
} from './defer/interfaces';
export {getDocument as ɵgetDocument} from './render3/interfaces/document';
export {
SHARED_STYLES_HOST as ɵSHARED_STYLES_HOST,
SharedStylesHost as ɵSharedStylesHost,
} from './render3/interfaces/shared_styles_host';
export {
convertToBitFlags as ɵconvertToBitFlags,
setCurrentInjector as ɵsetCurrentInjector,
Expand Down
35 changes: 35 additions & 0 deletions packages/core/src/render3/component_ref.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 {DOCUMENT} from '../document';
import {setActiveConsumer} from '../../primitives/signals';

import {ChangeDetectorRef} from '../change_detection/change_detector_ref';
Expand All @@ -17,6 +18,7 @@ import {Injector} from '../di/injector';
import {EnvironmentInjector} from '../di/r3_injector';
import {RuntimeError, RuntimeErrorCode} from '../errors';
import {Type} from '../interface/type';
import {ViewEncapsulation} from '../metadata/view';
import {
ComponentFactory as AbstractComponentFactory,
ComponentRef as AbstractComponentRef,
Expand Down Expand Up @@ -59,7 +61,11 @@ import {
TView,
TVIEW,
TViewType,
ENVIRONMENT,
ON_DESTROY_HOOKS,
} from './interfaces/view';
import {SHARED_STYLES_HOST} from './interfaces/shared_styles_host';
import {getDocument} from './interfaces/document';
import {MATH_ML_NAMESPACE, SVG_NAMESPACE} from './namespaces';

import {retrieveHydrationInfo} from '../hydration/utils';
Expand Down Expand Up @@ -178,12 +184,17 @@ function createRootLViewEnvironment(rootLViewInjector: Injector): LViewEnvironme
ngReflect = rootLViewInjector.get(NG_REFLECT_ATTRS_FLAG, NG_REFLECT_ATTRS_FLAG_DEFAULT);
}

const sharedStylesHost = rootLViewInjector.get(SHARED_STYLES_HOST, null);
const fallbackDocument = rootLViewInjector.get(DOCUMENT, getDocument());

return {
rendererFactory,
sanitizer,
changeDetectionScheduler,
ngReflect,
tracingService,
sharedStylesHost,
fallbackDocument,
};
}

Expand Down Expand Up @@ -338,6 +349,30 @@ export class ComponentFactory<T> extends AbstractComponentFactory<T> {

rootLView[HEADER_OFFSET] = hostElement;

// Determine if the component itself uses Shadow DOM, as these components will track their own
// shadow roots when they are created.
const usesShadowDom =
(cmpDef.encapsulation === ViewEncapsulation.ShadowDom ||
cmpDef.encapsulation === ViewEncapsulation.ExperimentalIsolatedShadowDom) &&
!(typeof ngServerMode !== 'undefined' && ngServerMode);
if (!usesShadowDom) {
const sharedStylesHost = rootLView[ENVIRONMENT].sharedStylesHost;
if (sharedStylesHost) {
// Check the root node (containing shadow root or document) and provide stylesheets there.
// If the created component is detached from the document, this will be `null` and stylesheets
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.

Will this be null or will it be the hostElement itself? Looking at the getRootNode docs on MDN, I'd assume it'd return the element itself?

// will be tracked once the component is attached to the document.
const rootNode = hostElement.getRootNode?.();
const isShadowRoot =
rootNode && typeof ShadowRoot !== 'undefined' && rootNode instanceof ShadowRoot;
const styleHost = isShadowRoot ? rootNode : rootLView[ENVIRONMENT].fallbackDocument.head;

sharedStylesHost.addHost(styleHost);
(rootLView[ON_DESTROY_HOOKS] ??= []).push(
() => void sharedStylesHost.removeHost(styleHost),
);
}
}

// rootView is the parent when bootstrapping
// TODO(misko): it looks like we are entering view here but we don't really need to as
// `renderView` does that. However as the code is written it is needed because
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/render3/interfaces/renderer_dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ export interface RNode {
* Used exclusively for building up DOM which are static (ie not View roots)
*/
appendChild(newChild: RNode): RNode;

/**
* Returns the root node (Document or ShadowRoot) of this node.
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.

Nitpicking, but this could return a node if called on an unattached node.

*
* @see https://developer.mozilla.org/en-US/docs/Web/API/Node/getRootNode
*/
getRootNode?(): RNode | ShadowRoot | null;
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.

When does this return null?

}

/**
Expand Down
47 changes: 47 additions & 0 deletions packages/core/src/render3/interfaces/shared_styles_host.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 {InjectionToken} from '../../di/injection_token';

/** Token used to retrieve the `SharedStylesHost`. */
export const SHARED_STYLES_HOST = new InjectionToken<SharedStylesHost>(
typeof ngDevMode !== 'undefined' && ngDevMode ? 'SHARED_STYLES_HOST' : '',
);

/** Manages stylesheets for components in the application. */
export interface SharedStylesHost {
/**
* Adds embedded styles to the DOM via HTML `style` elements.
* @param styles An array of style content strings.
* @param urls An array of URLs to be added as link tags.
*/
addStyles(styles: string[], urls?: string[]): void;

/**
* Removes embedded styles from the DOM that were added as HTML `style` elements.
* @param styles An array of style content strings.
* @param urls An array of URLs to be removed as link tags.
*/
removeStyles(styles: string[], urls?: string[]): void;

/**
* Adds a host node to contain styles added to the DOM and adds all existing style usage to
* the newly added host node.
*
* @param hostNode The node to contain styles added to the DOM.
*/
addHost(hostNode: Node): void;

/**
* Removes a host node from the set of style hosts and removes all existing style usage from
* the removed host node.
*
* @param hostNode The node to remove from the set of style hosts.
*/
removeHost(hostNode: Node): void;
}
20 changes: 20 additions & 0 deletions packages/core/src/render3/interfaces/view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {TConstants, TNode} from './node';
import type {LQueries, TQueries} from './query';
import {Renderer, RendererFactory} from './renderer';
import {RElement} from './renderer_dom';
import {SharedStylesHost} from './shared_styles_host';
import {TStylingKey, TStylingRange} from './styling';

// Below are constants for LView indices to help us look up LView members
Expand Down Expand Up @@ -396,6 +397,25 @@ export interface LViewEnvironment {
* (always disabled in prod mode).
*/
ngReflect: boolean;

/**
* `SharedStylesHost` for managing shared styles within the application.
*
* This property is optional because Angular's core rendering engine is platform-agnostic.
* Custom platforms (e.g., rendering to a terminal or canvas) that do not integrate with
* HTML stylesheets may choose not to provide a `SharedStylesHost`.
*/
sharedStylesHost: SharedStylesHost | null;

/**
* The fallback document used as the style host when a specific host (like a `ShadowRoot`)
* is not applicable.
*
* This node is captured at bootstrap to ensure document context parity. This is particularly
* important in environments with mocked documents (like unit tests), which provide the
* `DOCUMENT` token but don't call `setDocument`.
*/
fallbackDocument: Document;
}

/** Flags associated with an LView (saved in LView[FLAGS]) */
Expand Down
27 changes: 27 additions & 0 deletions packages/core/src/render3/view/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ import {
T_HOST,
TView,
TVIEW,
HOST,
ENVIRONMENT,
} from '../interfaces/view';
import {getDocument} from '../interfaces/document';
import {
addViewToDOM,
destroyLView,
Expand Down Expand Up @@ -116,6 +119,16 @@ export function addLViewToLContainer(
}
}

// To ensure styles are placed on a parent shadow root, we need to register it as a host.
const sharedStylesHost = lView[ENVIRONMENT].sharedStylesHost;
if (sharedStylesHost) {
const host = lView[HOST] ?? lContainer[NATIVE];
const rootNode = host.getRootNode?.();
const isShadowRoot =
rootNode && typeof ShadowRoot !== 'undefined' && rootNode instanceof ShadowRoot;
sharedStylesHost.addHost(isShadowRoot ? rootNode : lView[ENVIRONMENT].fallbackDocument.head);
}

// When in hydration mode, reset the pointer to the first child in
// the dehydrated view. This indicates that the view was hydrated and
// further attaching/detaching should work with this view as normal.
Expand Down Expand Up @@ -153,6 +166,20 @@ export function detachView(lContainer: LContainer, removeIndex: number): LView |
const viewToDetach = lContainer[indexInContainer];

if (viewToDetach) {
const host = viewToDetach[HOST] ?? lContainer[NATIVE];
if (host.isConnected) {
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: addLViewToLContainer adds the host without checking isConnected. Guarding the removal with if (host.isConnected) can lead to a state imbalance (e.g., if the user manually removes the element from the DOM before destroying it, or if it is created/destroyed entirely while detached). In such scenarios, removeHost is skipped, leaving the host in SharedStylesHost's map and leaking the ShadowRoot and its associated styles. Can we avoid checking isConnected here or reliably capture the original host to remove it unconditionally?

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.

Maybe we can keep a WeakMap of LView->Node and just grab the host that way?

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.

addLViewToLContainer adds the host without checking isConnected.

We don't check isConnected because we call addViewToDOM immediately before that, so I think it's fair to assume the host will always be connected at that time. Gemini claims that addToDOM = false is only used specifically for hydration when the DOM is already rendered, so that should be fine.

There is an if (parentRNode !== null) condition which could cause this situation, but I'm not clear on when that would ever happen practically speaking. If it does, then I think that's a deeper bug, since skipping adding styles here would mean we're rendering an unstyled component. It's a potential bug in addLViewToLContainer, not detachView.

I think I'll add an assertion here that host.isConnected === true. If TGP ever emits that error, then there's a potentially deeper bug worth investigating than isConnected symmetry.

Guarding the removal with if (host.isConnected) can lead to a state imbalance (e.g., if the user manually removes the element from the DOM before destroying it, or if it is created/destroyed entirely while detached).

In what scenario would this happen? If the user is manually grabbing an element and removing it from the DOM without going through Angular, I feel like a lot of other things would break before they started caring about leaking styles. Is this a supported use cases elsewhere?

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.

@dgp1130 People do manual DOM manipulation a lot more often than we'd like, unfortunately.

const sharedStylesHost = viewToDetach[ENVIRONMENT].sharedStylesHost;
if (sharedStylesHost) {
// Undo the `SharedStylesHost` registration.
const rootNode = host.getRootNode?.();
const isShadowRoot =
rootNode && typeof ShadowRoot !== 'undefined' && rootNode instanceof ShadowRoot;
sharedStylesHost.removeHost(
isShadowRoot ? rootNode : viewToDetach[ENVIRONMENT].fallbackDocument.head,
);
}
}

const declarationLContainer = viewToDetach[DECLARATION_LCONTAINER];
if (declarationLContainer !== null && declarationLContainer !== lContainer) {
detachMovedView(declarationLContainer, viewToDetach);
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/render3/view_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ export class ViewRef<T> implements EmbeddedViewRef<T>, ChangeDetectorRef {
}

destroy(): void {
if (this.destroyed) return;

if (this._appRef) {
this._appRef.detachView(this);
} else if (this._attachedToViewContainer) {
Expand Down
54 changes: 53 additions & 1 deletion packages/core/test/acceptance/view_container_ref_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
RendererFactory2,
RendererType2,
Sanitizer,
ɵSHARED_STYLES_HOST as SHARED_STYLES_HOST,
signal,
TemplateRef,
ViewChild,
Expand All @@ -50,6 +51,7 @@ import {
ChangeDetectionStrategy,
} from '../../src/core';
import {ComponentFixture, TestBed, TestComponentRenderer} from '../../testing';
import {MockSharedStylesHost} from '../../testing/src/mock_shared_styles_host';

describe('ViewContainerRef', () => {
/**
Expand Down Expand Up @@ -730,6 +732,46 @@ describe('ViewContainerRef', () => {
});
});

describe('destroy', () => {
// NOTE: We may not necessarily _want_ this to be the case, but it does seem to be necessary
// for some amount of Angular code out there. We may want to consider lifting this constraint.
it('should not throw if the injector is destroyed before a view is removed', () => {
@Component({
template: 'View Content',
standalone: false,
})
class DynamicComponent {}

@Component({
template: '<ng-container #vcr></ng-container>',
standalone: false,
})
class App {
@ViewChild('vcr', {read: ViewContainerRef, static: true})
vcr!: ViewContainerRef;
}

TestBed.configureTestingModule({declarations: [App, DynamicComponent]});

const envInjector = createEnvironmentInjector([], TestBed.inject(EnvironmentInjector));
const fixture = TestBed.createComponent(App);
fixture.detectChanges();

const componentRef = fixture.componentInstance.vcr.createComponent(DynamicComponent, {
environmentInjector: envInjector,
});
fixture.detectChanges();

// Destroy the environment injector first
envInjector.destroy();

// Should be able to destroy the component afterwards.
expect(() => {
componentRef.destroy();
}).not.toThrow();
});
});

describe('length', () => {
it('should return the number of embedded views', () => {
TestBed.configureTestingModule({declarations: [EmbeddedViewInsertionComp, VCRefDirective]});
Expand Down Expand Up @@ -1462,6 +1504,8 @@ describe('ViewContainerRef', () => {
declarations: [EmbeddedViewInsertionComp, VCRefDirective, EmbeddedComponent],
});

const mockSharedStylesHost = new MockSharedStylesHost();

@NgModule({
providers: [
{provide: String, useValue: 'root_module'},
Expand All @@ -1471,11 +1515,19 @@ describe('ViewContainerRef', () => {
{provide: ErrorHandler, useValue: TestBed.inject(ErrorHandler)},
{provide: RendererFactory2, useValue: TestBed.inject(RendererFactory2)},
{provide: ANIMATION_QUEUE, useValue: TestBed.inject(ANIMATION_QUEUE)},
{provide: SHARED_STYLES_HOST, useValue: mockSharedStylesHost},
{provide: DOCUMENT, useValue: document},
],
})
class MyAppModule {}

@NgModule({providers: [{provide: String, useValue: 'some_module'}]})
@NgModule({
providers: [
{provide: String, useValue: 'some_module'},
{provide: SHARED_STYLES_HOST, useValue: mockSharedStylesHost},
{provide: DOCUMENT, useValue: document},
],
})
class SomeModule {}

// Compile test modules in order to be able to pass the NgModuleRef or the
Expand Down
25 changes: 25 additions & 0 deletions packages/core/test/acceptance/view_ref_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,29 @@ describe('ViewRef', () => {
componentRef.hostView.destroy();
expect(viewRef.destroyed).toBe(true);
});

it('should be safe to call `destroy` multiple times', () => {
let ngOnDestroyCount = 0;

@Component({
template: '',
standalone: true,
})
class App {
ngOnDestroy() {
ngOnDestroyCount++;
}
}

const fixture = TestBed.createComponent(App);
fixture.detectChanges();

// Simulate double-destroy scenario (e.g., framework teardown racing with manual teardown like CDK Portal)
expect(() => {
fixture.componentRef.destroy();
fixture.componentRef.destroy();
}).not.toThrow();

expect(ngOnDestroyCount).toBe(1);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@
"SELF_TOKEN",
"SELF_TOKEN_REGEX",
"SHARED_ANIMATION_PROVIDERS",
"SHARED_STYLES_HOST",
"SIGNAL",
"SIMPLE_CHANGES_STORE",
"STABILITY_WARNING_THRESHOLD",
Expand Down Expand Up @@ -504,6 +505,7 @@
"getDOM",
"getDeclarationTNode",
"getDirectiveDef",
"getDocument",
"getElementDepthCount",
"getFactoryDef",
"getFirstLContainer",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@
"RuntimeError",
"SCHEDULE_IN_ROOT_ZONE",
"SCHEDULE_IN_ROOT_ZONE_DEFAULT",
"SHARED_STYLES_HOST",
"SIGNAL",
"SIGNAL_NODE",
"SIMPLE_CHANGES_STORE",
Expand Down Expand Up @@ -407,6 +408,7 @@
"getDOM",
"getDeclarationTNode",
"getDirectiveDef",
"getDocument",
"getFactoryDef",
"getFirstLContainer",
"getFirstNativeNode",
Expand Down
Loading
Loading