Skip to content

fix(animations): allow enter animation of every inserted element #44243

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
dario-piotrowicz wants to merge 1 commit into from
Closed

fix(animations): allow enter animation of every inserted element #44243

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
5 changes: 3 additions & 2 deletions packages/animations/browser/src/render/animation_engine_next.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AnimationMetadata, AnimationPlayer, AnimationTriggerMetadata} from '@angular/animations';

import {TriggerAst} from '../dsl/animation_ast';
import {buildAnimationAst} from '../dsl/animation_ast_builder';
import {AnimationTrigger, buildTrigger} from '../dsl/animation_trigger';
Expand Down Expand Up @@ -62,8 +63,8 @@ export class AnimationEngine {
this._transitionEngine.destroy(namespaceId, context);
}

onInsert(namespaceId: string, element: any, parent: any, insertBefore: boolean): void {
this._transitionEngine.insertNode(namespaceId, element, parent, insertBefore);
onInsert(namespaceId: string, element: any, parent: any): void {
this._transitionEngine.insertNode(namespaceId, element, parent);
}

onRemove(namespaceId: string, element: any, context: any, isHostElement?: boolean): void {
Expand Down
29 changes: 22 additions & 7 deletions packages/animations/browser/src/render/transition_animation_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,7 @@ export class TransitionAnimationEngine {
return false;
}

insertNode(namespaceId: string, element: any, parent: any, insertBefore: boolean): void {
insertNode(namespaceId: string, element: any, parent: any): void {
if (!isElementNode(element)) return;

// special case for when an element is removed and reinserted (move operation)
Expand Down Expand Up @@ -713,10 +713,7 @@ export class TransitionAnimationEngine {
}
}

// only *directives and host elements are inserted before
if (insertBefore) {
this.collectEnterElement(element);
}
this.collectEnterElement(element);
}

collectEnterElement(element: any) {
Expand Down Expand Up @@ -936,7 +933,7 @@ export class TransitionAnimationEngine {
enterNodeMap.forEach((nodes, root) => {
const className = ENTER_CLASSNAME + i++;
enterNodeMapIds.set(root, className);
nodes.forEach(node => addClass(node, className));
nodes.forEach(node => addClassRecursively(node, className));
});

const allLeaveNodes: any[] = [];
Expand Down Expand Up @@ -967,7 +964,7 @@ export class TransitionAnimationEngine {
cleanupFns.push(() => {
enterNodeMap.forEach((nodes, root) => {
const className = enterNodeMapIds.get(root)!;
nodes.forEach(node => removeClass(node, className));
nodes.forEach(node => removeClassRecursively(node, className));
});

leaveNodeMap.forEach((nodes, root) => {
Expand Down Expand Up @@ -1734,6 +1731,15 @@ function addClass(element: any, className: string) {
}
}

function addClassRecursively(element: any, className: string) {
Copy link
Copy Markdown
Member

@JoostK JoostK Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I am concerned about this approach for perf reasons; adding and removing classes is not free. Would it be possible to tweak query selectors from .${class} to something like .${class}, .${class} * to select all descendants? I am not sure about the perf characteristics of such query, though.

Copy link
Copy Markdown
Contributor Author

@dario-piotrowicz dario-piotrowicz Nov 22, 2021

Choose a reason for hiding this comment

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

Yes I understand what you mean....

I don't think that tweaking the selector should be too much of an issue

For context the code that does the querying is this one:

angular/packages/animations/browser/src/dsl/animation_timeline_builder.ts

Lines 563 to 588 in 155742e

invokeQuery(
selector: string, originalSelector: string, limit: number, includeSelf: boolean,
optional: boolean, errors: any[]): any[] {
let results: any[] = [];
if (includeSelf) {
results.push(this.element);
}
if (selector.length > 0) { // only if :self is used then the selector can be empty
selector = selector.replace(ENTER_TOKEN_REGEX, '.' + this._enterClassName);
selector = selector.replace(LEAVE_TOKEN_REGEX, '.' + this._leaveClassName);
const multi = limit != 1;
let elements = this._driver.query(this.element, selector, multi);
if (limit !== 0) {
elements = limit < 0 ? elements.slice(elements.length + limit, elements.length) :
elements.slice(0, limit);
}
results.push(...elements);
}
if (!optional && results.length == 0) {
errors.push(`\`query("${originalSelector}")\` returned zero elements. (Use \`query("${
originalSelector}", { optional: true })\` if you wish to allow this.)`);
}
return results;
}
}

I think that before selector = selector.replace(ENTER_TOKEN_REGEX, '.' + this._enterClassName); we could have a check and if the element doing the querying is entering then we just substitute the assignment to: selector = selector.replace(ENTER_TOKEN_REGEX, '*');
(ps: there is not info about the parent element entering so we would need to add that somehow too)

this seems like a pretty inexpensive change

Then what actually actually does the querying (called by the driver._query above) is a simple querySelector/querySelectorAll:

angular/packages/animations/browser/src/render/shared.ts

Lines 192 to 214 in 8d0511d

_query = (element: any, selector: string, multi: boolean): any[] => {
let results: any[] = [];
if (multi) {
// DO NOT REFACTOR TO USE SPREAD SYNTAX.
// For element queries that return sufficiently large NodeList objects,
// using spread syntax to populate the results array causes a RangeError
// due to the call stack limit being reached. `Array.from` can not be used
// as well, since NodeList is not iterable in IE 11, see
// https://developer.mozilla.org/en-US/docs/Web/API/NodeList
// More info is available in #38551.
const elems = element.querySelectorAll(selector);
for (let i = 0; i < elems.length; i++) {
results.push(elems[i]);
}
} else {
const elm = element.querySelector(selector);
if (elm) {
results.push(elm);
}
}
return results;
};
}

this again doesn't seem like a too expensive change since we use the browser's native api... I think?! 🤷

what do you think? does this sound like a good thing to try out? shall I give it a shot?

if (isElementNode(element)) {
addClass(element, className);
element.childNodes.forEach(
(child: HTMLElement) => addClassRecursively(child, className),
);
}
}

function removeClass(element: any, className: string) {
if (element.classList) {
element.classList.remove(className);
Expand All @@ -1745,6 +1751,15 @@ function removeClass(element: any, className: string) {
}
}

function removeClassRecursively(element: any, className: string) {
if (isElementNode(element)) {
removeClass(element, className);
element.childNodes.forEach(
(child: HTMLElement) => removeClassRecursively(child, className),
);
}
}

function removeNodesAfterAnimationDone(
engine: TransitionAnimationEngine, element: any, players: AnimationPlayer[]) {
optimizeGroupPlayer(players).onDone(() => engine.processLeaveNode(element));
Expand Down
6 changes: 3 additions & 3 deletions packages/animations/browser/test/render/transition_animation_engine_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ describe('TransitionAnimationEngine', () => {
registerTrigger(child, engine, childTrigger);

element.appendChild(child);
engine.insertNode(DEFAULT_NAMESPACE_ID, child, element, true);
engine.insertNode(DEFAULT_NAMESPACE_ID, child, element);

setProperty(element, engine, 'parent', 'value');
setProperty(child, engine, 'child', 'visible');
Expand Down Expand Up @@ -652,9 +652,9 @@ describe('TransitionAnimationEngine', () => {
element.appendChild(child2);

element.appendChild(child1);
engine.insertNode(DEFAULT_NAMESPACE_ID, child1, element, true);
engine.insertNode(DEFAULT_NAMESPACE_ID, child1, element);
element.appendChild(child2);
engine.insertNode(DEFAULT_NAMESPACE_ID, child2, element, true);
engine.insertNode(DEFAULT_NAMESPACE_ID, child2, element);

expect(element.contains(child1)).toBe(true);
expect(element.contains(child2)).toBe(true);
Expand Down
4 changes: 3 additions & 1 deletion packages/core/test/animation/animation_integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2519,7 +2519,9 @@ describe('animation tests', function() {
});
});

it('should not animate i18n insertBefore', () => {
// TO DISCUSS: every insertion is going to be threated as a move one, this will so also
// apply to i18n elements, I think it is ok, is it? can this test be removed?
xit('should not animate i18n insertBefore', () => {
Comment on lines 2522 to 2524
Copy link
Copy Markdown
Contributor Author

@dario-piotrowicz dario-piotrowicz Nov 21, 2021

Choose a reason for hiding this comment

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

The comment should be self explanatory, but yeah, it seems like i18n insertions were done via insertBefore so they needed not to be treated as entering, since now everything is treated as entering the i18n insertions are no difference, I guess this should be ok 🤔

Copy link
Copy Markdown
Contributor

@jessicajaniuk jessicajaniuk Nov 22, 2021

Choose a reason for hiding this comment

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

Did you intend to say "threated"? Or did you mean threaded?

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin Nov 22, 2021

Choose a reason for hiding this comment

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

Or "treated"?

Copy link
Copy Markdown
Contributor Author

@dario-piotrowicz dario-piotrowicz Nov 22, 2021

Choose a reason for hiding this comment

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

"treated" 😅

honestly I just wanted to remove this test and not x-it, I just put the comment in the code to start a discussion
(seems like a succeeded, even though this is not the sort of discussion I was expecting 😆)

// I18n uses `insertBefore` API to insert nodes in correct order. Animation assumes that
// any `insertBefore` is a move and tries to animate it.
// NOTE: This test was extracted from `g3`
Expand Down
166 changes: 103 additions & 63 deletions packages/core/test/animation/animation_query_integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,8 +1017,8 @@ describe('animation query tests', function() {
<div [@myAnimation]="items.length" class="parent">
<ng-container *ngFor="let item of items">
<section>
<div *ngIf="item % 2 == 0">even {{ item }}</div>
<div *ngIf="item % 2 == 1">odd {{ item }}</div>
<div class="item" *ngIf="item % 2 == 0">even {{ item }}</div>
<div class="item" *ngIf="item % 2 == 1">odd {{ item }}</div>
</section>
</ng-container>
</div>
Expand All @@ -1027,12 +1027,13 @@ describe('animation query tests', function() {
'myAnimation',
[
transition('0 => 5', [
query(':enter', [
query('.item:enter', [
style({ opacity: '0' }),
animate(1000, style({ opacity: '1' }))
])
]),
transition('5 => 0', [
// TODO: :enter and :leave of inner elements behave differently they should probably be aligned
Copy link
Copy Markdown
Contributor Author

@dario-piotrowicz dario-piotrowicz Nov 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

my changes are only fixing the :enter issue, the :leave one can be treated separately (of if you want I can look into it and add it to this PR)

query(':leave', [
style({ opacity: '1' }),
animate(1000, style({ opacity: '0' }))
Expand Down Expand Up @@ -2515,79 +2516,118 @@ describe('animation query tests', function() {
expect(element.innerText.trim()).toMatch(/this\s+child/mg);
}));

it('should only mark outermost *directive nodes :enter and :leave when inserts and removals occur',
() => {
@Component({
selector: 'ani-cmp',
animations: [
trigger(
'anim',
[
transition(
'* => enter',
[
query(':enter', [animate(1000, style({color: 'red'}))]),
]),
transition(
'* => leave',
[
query(':leave', [animate(1000, style({color: 'blue'}))]),
]),
]),
],
template: `
it('should mark all nodes :enter when their parent\'s insertion occurs', () => {
@Component({
selector: 'ani-cmp',
animations: [
trigger(
'anim',
[
transition(
'* => enter',
[
query(':enter', [animate(1000, style({color: 'red'}))]),
]),
]),
],
template: `
<section class="container" [@anim]="exp ? 'enter' : 'leave'">
<div class="a" *ngIf="exp">
<div class="b" *ngIf="exp">
<div class="c" *ngIf="exp">
<div class="b">
<div class="c">
text
</div>
</div>
</div>
<div>
<div class="d" *ngIf="exp">
<div class="d">
<div class="e">
text2
</div>
</div>
</section>
`
})
class Cmp {
// TODO(issue/24571): remove '!'.
public exp!: boolean;
}
})
class Cmp {
public exp = false;
}

TestBed.configureTestingModule({declarations: [Cmp]});
TestBed.configureTestingModule({declarations: [Cmp]});

const engine = TestBed.inject(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
const container = fixture.elementRef.nativeElement;
const engine = TestBed.inject(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;

cmp.exp = true;
fixture.detectChanges();
engine.flush();
cmp.exp = true;
fixture.detectChanges();
engine.flush();

let players = getLog();
resetLog();
expect(players.length).toEqual(2);
const [p1, p2] = players;
let players = getLog();
resetLog();
expect(players.length).toEqual(5);
const [p1, p2, p3, p4, p5] = players;

expect(p1.element.classList.contains('a')).toBeTrue();
expect(p2.element.classList.contains('d')).toBeTrue();
expect(p1.element.classList.contains('a')).toBeTrue();
expect(p2.element.classList.contains('b')).toBeTrue();
expect(p3.element.classList.contains('c')).toBeTrue();
expect(p4.element.classList.contains('d')).toBeTrue();
expect(p5.element.classList.contains('e')).toBeTrue();
});

cmp.exp = false;
fixture.detectChanges();
engine.flush();
// TODO: this should most likely not happen anymore and all children of a removed parent
// should be marked as :leave (so that they can be queried accordingly)
Comment on lines 2576 to 2577
Copy link
Copy Markdown
Contributor Author

@dario-piotrowicz dario-piotrowicz Nov 21, 2021

Choose a reason for hiding this comment

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

my changes are only fixing the :enter issue, the :leave one can be treated separately (of if you want I can look into it and add it to this PR)

it('should only mark outermost *directive nodes :leave when removals occur', () => {
@Component({
selector: 'ani-cmp',
animations: [
trigger(
'anim',
[
transition(
'* => leave',
[
query(':leave', [animate(1000, style({color: 'blue'}))]),
]),
]),
],
template: `
<section class="container" [@anim]="exp ? 'enter' : 'leave'">
<div class="a" *ngIf="exp">
<div class="b" *ngIf="exp">
<div class="c" *ngIf="exp">
text
</div>
</div>
</div>
<div>
<div class="d" *ngIf="exp">
text2
</div>
</div>
</section>
`
})
class Cmp {
public exp = true;
}

players = getLog();
resetLog();
expect(players.length).toEqual(2);
const [p3, p4] = players;
TestBed.configureTestingModule({declarations: [Cmp]});

expect(p3.element.classList.contains('a')).toBeTrue();
expect(p4.element.classList.contains('d')).toBeTrue();
});
const engine = TestBed.inject(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
fixture.detectChanges();
cmp.exp = false;
fixture.detectChanges();
engine.flush();

let players = getLog();
resetLog();
expect(players.length).toEqual(2);
const [p1, p2] = players;

expect(p1.element.classList.contains('a')).toBeTrue();
expect(p2.element.classList.contains('d')).toBeTrue();
});

it('should collect multiple root levels of :enter and :leave nodes', () => {
@Component({
Expand All @@ -2599,22 +2639,22 @@ describe('animation query tests', function() {
transition(
'* => *',
[
query(':leave', [animate('1s', style({opacity: 0}))], {optional: true}),
query(':enter', [animate('1s', style({opacity: 1}))], {optional: true})
query('.q:leave', [animate('1s', style({opacity: 0}))], {optional: true}),
query('.q:enter', [animate('1s', style({opacity: 1}))], {optional: true})
])
])],
template: `
<div [@pageAnimation]="status">
<header>
<div *ngIf="!loading" class="title">{{ title }}</div>
<div *ngIf="loading" class="loading">loading...</div>
<div *ngIf="!loading" class="title q">{{ title }}</div>
<div *ngIf="loading" class="loading q">loading...</div>
</header>
<section>
<div class="page">
<div *ngIf="page1" class="page1">
<div *ngIf="page1" class="page1 q">
<div *ngIf="true">page 1</div>
</div>
<div *ngIf="page2" class="page2">
<div *ngIf="page2" class="page2 q">
<div *ngIf="true">page 2</div>
</div>
</div>
Expand Down
10 changes: 2 additions & 8 deletions packages/core/test/animation/animations_with_web_animations_integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,7 @@ describe('animation integration tests using web animations', function() {
@Component({
selector: 'ani-cmp',
template: `
<div @auto *ngIf="exp">
<div style="line-height:20px;">1</div>
<div style="line-height:20px;">2</div>
<div style="line-height:20px;">3</div>
<div style="line-height:20px;">4</div>
<div style="line-height:20px;">5</div>
</div>
<div @auto *ngIf="exp" style="height: 100px"></div>
`,
animations: [trigger(
'auto',
Expand Down Expand Up @@ -315,7 +309,7 @@ describe('animation integration tests using web animations', function() {
player = engine.players[0]! as TransitionAnimationPlayer;
let queriedPlayers =
((player as TransitionAnimationPlayer).getRealPlayer() as AnimationGroupPlayer).players;
expect(queriedPlayers.length).toEqual(5);
expect(queriedPlayers.length).toEqual(10);

let i = 0;
for (i = 0; i < queriedPlayers.length; i++) {
Expand Down
7 changes: 3 additions & 4 deletions packages/platform-browser/animations/src/animation_renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,13 +164,12 @@ export class BaseAnimationRenderer implements Renderer2 {

appendChild(parent: any, newChild: any): void {
this.delegate.appendChild(parent, newChild);
this.engine.onInsert(this.namespaceId, newChild, parent, false);
this.engine.onInsert(this.namespaceId, newChild, parent);
}

insertBefore(parent: any, newChild: any, refChild: any, isMove: boolean = true): void {
insertBefore(parent: any, newChild: any, refChild: any): void {
this.delegate.insertBefore(parent, newChild, refChild);
// If `isMove` true than we should animate this insert.
this.engine.onInsert(this.namespaceId, newChild, parent, isMove);
this.engine.onInsert(this.namespaceId, newChild, parent);
}

removeChild(parent: any, oldChild: any, isHostElement: boolean): void {
Expand Down