fix(platform-browser): update started state on reset

[iRealNirmal]

This commit fixes the state of variable _started on call of reset method.

Resolves #18140

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?

on call of hasStarted method after reset method is returning true.

Issue Number: #18140

What is the new behavior?

After calling reset method, hasstarted method will return false.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[petebacondarwin]

LGTM

[AndrewKushnir]

@iRealNirmal the change looks good, I'm wondering if we should reset _started flag to false in finish and destroy methods too (for completeness)? // cc @petebacondarwin

(I will remove "presubmit" flag for now, since additional changes might be required)

[petebacondarwin]

I suggested setting it to false at finish, but @iRealNirmal pointed out that this could be a breaking change, and it is not clear from the docs and the interface what hasStarted() really means. So I would err on the side of caution and not do the additional changes.

[iRealNirmal]

@iRealNirmal the change looks good, I'm wondering if we should reset _started flag to false in finish and destroy methods too (for completeness)? // cc @petebacondarwin

(I will remove "presubmit" flag for now, since additional changes might be required)

@AndrewKushnir I was just worried if other world project are using that flag to know if it was played already or not. If we do that change this will surely break it for them.

Regarding ondestory I don't think we need to set it but ondone / onfinish can be case for reset but for that also I would need clear consent from you or any person from your team.

As soon you can provide me consent where I should change the state( list will be helpful), I will do it.

[0biWanKenobi]

what about pause though? There's still no way of knowing if the player is in a paused state.. Which was part of the issue I had originally created. I agree that the purpose of hasStarted() is not clear, and that we can't potentially introduce breaking changes. So, I would propose a isPaused() or something similar, would that be viable?

[iRealNirmal]

I would like to keep both things separate. When you are asking about isPaused(), I am thinking it more like feature not a fix.

Regarding answer your question regarding viable, yes some time investment and it will be done. But still I would prefer a different ticket for isPaused to have history.

Coming back to hasStarted I would like more clarity from functionality aspect maybe @petebacondarwin and @AndrewKushnir can do some internal brainstorm with other team members. Doing any changes which is not clear is not a good idea I own self wouldn't approve that PR.

[petebacondarwin]

Hi @iRealNirmal and @0biWanKenobi (got to love that zero).

@AndrewKushnir and I just had a long chat about this. There is no one on the team at the moment who understands the animations package deeply or knows all of the history as to why the APIs were designed as they are.

It seems that the hasStarted() API is a bit of an unusual departure from the rest of the API, where one is expected to register callbacks to find out what the state of the player is (e.g. onStart(), onDone(), onDestroy(), etc. I suspect that this was originally an internal flag that was used to prevent animations from being triggered unexpectedly when they had already been triggered (i.e. calling play() more than once. But then when there were players that delegated to other players this flag became public via the hasStarted() method. It is not clear whether this method was ever meant to be used by applications to know the current state of the animation.

What is more worrying is that there are different behaviours between different players. Some of them do reset the _onStarted property when reset() is called. Some do not. Notably we have CssKeyframesPlayer which has:

  hasStarted(): boolean {
    return this._state >= AnimatorControlState.STARTED;
  }

In this case, hasStarted() returns true if the _state is one of STARTED, FINISHED or DESTROYED.

We believe that it is probably the case that reset() should always reset the hasStarted() value.
So for this PR, we would like to propose that @iRealNirmal go through the code and ensure that all players will reset this flag as necessary, adding tests for each if possible. We should also update the documentation for AnimationPlayer to explicitly declare that the hasStarted() value will be updated only by a call to play() or reset(). Once the PR is updated, @AndrewKushnir will run a full presubmit to check whether this change would break any apps inside Google.

Here are the implementation of hasStarted():

• CssKeyframesPlater - this actually has an unused _started property that can be removed; it uses the _state value instead, which should be set to 0 in the reset() method.
• TransitionAnimationPlayer - this delegates to _player so does not need to change.
• WebAnimationsPlayer - this already sets _started to false in reset().
• MockAnimationPlayer - needs a reset() method defining that sets __started to false (note double underscore).
• NoopAnimationPlayer - needs to set _started to false in the (currently empty) reset() method.
• AnimationGroupPlayer - this already sets _started to false in reset().
• RendererAnimationPlayer - this one is has already been fixed in this PR.

Further, if one wants to know the current state of the animation then it seems that we should lean on the onXxx() API. This may mean adding (in a separate PR for 12.1) new functionality of onPause() to complement onStart() and onDone(). But this should not be dealt with in this PR.

[iRealNirmal]

thanks @petebacondarwin and @AndrewKushnir understood what I have to do, Will take care of it.

[AndrewKushnir]

can you remove review state as pr is approved by @crisbeto

@iRealNirmal since this PR changes a d.ts file (testing.d.ts), additional reviews from the public-api group members are required. We'll update the status (back to the merge state) once the changes are approved by the public-api group.

[jessicajaniuk]

LGTM 🍪

reviewed-for: public-api

[alxhub]

Reviewed-for: public-api

[alxhub]

I'm approving this from the public API perspective, but I noticed that the commit message is very sparse, especially compared to the discussion on this PR.

@iRealNirmal, can you update the commit message to capture:

• what the previous behavior was prior to this fix
• why that behavior was wrong, and what consequence it had
• what the new behavior is after the fix

Thanks!

[iRealNirmal]

I'm approving this from the public API perspective, but I noticed that the commit message is very sparse, especially compared to the discussion on this PR.

@iRealNirmal, can you update the commit message to capture:

• what the previous behavior was prior to this fix
• why that behavior was wrong, and what consequence it had
• what the new behavior is after the fix

Thanks!

sure will do.

[iRealNirmal]

Any update on this PR ?

[AndrewKushnir]

Hi @iRealNirmal, sorry for the delay, I've started a new presubmit and will keep this thread updated.

[AndrewKushnir]

@iRealNirmal FYI presubmits went well and since the PR was marked for merge before, I'm adding that label back. I've also rebased this PR to re-run CI and if it's still "green", the PR should be fully ready for merge. Thank you.

[angular-automatic-lock-bot]

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