From e3d9560e5c797b463c49c3b12ae22eadbf8f147b Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Mon, 18 Sep 2023 15:37:31 +0000 Subject: [PATCH] perf(platform-browser): disable styles of removed components instead of removing This commit changes the behaviour of `REMOVE_STYLES_ON_COMPONENT_DESTROY` in 2 main ways: - `style` nodes are disabled instead of removed from DOM. This causes the same runtime behaviour but avoids recomputations when the stylesheet is re-added when the component is re-created. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_style_sheet.h;l=266;drc=31fb07c05718d671d96c227855bfe97af9e3fb20 - `style` nodes of components with `ViewEncapsulation.Emulated` are neither removed nor disabled. This is because such styles from such components cannot leak and effect other components. NB: This changes is being done following some performance bottlenecks observed in Phanteon and their own recommendations. Context: http://chat/room/AAAAxKxTk40/jaP6Lj6fhmQ/jaP6Lj6fhmQ https://crbug.com/1444522 http://b/289992821 --- .../platform-browser/src/dom/dom_renderer.ts | 36 ++-- .../src/dom/shared_styles_host.ts | 30 +-- .../test/dom/dom_renderer_spec.ts | 171 +++++++++--------- 3 files changed, 124 insertions(+), 113 deletions(-) diff --git a/packages/platform-browser/src/dom/dom_renderer.ts b/packages/platform-browser/src/dom/dom_renderer.ts index 056e913de202c..e17b3c6abf111 100644 --- a/packages/platform-browser/src/dom/dom_renderer.ts +++ b/packages/platform-browser/src/dom/dom_renderer.ts @@ -36,13 +36,13 @@ const REMOVE_STYLES_ON_COMPONENT_DESTROY_DEFAULT = true; /** * A [DI token](guide/glossary#di-token "DI token definition") that indicates whether styles - * of destroyed components should be removed from DOM. + * of destroyed components with `ViewEncapsulation.None` should be disabled. * * By default, the value is set to `true`. * @publicApi */ export const REMOVE_STYLES_ON_COMPONENT_DESTROY = - new InjectionToken('RemoveStylesOnCompDestroy', { + new InjectionToken(ngDevMode ? 'RemoveStylesOnCompDestroy' : '', { providedIn: 'root', factory: () => REMOVE_STYLES_ON_COMPONENT_DESTROY_DEFAULT, }); @@ -70,7 +70,7 @@ export class DomRendererFactory2 implements RendererFactory2, OnDestroy { private readonly eventManager: EventManager, private readonly sharedStylesHost: SharedStylesHost, @Inject(APP_ID) private readonly appId: string, - @Inject(REMOVE_STYLES_ON_COMPONENT_DESTROY) private removeStylesOnCompDestroy: boolean, + @Inject(REMOVE_STYLES_ON_COMPONENT_DESTROY) private disableStylesOnCompDestroy: boolean, @Inject(DOCUMENT) private readonly doc: Document, @Inject(PLATFORM_ID) readonly platformId: Object, readonly ngZone: NgZone, @@ -112,14 +112,12 @@ export class DomRendererFactory2 implements RendererFactory2, OnDestroy { const ngZone = this.ngZone; const eventManager = this.eventManager; const sharedStylesHost = this.sharedStylesHost; - const removeStylesOnCompDestroy = this.removeStylesOnCompDestroy; const platformIsServer = this.platformIsServer; switch (type.encapsulation) { case ViewEncapsulation.Emulated: renderer = new EmulatedEncapsulationDomRenderer2( - eventManager, sharedStylesHost, type, this.appId, removeStylesOnCompDestroy, doc, - ngZone, platformIsServer); + eventManager, sharedStylesHost, type, this.appId, doc, ngZone, platformIsServer); break; case ViewEncapsulation.ShadowDom: return new ShadowDomRenderer( @@ -127,7 +125,7 @@ export class DomRendererFactory2 implements RendererFactory2, OnDestroy { platformIsServer); default: renderer = new NoneEncapsulationDomRenderer( - eventManager, sharedStylesHost, type, removeStylesOnCompDestroy, doc, ngZone, + eventManager, sharedStylesHost, type, this.disableStylesOnCompDestroy, doc, ngZone, platformIsServer); break; } @@ -395,20 +393,19 @@ class ShadowDomRenderer extends DefaultDomRenderer2 { } class NoneEncapsulationDomRenderer extends DefaultDomRenderer2 { - private readonly styles: string[]; + protected styles: string[]; constructor( eventManager: EventManager, private readonly sharedStylesHost: SharedStylesHost, component: RendererType2, - private removeStylesOnCompDestroy: boolean, + private disableStylesOnCompDestroy: boolean, doc: Document, ngZone: NgZone, platformIsServer: boolean, - compId?: string, ) { super(eventManager, doc, ngZone, platformIsServer); - this.styles = compId ? shimStylesContent(compId, component.styles) : component.styles; + this.styles = component.styles; } applyStyles(): void { @@ -416,11 +413,9 @@ class NoneEncapsulationDomRenderer extends DefaultDomRenderer2 { } override destroy(): void { - if (!this.removeStylesOnCompDestroy) { - return; + if (this.disableStylesOnCompDestroy) { + this.sharedStylesHost.disableStyles(this.styles); } - - this.sharedStylesHost.removeStyles(this.styles); } } @@ -430,12 +425,11 @@ class EmulatedEncapsulationDomRenderer2 extends NoneEncapsulationDomRenderer { constructor( eventManager: EventManager, sharedStylesHost: SharedStylesHost, component: RendererType2, - appId: string, removeStylesOnCompDestroy: boolean, doc: Document, ngZone: NgZone, - platformIsServer: boolean) { + appId: string, doc: Document, ngZone: NgZone, platformIsServer: boolean) { + super(eventManager, sharedStylesHost, component, false, doc, ngZone, platformIsServer); const compId = appId + '-' + component.id; - super( - eventManager, sharedStylesHost, component, removeStylesOnCompDestroy, doc, ngZone, - platformIsServer, compId); + + this.styles = shimStylesContent(compId, component.styles); this.contentAttr = shimContentAttribute(compId); this.hostAttr = shimHostAttribute(compId); } @@ -450,4 +444,6 @@ class EmulatedEncapsulationDomRenderer2 extends NoneEncapsulationDomRenderer { super.setAttribute(el, this.contentAttr, ''); return el; } + + override destroy(): void {} } diff --git a/packages/platform-browser/src/dom/shared_styles_host.ts b/packages/platform-browser/src/dom/shared_styles_host.ts index 31c449b75e465..b9b90826ec36b 100644 --- a/packages/platform-browser/src/dom/shared_styles_host.ts +++ b/packages/platform-browser/src/dom/shared_styles_host.ts @@ -44,16 +44,22 @@ export class SharedStylesHost implements OnDestroy { } } - removeStyles(styles: string[]): void { + disableStyles(styles: string[]): void { for (const style of styles) { const usageCount = this.changeUsageCount(style, -1); if (usageCount <= 0) { - this.onStyleRemoved(style); + this.visitStyleElement(style, (node) => { + node.disabled = true; + }); } } } + visitStyleElement(style: string, callback: (node: HTMLStyleElement) => void): void { + this.styleRef.get(style)?.elements?.forEach(callback); + } + ngOnDestroy(): void { const styleNodesInDOM = this.styleNodesInDOM; if (styleNodesInDOM) { @@ -62,7 +68,10 @@ export class SharedStylesHost implements OnDestroy { } for (const style of this.getAllStyles()) { - this.onStyleRemoved(style); + this.visitStyleElement(style, (node) => { + node.remove(); + }); + this.styleRef.delete(style); } this.resetHostNodes(); @@ -90,12 +99,6 @@ export class SharedStylesHost implements OnDestroy { } } - private onStyleRemoved(style: string): void { - const styleRef = this.styleRef; - styleRef.get(style)?.elements?.forEach((node) => node.remove()); - styleRef.delete(style); - } - private collectServerRenderedStyles(): Map|null { const styles = this.doc.head?.querySelectorAll( `style[${APP_ID_ATTRIBUTE_NAME}="${this.appId}"]`); @@ -119,13 +122,14 @@ export class SharedStylesHost implements OnDestroy { const map = this.styleRef; if (map.has(style)) { const styleRefValue = map.get(style)!; - styleRefValue.usage += delta; + styleRefValue.usage = Math.max(0, styleRefValue.usage + delta); return styleRefValue.usage; } - map.set(style, {usage: delta, elements: []}); - return delta; + const usage = Math.max(0, delta); + map.set(style, {usage, elements: []}); + return usage; } private getStyleElement(host: Node, style: string): HTMLStyleElement { @@ -163,6 +167,8 @@ export class SharedStylesHost implements OnDestroy { private addStyleToHost(host: Node, style: string): void { const styleEl = this.getStyleElement(host, style); + // Enable the stylesheet + styleEl.disabled = false; host.appendChild(styleEl); const styleRef = this.styleRef; diff --git a/packages/platform-browser/test/dom/dom_renderer_spec.ts b/packages/platform-browser/test/dom/dom_renderer_spec.ts index d7a1106b6e80b..d4673e669ed0a 100644 --- a/packages/platform-browser/test/dom/dom_renderer_spec.ts +++ b/packages/platform-browser/test/dom/dom_renderer_spec.ts @@ -5,7 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -import {Component, Renderer2, ViewEncapsulation} from '@angular/core'; +import {Component, DebugElement, Renderer2, ViewEncapsulation} from '@angular/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {NAMESPACE_URIS, REMOVE_STYLES_ON_COMPONENT_DESTROY} from '@angular/platform-browser/src/dom/dom_renderer'; @@ -143,79 +143,30 @@ import {expect} from '@angular/platform-browser/testing/src/matchers'; expect(otherChild.parentNode).toBe(template.content); }); - describe( - 'should not cleanup styles of destroyed components when `REMOVE_STYLES_ON_COMPONENT_DESTROY` is `false`', - () => { - beforeEach(() => { - TestBed.resetTestingModule(); - - TestBed.configureTestingModule({ - declarations: [ - SomeAppForCleanUp, - CmpEncapsulationEmulated, - CmpEncapsulationNone, - ], - providers: [ - { - provide: REMOVE_STYLES_ON_COMPONENT_DESTROY, - useValue: false, - }, - ], - }); - }); - - it('works for components without encapsulation emulated', async () => { - const fixture = TestBed.createComponent(SomeAppForCleanUp); - const compInstance = fixture.componentInstance; - compInstance.showEmulatedComponents = true; - - fixture.detectChanges(); - // verify style is in DOM - expect(await styleCount(fixture, '.emulated')).toBe(1); - - // Remove a single instance of the component. - compInstance.componentOneInstanceHidden = true; - fixture.detectChanges(); - // Verify style is still in DOM - expect(await styleCount(fixture, '.emulated')).toBe(1); - - // Hide all instances of the component - compInstance.componentTwoInstanceHidden = true; - fixture.detectChanges(); - - // Verify style is still in DOM - expect(await styleCount(fixture, '.emulated')).toBe(1); - }); - - it('works for components without encapsulation none', async () => { - const fixture = TestBed.createComponent(SomeAppForCleanUp); - const compInstance = fixture.componentInstance; - compInstance.showEmulatedComponents = false; - - fixture.detectChanges(); - // verify style is in DOM - expect(await styleCount(fixture, '.none')).toBe(1); - - // Remove a single instance of the component. - compInstance.componentOneInstanceHidden = true; - fixture.detectChanges(); - // Verify style is still in DOM - expect(await styleCount(fixture, '.none')).toBe(1); - - // Hide all instances of the component - compInstance.componentTwoInstanceHidden = true; - fixture.detectChanges(); - - // Verify style is still in DOM - expect(await styleCount(fixture, '.none')).toBe(1); - }); + describe('When `REMOVE_STYLES_ON_COMPONENT_DESTROY` is `false`', () => { + beforeEach(() => { + TestBed.resetTestingModule(); + + TestBed.configureTestingModule({ + declarations: [ + SomeAppForCleanUp, + CmpEncapsulationEmulated, + CmpEncapsulationNone, + ], + providers: [ + { + provide: REMOVE_STYLES_ON_COMPONENT_DESTROY, + useValue: false, + }, + ], }); + }); - describe('should cleanup styles of destroyed components by default', () => { - it('works for components without encapsulation emulated', async () => { + it('should not disable styles for components with encapsulation emulated', async () => { const fixture = TestBed.createComponent(SomeAppForCleanUp); const compInstance = fixture.componentInstance; compInstance.showEmulatedComponents = true; + fixture.detectChanges(); // verify style is in DOM expect(await styleCount(fixture, '.emulated')).toBe(1); @@ -230,11 +181,13 @@ import {expect} from '@angular/platform-browser/testing/src/matchers'; compInstance.componentTwoInstanceHidden = true; fixture.detectChanges(); - // Verify style is not in DOM - expect(await styleCount(fixture, '.emulated')).toBe(0); + // Verify style is not disabled in DOM + const styles = await getStyles(fixture, '.emulated'); + expect(styles?.length).toBe(1); + expect(styles?.[0].nativeElement.disabled).toBeFalse(); }); - it('works for components without encapsulation none', async () => { + it('should not disable styles for components with encapsulation none', async () => { const fixture = TestBed.createComponent(SomeAppForCleanUp); const compInstance = fixture.componentInstance; compInstance.showEmulatedComponents = false; @@ -253,15 +206,76 @@ import {expect} from '@angular/platform-browser/testing/src/matchers'; compInstance.componentTwoInstanceHidden = true; fixture.detectChanges(); - // Verify style is not in DOM - expect(await styleCount(fixture, '.emulated')).toBe(0); + // Verify style is not disabled in DOM + const styles = await getStyles(fixture, '.none'); + expect(styles?.length).toBe(1); + expect(styles?.[0].nativeElement.disabled).toBeFalse(); + }); + }); + + describe('When `REMOVE_STYLES_ON_COMPONENT_DESTROY` is `true` or default.', () => { + it('should not disable styles for components with encapsulation emulated', async () => { + const fixture = TestBed.createComponent(SomeAppForCleanUp); + const compInstance = fixture.componentInstance; + compInstance.showEmulatedComponents = true; + fixture.detectChanges(); + // verify style is in DOM + expect(await styleCount(fixture, '.emulated')).toBe(1); + + // Remove a single instance of the component. + compInstance.componentOneInstanceHidden = true; + fixture.detectChanges(); + // Verify style is still in DOM + expect(await styleCount(fixture, '.emulated')).toBe(1); + + // Hide all instances of the component + compInstance.componentTwoInstanceHidden = true; + fixture.detectChanges(); + + // Verify style is not disabled in DOM + const styles = await getStyles(fixture, '.emulated'); + expect(styles?.length).toBe(1); + expect(styles?.[0].nativeElement.disabled).toBeFalse(); + }); + + it('should disable styles for components with encapsulation none', async () => { + const fixture = TestBed.createComponent(SomeAppForCleanUp); + const compInstance = fixture.componentInstance; + compInstance.showEmulatedComponents = false; + + fixture.detectChanges(); + // verify style is in DOM + expect(await styleCount(fixture, '.none')).toBe(1); + + // Remove a single instance of the component. + compInstance.componentOneInstanceHidden = true; + fixture.detectChanges(); + // Verify style is still in DOM + expect(await styleCount(fixture, '.none')).toBe(1); + + // Hide all instances of the component + compInstance.componentTwoInstanceHidden = true; + fixture.detectChanges(); + + // Verify style is disabled in DOM + const styles = await getStyles(fixture, '.none'); + expect(styles?.length).toBe(1); + expect(styles?.[0].nativeElement.disabled).toBeTrue(); }); }); }); } + async function styleCount( fixture: ComponentFixture, cssContentMatcher: string): Promise { + const styles = await getStyles(fixture, cssContentMatcher); + + return styles?.length ?? 0; +} + +async function getStyles(fixture: ComponentFixture, cssContentMatcher: string): + Promise { // flush await new Promise(resolve => { setTimeout(() => resolve(), 0); @@ -270,13 +284,8 @@ async function styleCount( const html = fixture.debugElement.parent?.parent; const debugElements = html?.queryAll(By.css('style')); - if (!debugElements) { - return 0; - } - - return debugElements - .filter(({nativeElement}) => nativeElement.textContent.includes(cssContentMatcher)) - .length; + return debugElements?.filter( + ({nativeElement}) => nativeElement.textContent.includes(cssContentMatcher)); }