feat(core): support bootstrapping Angular components underneath shadow roots#66782
feat(core): support bootstrapping Angular components underneath shadow roots#66782dgp1130 wants to merge 11 commits intoangular:mainfrom
Conversation
6ffae5d to
855f44b
Compare
d1e53b0 to
c421a8e
Compare
b5ed4ee to
b1189be
Compare
ROOT_STYLE_HOST to configure Angular to bootstrap under a shadow root272a8b9 to
33fe9f5
Compare
|
One challenge I'm discovering from looking through g3 failures is that CDK Portal apparently relies on being able to manually move rendered DOM from one location to another. This PR breaks that use case because Angular calls |
efc094b to
eee5a01
Compare
d78e646 to
9248874
Compare
thePunderWoman
left a comment
There was a problem hiding this comment.
AGENT: I have left a few inline suggestions regarding potential state imbalances. Please let me know your thoughts!
|
|
||
| if (viewToDetach) { | ||
| const host = viewToDetach[HOST] ?? lContainer[NATIVE]; | ||
| if (host.isConnected) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Maybe we can keep a WeakMap of LView->Node and just grab the host that way?
There was a problem hiding this comment.
addLViewToLContaineradds the host without checkingisConnected.
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?
There was a problem hiding this comment.
@dgp1130 People do manual DOM manipulation a lot more often than we'd like, unfortunately.
| appendChild(newChild: RNode): RNode; | ||
|
|
||
| /** | ||
| * Returns the root node (Document or ShadowRoot) of this node. |
There was a problem hiding this comment.
Nitpicking, but this could return a node if called on an unattached node.
| for (const [url, {elements}] of this.external) { | ||
| elements.push(this.addElement(hostNode, createLinkElement(url, this.doc))); | ||
| const existingUsage = this.hosts.get(hostNode) ?? 0; | ||
| if (existingUsage === 0) { |
There was a problem hiding this comment.
Nitpicking: Can we move the this.hosts.set(hostNode, existingUsage + 1); up and invert this guard to reduce nesting?
| * | ||
| * @see https://developer.mozilla.org/en-US/docs/Web/API/Node/getRootNode | ||
| */ | ||
| getRootNode?(): RNode | ShadowRoot | null; |
|
|
||
| if (viewToDetach) { | ||
| const host = viewToDetach[HOST] ?? lContainer[NATIVE]; | ||
| if (host.isConnected) { |
There was a problem hiding this comment.
Maybe we can keep a WeakMap of LView->Node and just grab the host that way?
| 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 |
There was a problem hiding this comment.
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?
| } | ||
|
|
||
| if (usage! === 1) { | ||
| this.hosts.delete(hostNode); |
There was a problem hiding this comment.
Should we also be removing the inline and external styles when the last host is removed?
554fe0f to
9248874
Compare
…` with a dedicated `InjectionToken` This allows code in `@angular/core` to inject and use `SharedStylesHost`, even though the implementation is defined in `@angular/platform-browser`.
This is necessary for an `RNode` to discover whether it is within the context of a shadow root, which is needed to know where a component's style should be placed. The method is Baseline Widely Available, however we need to treat it as optional for SSR / JSDom contexts where shadow DOM is not supported.
…esHost` When rendering components underneath shadow roots, it is possible to render multiple times, meaning `SharedStylesHost.prototype.addHost` will potentially be called multiple times on the same shadow root. This is reasonable, and we need to make sure that as long as any one component is still using a given shadow root as its host, we should continue applying styles there. Only when all usages of the host are removed do we stop adding styles to it.
This is useful for testing that `addHost` / `removeHost` were called the correct number of times to ensure hosts are not leaking.
Prior to this change, `ViewRef.prototype.destroy` did not short-circuit if it had already been called. This led to a serious issue in certain dynamic component setups, particularly with `@angular/cdk/portal`. When the CDK attaches a `ComponentPortal`, it explicitly registers a disposal function that calls `ComponentRef.prototype.destroy()`. If the portal outlet itself is destroyed as part of a classic Angular teardown sequence, the native framework teardown will destroy the portal`s parent `ViewContainerRef`, subsequently destroying its child views. However, moments later, the CDK`s `ngOnDestroy` hook executes and forcibly repeats the process by triggering `ComponentRef.prototype.destroy()`. Without idempotency, this second destroy invocation cascaded down into the `LView` cleanup processes (`destroyLView`) again, which led to duplicate style unregistration via `SharedStylesHost.removeHost`. If a host had already been unregistered, `SharedStylesHost` threw errors or suffered internal tracking corruption. Adding `if (this.destroyed) return;` acts as a necessary guarantee that views, and by extension style hosts, are only cleaned up precisely once, preserving cross-layer stability.
…w roots This updates component creation and view attach/detach to identify any containing shadow root and apply required styles to it. Managing this during attach / detach onto a view container seems like the ideal timing for what we need, since a node might theoretically be moved from one shadow root to another post-render. Awkwardly, `ViewEncapsulation.ShadowDom` / `ViewEncapsulation.IsolatedShadowDom` manage tracking of hosts already directly in `dom_renderer.ts`, and components should put their own styles in their own shadow root, not a containing shadow root. So we need to leave this alone for now and only modify the other `ViewEncapsulation` modes. A future refactor should consider unifying all `ViewEncapsulation` modes into a single code path. This is quite inefficient as it adds all styles to all shadow roots, which is excessive and unnecessary. However precisely adding only the required styles is a more complicated refactor. I initially tried to call `SharedStylesHost` via dependency injection, but found that injecting during component destruction does not work reliably, as the injector may already be destroyed. Instead, `SharedStylesHost` is tracked in the `LViewEnvironment` which works well as a single object reference shared amongst all components anyways.
…ter destruction Since `SharedStylesHost` can receive `addHost` and `removeHost` calls after it is destroyed in testing environments (e.g., when fakeAsync triggers CD after teardown), we need to safely no-op these operations to avoid errors.
… `LViewEnvironment` Prior to this change, when style encapsulation fell back from Shadow DOM, it defaulted to the global `document.head` via `getDocument()`. This was problematic for mocked or isolated environments (such as unit tests) where a custom `DOCUMENT` token is provided, but `setDocument` is not called. Because `SharedStylesHost` relies on the node it is passed, using the global `document.head` in a mocked environment could lead to cross-document DOM insertion errors or just failing to style the component correctly. This commit introduces `fallbackDocument` to `LViewEnvironment`. It is captured at bootstrap in `createRootLViewEnvironment` directly from the `DOCUMENT` injection token, since we can't reliably inject during component destruction, falling back to `getDocument()`. This ensures that the correct document context is plumbed through the view layer, guaranteeing stable style injection inside the expected document environment without cross-document pollution.
…ost in `SharedStylesHost` `SharedStylesHost` has a known bug where removing hosts does not remove styles from the DOM and fixing that would be a breaking change. However, this leads to a separate bug where removing a style down to zero usages and then adding an extra one, could duplicate styles on the page. This commit ensures styles are only appended if they aren't already present on the parent node from a previously leaked style.
This tests bootstrapping Angular underneath a shadow root and that styles are applied and removed at the correct locations.
…d on the injector While developing this feature, I attempted to inject a service during component destruction (to call `SharedStylesHost.prototype.removeHost`), however this broke some Material use cases which seem to destroy the injector before the component. Whether this constraint was intended or not, I believe we currently live in a world where component destruction cannot depend on its injection, so I'm enforcing this in a test to be clear about that constraint. This is not intended to imply that this is a _good_ constraint or that we _want_ this to be the case, but it does represent the reality we currently exist in.
9248874 to
17f2628
Compare
Local tests seem to be passing, but I'm currently working through some google3 failures.
This PR allows Angular components to be bootstrapped inside of a shadow root and properly apply their styles to that shadow root instead of the document
<head>. This PR is an alternative to #66048, which has been stalled due to complicated tests which we've been struggled to debug, and attempts to solve the same problem by making minimal changes to the frameworks and avoid pulling on more complexity than absolutely necessary.The early commits are just refactoring some internals and setting up the right utilities and types required. The
testcommit with shadow DOM is the ultimate goal of this PR.The main
featcommit is the real work. It tracks style hosts (document<head>or shadow root) during component render and attach/detach to a container. This allows a single component render in a random location to find an ancestor shadow root and track it for styles to be added as appropriate.Based on some discussions with @AndrewKushnir, we believe the right time to do this is during component render xor attach to view container. Since a component could theoretically be moved from one container to another or rendered in a detached state (via
createComponent) and then attached to a specific container later.We use
Node.prototype.getRootNodeto determine whether or not a particular component is within aShadowRoot. We could use some framework-internal data structures, but ultimately we need to support shadow roots outside the root component anyways and unknown to Angular, so we cannot expect to rely on framework data structures to solve this entire problem.