fix(animations): allow enter animation of every inserted element by dario-piotrowicz · Pull Request #44243 · angular/angular

dario-piotrowicz · 2021-11-21T11:31:14Z

currently in the animations package not all newly inserted elements
are treated as entering ones, thus querying them via ':enter' does not
always work as expeted, change such behavior so that all newly inserted
elements are considered entering ones

solves: #30477

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix (sort of)
Feature (sort of)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #30477

In the angular package querying "entering" elements not always work, this has caused confusion amongst developers as some of their animations were not being applied as they expected to

(for more details see the issue thread and PR #44216)

What is the new behavior?

All elements added to the DOM are not treated as "moving" ones as such they can be queried with ":enter" as one would expect to

Does this PR introduce a breaking change?

Yes
No
Likely

Other information

I think that the changes here are debatable, the behaviour of not treating all new elements as "entering"/"move" ones seems to have been put in place purposely, I believe that the logic being was to only allow ":enter" queries to work on elements that enter by their own accord and not those that enter because of their parents, if an element enters because of its parent then the parent's transition can be ":enter" and the query will be specific for the child itself.
Thus I am not 100% sure if these changes are actually providing a real benefit, or if we should just change the documentation to make clear when child elements can be queried or not based on the existing implementation (meaning, is it really important to be able to do transition(":enter", [ query(":enter", ...? does that provide a clear better developer experience? are there instances this implementation doesn't provide the functionality needed?)
some aio docs animations don't work properly because of this issue (or the lack of clarity around it)

dario-piotrowicz · 2021-11-21T11:48:46Z

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 🤔

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

Or "treated"?

"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 😆)

dario-piotrowicz · 2021-11-21T11:49:49Z

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)

dario-piotrowicz · 2021-11-21T11:50:34Z

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)

currently in the animations package not all newly inserted elements are treated as entering ones, thus querying them via ':enter' does not always work as expeted, change such behavior so that all newly inserted elements are considered entering ones solves: angular#30477

jessicajaniuk

LGTM 🍪

Other than that one spelling error this looks good.

jessicajaniuk · 2021-11-22T20:13:53Z

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

JoostK

Drive-by-comment about one change in particular; I have not attempted to understand the problem nor the approach taken by this PR (and that would likely require significant time as the animations logic is 🤯)

JoostK · 2021-11-22T21:01:21Z

  }
 }

+function addClassRecursively(element: any, className: string) {


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.

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?

dario-piotrowicz · 2021-11-23T20:14:43Z

Thanks so very much for the reviewing all 🙂

In the issue's thread (#30477) we've agreed that the current behavior is correct and that instead of changing it, it'd be better to keep it as is and instead clarify/improve the docs, so that we can avoid future misunderstandings/confusion like the one that lead to the issue being opened

angular-automatic-lock-bot · 2021-12-24T16:06:02Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla Bot added the cla: yes label Nov 21, 2021

pullapprove Bot requested review from crisbeto and jessicajaniuk November 21, 2021 11:31

dario-piotrowicz force-pushed the query-enter-change branch from e8fdf5d to d3d5b43 Compare November 21, 2021 11:34

dario-piotrowicz mentioned this pull request Nov 21, 2021

docs(animations): remove incorrect information regarding animateChild #44216

Closed

14 tasks

dario-piotrowicz force-pushed the query-enter-change branch from d3d5b43 to a88bf99 Compare November 21, 2021 11:46

dario-piotrowicz commented Nov 21, 2021

View reviewed changes

dario-piotrowicz force-pushed the query-enter-change branch from a88bf99 to 87475d7 Compare November 21, 2021 12:41

jessicajaniuk added the area: animations legacy animations package only. Otherwise use area: core. label Nov 22, 2021

ngbot Bot added this to the Backlog milestone Nov 22, 2021

jessicajaniuk approved these changes Nov 22, 2021

View reviewed changes

JoostK reviewed Nov 22, 2021

View reviewed changes

dario-piotrowicz mentioned this pull request Nov 23, 2021

animateChild does not work when used in router transitions #30477

Closed

dario-piotrowicz closed this Nov 23, 2021

dario-piotrowicz mentioned this pull request Nov 27, 2021

Enter and leave animations don't behave as expected with parent-child relationships #35825

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Dec 24, 2021

	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;
	}
	}

	_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;
	};
	}

Conversation

dario-piotrowicz commented Nov 21, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

dario-piotrowicz Nov 21, 2021

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Nov 21, 2021

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

JoostK Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz commented Nov 23, 2021

Uh oh!

angular-automatic-lock-bot Bot commented Dec 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dario-piotrowicz Nov 21, 2021 •

edited

Loading

JoostK Nov 22, 2021 •

edited

Loading