Skip to content

Commit

Permalink
fix(platform-browser): wait until animation completion before destroy…
Browse files Browse the repository at this point in the history
…ing renderer (#50860)

Prior to this commit, the renderer destroy method was being called before the animation complete. This is problematic when using `REMOVE_STYLES_ON_COMPONENT_DESTROY` as it causes the styles to be removed too early.

This commit, updates this destroy logic to be call the render destroy once the animations complete.

This has been reported internally in:
- http://b/271251353#comment12
- http://b/282004950#comment5

PR Close #50860
  • Loading branch information
alan-agius4 authored and AndrewKushnir committed Jun 27, 2023
1 parent 39935ee commit 0380564
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,8 @@ export class AnimationEngine {
whenRenderingDone(): Promise<any> {
return this._transitionEngine.whenRenderingDone();
}

afterFlushAnimationsDone(cb: VoidFunction): void {
this._transitionEngine.afterFlushAnimationsDone(cb);
}
}
17 changes: 12 additions & 5 deletions packages/platform-browser/animations/src/animation_renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,19 +138,26 @@ export class AnimationRendererFactory implements RendererFactory2 {
export class BaseAnimationRenderer implements Renderer2 {
constructor(
protected namespaceId: string, public delegate: Renderer2, public engine: AnimationEngine,
private _onDestroy?: () => void) {
this.destroyNode = this.delegate.destroyNode ? (n) => delegate.destroyNode!(n) : null;
}
private _onDestroy?: () => void) {}

get data() {
return this.delegate.data;
}

destroyNode: ((n: any) => void)|null;
destroyNode(node: any): void {
this.delegate.destroyNode?.(node);
}

destroy(): void {
this.engine.destroy(this.namespaceId, this.delegate);
this.delegate.destroy();
this.engine.afterFlushAnimationsDone(() => {
// Call the renderer destroy method after the animations has finished as otherwise
// styles will be removed too early which will cause an unstyled animation.
queueMicrotask(() => {
this.delegate.destroy();
});
});

this._onDestroy?.();
}

Expand Down
41 changes: 23 additions & 18 deletions packages/platform-browser/test/dom/dom_renderer_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,50 +144,50 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
});

describe('should not cleanup styles of destroyed components by default', () => {
it('works for components without encapsulation emulated', () => {
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(styleCount(fixture, '.emulated')).toBe(1);
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(styleCount(fixture, '.emulated')).toBe(1);
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(styleCount(fixture, '.emulated')).toBe(1);
expect(await styleCount(fixture, '.emulated')).toBe(1);
});

it('works for components without encapsulation none', () => {
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(styleCount(fixture, '.none')).toBe(1);
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(styleCount(fixture, '.none')).toBe(1);
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(styleCount(fixture, '.none')).toBe(1);
expect(await styleCount(fixture, '.none')).toBe(1);
});
});

Expand All @@ -212,56 +212,61 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
});
});

it('works for components without encapsulation emulated', () => {
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(styleCount(fixture, '.emulated')).toBe(1);
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(styleCount(fixture, '.emulated')).toBe(1);
expect(await styleCount(fixture, '.emulated')).toBe(1);

// Hide all instances of the component
compInstance.componentTwoInstanceHidden = true;
fixture.detectChanges();

// Verify style is not in DOM
expect(styleCount(fixture, '.emulated')).toBe(0);
expect(await styleCount(fixture, '.emulated')).toBe(0);
});

it('works for components without encapsulation none', () => {
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(styleCount(fixture, '.none')).toBe(1);
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(styleCount(fixture, '.none')).toBe(1);
expect(await styleCount(fixture, '.none')).toBe(1);

// Hide all instances of the component
compInstance.componentTwoInstanceHidden = true;
fixture.detectChanges();

// Verify style is not in DOM
expect(styleCount(fixture, '.emulated')).toBe(0);
expect(await styleCount(fixture, '.emulated')).toBe(0);
});
});
});
}

function styleCount(fixture: ComponentFixture<unknown>, cssContentMatcher: string): number {
async function styleCount(
fixture: ComponentFixture<unknown>, cssContentMatcher: string): Promise<number> {
// flush
await new Promise<void>(resolve => {
setTimeout(() => resolve(), 0);
});

const html = fixture.debugElement.parent?.parent;
const debugElements = html?.queryAll(By.css('style'));

Expand Down

0 comments on commit 0380564

Please sign in to comment.