fix(animations): `animateChild` shouldn't trigger animation on disabled element #37724

Zuckjet · 2020-06-25T03:34:07Z

…ations

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
Feature
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: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

Fixes 35558

googlebot · 2020-06-25T03:34:10Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-06-25T04:24:52Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Currently child animation will be triggered by `animateChild()` despite it has been disabled. These changes add some logic to prevent that unexpected behavior. PR Close angular#37724

Zuckjet · 2020-06-25T11:45:19Z

By the way , the test file has some typo errors before. And I fix it with this PR together.

Currently child animation will be triggered by `animateChild()` despite it has been disabled. These changes add some logic to prevent that unexpected behavior. PR Close angular#37724

Zuckjet · 2021-06-15T12:15:20Z

The cr/circleci error seems is not relevant with this PR.
It has passed almost one year since first commit, and I am glad to help If there is something I can do to help this PR go on.

jessicajaniuk · 2021-06-15T16:13:59Z

I've restarted the CI target and am running a google3 presubmit.

crisbeto · 2021-06-15T17:51:36Z

packages/animations/browser/src/render/transition_animation_engine.ts

@@ -1033,7 +1033,10 @@ export class TransitionAnimationEngine {
        // instead stretch the first keyframe gap up until the animation starts. The
        // reason this is important is to prevent extra initialization styles from being
        // required by the user in the animation.
-        instruction.timelines.forEach(tl => tl.stretchStartingKeyframe = true);
+        instruction.timelines = instruction.timelines.filter(tl => {


Overall the changes look good, but this filter with a side effect looks weird since filter predicate functions are supposed to be pure. I'd recommend doing this instead:

const timelines = []; instruction.timelines.forEach(tl => { tl.stretchStartingKeyframe = true; if (!this.disabledNodes.has(tl.element)) { timelines.push(tl); } }); instruction.timelines = timelines;

Thanks, have updated code

Currently child animation will be triggered by `animateChild()` despite it has been disabled. These changes add some logic to prevent that unexpected behavior. PR Close angular#37724

crisbeto

LGTM

Zuckjet · 2021-07-01T06:59:39Z

friendly ping @IgorMinar

jessicajaniuk · 2021-12-13T19:00:37Z

@Zuckjet Looks like there's a code conflict here. Can you rebase this? I think we should be able to land it. Once you rebase, I'll re-run a presubmit and we should be green.

Currently child animation will be triggered by `animateChild()` despite it has been disabled. These changes add some logic to prevent that unexpected behavior. PR Close angular#37724

Zuckjet · 2021-12-14T02:43:41Z

@Zuckjet Looks like there's a code conflict here. Can you rebase this? I think we should be able to land it. Once you rebase, I'll re-run a presubmit and we should be green.

have fixed the conflict.

jessicajaniuk · 2021-12-14T17:37:05Z

Everything looks good. Thanks @Zuckjet!

alxhub · 2021-12-14T21:05:03Z

This PR was merged into the repository by commit bab7ed3.

Currently child animation will be triggered by `animateChild()` despite it has been disabled. These changes add some logic to prevent that unexpected behavior. PR Close #37724 PR Close #37724

…#37724) Currently child animation will be triggered by `animateChild()` despite it has been disabled. These changes add some logic to prevent that unexpected behavior. PR Close angular#37724 PR Close angular#37724

angular-automatic-lock-bot · 2022-01-14T16:10:21Z

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

googlebot added the cla: no label Jun 25, 2020

pullapprove bot requested a review from matsko June 25, 2020 03:34

Zuckjet force-pushed the animations/animate-child branch from 45942e3 to 68ecf2f Compare June 25, 2020 04:24

googlebot added cla: yes and removed cla: no labels Jun 25, 2020

Zuckjet force-pushed the animations/animate-child branch from 68ecf2f to e5e1f92 Compare June 25, 2020 04:38

Zuckjet force-pushed the animations/animate-child branch from e5e1f92 to fcd2c9f Compare June 25, 2020 05:40

Zuckjet force-pushed the animations/animate-child branch from fcd2c9f to dfd3af5 Compare June 25, 2020 11:07

Zuckjet changed the title ~~fix(animations): animateChild should not invoke disabled child anim…~~ fix(animations): animateChild shouldn't trigger animation on disabled element Jun 25, 2020

mhevery added the area: animations label Jun 25, 2020

ngbot bot added this to the needsTriage milestone Jun 25, 2020

Zuckjet force-pushed the animations/animate-child branch from dfd3af5 to 28aa7ee Compare June 26, 2020 04:33

pullapprove bot requested a review from mhevery June 26, 2020 04:34

Zuckjet force-pushed the animations/animate-child branch from 28aa7ee to 01edf66 Compare June 26, 2020 05:14

Zuckjet force-pushed the animations/animate-child branch from 01edf66 to 93fa7c0 Compare June 26, 2020 05:59

Zuckjet force-pushed the animations/animate-child branch from 93fa7c0 to 6012442 Compare June 26, 2020 06:45

Zuckjet force-pushed the animations/animate-child branch from 6012442 to 7dccfd7 Compare June 26, 2020 06:58

Zuckjet force-pushed the animations/animate-child branch from 7dccfd7 to c92325f Compare June 26, 2020 07:11

pullapprove bot removed the request for review from matsko August 27, 2020 14:35

crisbeto reviewed Jun 15, 2021

View reviewed changes

Zuckjet force-pushed the animations/animate-child branch from 622778c to 4101aca Compare June 16, 2021 03:09

crisbeto approved these changes Jun 16, 2021

View reviewed changes

jessicajaniuk removed the request for review from IgorMinar December 13, 2021 18:58

jessicajaniuk removed the risk: medium label Dec 13, 2021

ngbot bot modified the milestones: needsTriage, Backlog Dec 13, 2021

jessicajaniuk added the risky Identifies a PR that has a level of risk to merging label Dec 13, 2021

Zuckjet force-pushed the animations/animate-child branch from 4101aca to a270dcf Compare December 14, 2021 02:01

fix(animations): should not invoke disabled child animations

d60c34f

Currently child animation will be triggered by `animateChild()` despite it has been disabled. These changes add some logic to prevent that unexpected behavior. PR Close angular#37724

Zuckjet force-pushed the animations/animate-child branch from a270dcf to d60c34f Compare December 14, 2021 02:20

jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Dec 14, 2021

alxhub closed this in bab7ed3 Dec 14, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(animations): `animateChild` shouldn't trigger animation on disabled element #37724

fix(animations): `animateChild` shouldn't trigger animation on disabled element #37724

Zuckjet commented Jun 25, 2020 •

edited

googlebot commented Jun 25, 2020

googlebot commented Jun 25, 2020

Zuckjet commented Jun 25, 2020 •

edited

Zuckjet commented Jun 15, 2021 •

edited

jessicajaniuk commented Jun 15, 2021

crisbeto Jun 15, 2021

Zuckjet Jun 16, 2021

crisbeto left a comment

Zuckjet commented Jul 1, 2021

jessicajaniuk commented Dec 13, 2021

Zuckjet commented Dec 14, 2021

jessicajaniuk commented Dec 14, 2021

alxhub commented Dec 14, 2021

angular-automatic-lock-bot bot commented Jan 14, 2022

fix(animations): animateChild shouldn't trigger animation on disabled element #37724

fix(animations): animateChild shouldn't trigger animation on disabled element #37724

Conversation

Zuckjet commented Jun 25, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Jun 25, 2020

googlebot commented Jun 25, 2020

Zuckjet commented Jun 25, 2020 • edited

Zuckjet commented Jun 15, 2021 • edited

jessicajaniuk commented Jun 15, 2021

crisbeto Jun 15, 2021

Choose a reason for hiding this comment

Zuckjet Jun 16, 2021

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

Zuckjet commented Jul 1, 2021

jessicajaniuk commented Dec 13, 2021

Zuckjet commented Dec 14, 2021

jessicajaniuk commented Dec 14, 2021

alxhub commented Dec 14, 2021

angular-automatic-lock-bot bot commented Jan 14, 2022

fix(animations): `animateChild` shouldn't trigger animation on disabled element #37724

fix(animations): `animateChild` shouldn't trigger animation on disabled element #37724

Zuckjet commented Jun 25, 2020 •

edited

Zuckjet commented Jun 25, 2020 •

edited

Zuckjet commented Jun 15, 2021 •

edited