Skip to content

fix(core): deferred blocks not removing content immediately when animations are enabled #51971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
crisbeto wants to merge 1 commit into from
Closed

fix(core): deferred blocks not removing content immediately when animations are enabled #51971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion packages/core/src/render3/instructions/defer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
*/

import {InjectionToken, Injector, ɵɵdefineInjectable} from '../../di';
import {inject} from '../../di/injector_compatibility';
import {findMatchingDehydratedView} from '../../hydration/views';
import {populateDehydratedViewsInLContainer} from '../../linker/view_container_ref';
import {assertDefined, assertElement, assertEqual, throwError} from '../../util/assert';
Expand Down
24 changes: 18 additions & 6 deletions packages/core/src/render3/instructions/defer_events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,10 @@ class DeferEventEntry {
* Registers an interaction trigger.
* @param trigger Element that is the trigger.
* @param callback Callback to be invoked when the trigger is interacted with.
* @param injector Injector that can be used by the trigger to resolve DI tokens.
*/
export function onInteraction(trigger: Element, callback: VoidFunction) {
export function onInteraction(
trigger: Element, callback: VoidFunction, injector: Injector): VoidFunction {
let entry = interactionTriggers.get(trigger);

// If this is the first entry for this element, add the listeners.
Expand All @@ -59,9 +61,13 @@ export function onInteraction(trigger: Element, callback: VoidFunction) {
entry = new DeferEventEntry();
interactionTriggers.set(trigger, entry);

for (const name of interactionEventNames) {
trigger.addEventListener(name, entry.listener, eventListenerOptions);
}
// Ensure that the handler runs in the NgZone since it gets
// registered in `afterRender` which runs outside.
injector.get(NgZone).run(() => {
Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Oct 1, 2023

Choose a reason for hiding this comment

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

Note that I was having a hard time capturing this behavior in a unit test, because the testing APIs would trigger change detection which would flush the animations.

Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir Oct 1, 2023

Choose a reason for hiding this comment

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

Thanks for the comment, I haven't seen it while leaving a review comment (#51971 (review)). May be we can have a test to make sure that we run event handlers inside NgZone?

Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Oct 1, 2023

Choose a reason for hiding this comment

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

Hmm, I don't know if we can observe the internal event handlers without introducing another DI token. I ended up adding a dev mode assertion that it's in the NgZone which should give us the same result.

Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir Oct 1, 2023

Choose a reason for hiding this comment

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

I ended up adding a dev mode assertion that it's in the NgZone which should give us the same result.

That's a great idea!

Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Oct 1, 2023

Choose a reason for hiding this comment

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

It looks like the assertion isn't super reliable since Zone.js doesn't seem to patch addEventListener in the same way in Node and in the browser so we were getting different test results. I reworked it to spy on addEventListener and check that it's called within the zone instead.

for (const name of interactionEventNames) {
trigger.addEventListener(name, entry!.listener, eventListenerOptions);
Comment thread
AndrewKushnir marked this conversation as resolved.
Outdated
}
});
}

entry.callbacks.add(callback);
Expand All @@ -84,15 +90,21 @@ export function onInteraction(trigger: Element, callback: VoidFunction) {
* Registers a hover trigger.
* @param trigger Element that is the trigger.
* @param callback Callback to be invoked when the trigger is hovered over.
* @param injector Injector that can be used by the trigger to resolve DI tokens.
*/
export function onHover(trigger: Element, callback: VoidFunction): VoidFunction {
export function onHover(
trigger: Element, callback: VoidFunction, injector: Injector): VoidFunction {
let entry = hoverTriggers.get(trigger);

// If this is the first entry for this element, add the listener.
if (!entry) {
entry = new DeferEventEntry();
trigger.addEventListener('mouseenter', entry.listener, eventListenerOptions);
hoverTriggers.set(trigger, entry);
// Ensure that the handler runs in the NgZone since it gets
// registered in `afterRender` which runs outside.
injector.get(NgZone).run(() => {
trigger.addEventListener('mouseenter', entry!.listener, eventListenerOptions);
});
}

entry.callbacks.add(callback);
Expand Down
56 changes: 55 additions & 1 deletion packages/core/test/acceptance/defer_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import {ɵPLATFORM_BROWSER_ID as PLATFORM_BROWSER_ID} from '@angular/common';
import {ɵsetEnabledBlockTypes as setEnabledBlockTypes} from '@angular/compiler/src/jit_compiler_facade';
import {Component, Input, PLATFORM_ID, QueryList, Type, ViewChildren, ɵDEFER_BLOCK_DEPENDENCY_INTERCEPTOR} from '@angular/core';
import {Component, Input, NgZone, PLATFORM_ID, QueryList, Type, ViewChildren, ɵDEFER_BLOCK_DEPENDENCY_INTERCEPTOR} from '@angular/core';
import {getComponentDef} from '@angular/core/src/render3/definition';
import {DeferBlockBehavior, fakeAsync, flush, TestBed} from '@angular/core/testing';

Expand Down Expand Up @@ -1227,6 +1227,7 @@ describe('@defer', () => {
template: 'Primary block content.',
})
class NestedCmp {
@Input() block!: string;
Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Oct 1, 2023

Choose a reason for hiding this comment

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

This is unrelated, I just noticed that it was logging a warning.

}

@Component({
Expand Down Expand Up @@ -1403,6 +1404,7 @@ describe('@defer', () => {
template: 'Primary block content.',
})
class NestedCmp {
@Input() block!: string;
}

@Component({
Expand Down Expand Up @@ -1956,6 +1958,32 @@ describe('@defer', () => {
expect(spy).toHaveBeenCalledWith('keydown', jasmine.any(Function), jasmine.any(Object));
}));

it('should bind the trigger events inside the NgZone', fakeAsync(() => {
@Component({
standalone: true,
template: `
@defer (on interaction(trigger)) {
Main content
}

<button #trigger></button>
`
})
class MyCmp {
}

const eventsInZone: Record<string, boolean> = {};
const fixture = TestBed.createComponent(MyCmp);
const button = fixture.nativeElement.querySelector('button');

spyOn(button, 'addEventListener').and.callFake((name: string) => {
eventsInZone[name] = NgZone.isInAngularZone();
});
fixture.detectChanges();

expect(eventsInZone).toEqual({click: true, keydown: true});
}));

it('should prefetch resources on interaction', fakeAsync(() => {
@Component({
standalone: true,
Expand Down Expand Up @@ -2254,6 +2282,32 @@ describe('@defer', () => {
expect(spy).toHaveBeenCalledWith('mouseenter', jasmine.any(Function), jasmine.any(Object));
}));

it('should bind the trigger events inside the NgZone', fakeAsync(() => {
@Component({
standalone: true,
template: `
@defer (on hover(trigger)) {
Main content
}

<button #trigger></button>
`
})
class MyCmp {
}

const eventsInZone: Record<string, boolean> = {};
const fixture = TestBed.createComponent(MyCmp);
const button = fixture.nativeElement.querySelector('button');

spyOn(button, 'addEventListener').and.callFake((name: string) => {
eventsInZone[name] = NgZone.isInAngularZone();
});
fixture.detectChanges();

expect(eventsInZone).toEqual({mouseenter: true});
}));

it('should prefetch resources on hover', fakeAsync(() => {
// Domino doesn't support creating custom events so we have to skip this test.
if (!isBrowser) {
Expand Down