Skip to content
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.

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

Animation numsteps wrong value #8613

Closed
LeeLenaleee opened this issue Mar 10, 2021 · 6 comments · Fixed by #8616
Closed

Animation numsteps wrong value #8613

LeeLenaleee opened this issue Mar 10, 2021 · 6 comments · Fixed by #8616

Comments

@LeeLenaleee
Copy link
Collaborator

Expected Behavior

The numsteps should be the duration of that animation and not the global one

Current Behavior

the short tooltip animation works but the onProgress callback in the animation block for the options reports back the main long duration.

V2 (correct behaviour): https://www.chartjs.org/samples/latest/advanced/progress-bar.html
V3 (wrong behaviour): https://www.chartjs.org/samples/master/advanced/progress-bar.html

Context

Found this through another issue

Environment

  • Chart.js version: 3.0.0-beta.13 master
  • Browser name and version: Chrome Versie 88.0.4324.190 (Officiële build) (64-bits)
  • Link to your project:
@etimberg
Copy link
Member

Good question as to what the most correct behaviour is here. For example, what happens if the tooltip is configured such that the duration of the position change is 400ms, but the background color changes in 800ms. That's essentially the case that I am seeing happen while debugging this sample. I see 15 different animations, 7 of which are active. Durations range from 2000ms to 200ms.

Active Duration Property From To
true 2000 'borderColor' 'rgb(255, 99, 132)' '#FF4069'
true 2000 'backgroundColor' 'rgb(255, 99, 132)' '#FF4069'
true 2000 'radius' 3 4
false 2000 'borderWidth' 1 1
true 2000 'borderColor' 'rgb(54, 162, 235)' #059BFF'
true 2000 'backgroundColor' 'rgb(54, 162, 235)' '#059BFF'
true 2000 'radius' 3 4
false 2000 'borderWidth' 1 1
false 400 'caretY' 348.107 348.107
false 400 'caretX' `40.76 40.76
false 400 'height' 56 56
false 400 'width' 168 168
false 400 'x' 320.107 320.107
false 400 'y' 47.76 47.76
true 200 'opacity' 0 1

@LeeLenaleee
Copy link
Collaborator Author

LeeLenaleee commented Mar 11, 2021

As an end user with the current implementation of the sample I would say the opacity since when thats 1 you see it, but if you implement it otherwise when you still see the backgroundcolor changing it would be weird to see that the progress bar is already done.

So I think the behaviour should then be to take the animation which takes the longest and show that progressbar (guess that is how it works now) even though it feels really weird to see the tooltip already dissapeared while the appear animation is still in progress

@etimberg
Copy link
Member

I don't think it correctly gets the longest even; it just uses the top level which in this case happens to be the longest. One thought I had was to change the callback to call for each animation that is firing so the callback would be called 7x per step here since there are 7 active animations. Without additional meta-data though, it would be very hard to know what property is for which item. For example, which backgroundColor animation is for which point, etc.

@LeeLenaleee
Copy link
Collaborator Author

If every callback would get metadata about it, it would also solve #8612 since you can filter the right animation out from it

@kurkle
Copy link
Member

kurkle commented Mar 11, 2021

I don't think it correctly gets the longest even; it just uses the top level which in this case happens to be the longest. One thought I had was to change the callback to call for each animation that is firing so the callback would be called 7x per step here since there are 7 active animations. Without additional meta-data though, it would be very hard to know what property is for which item. For example, which backgroundColor animation is for which point, etc.

It finds the longest duration every time animations are started, and use that in the progress event:

anims.duration = anims.items.reduce((acc, cur) => Math.max(acc, cur._duration), 0);

It might be and performance killer to call the onProgress for every animation, because there can be thousands. I think its the proper thing to only call it once a frame. I think we could send the list of animations as a parameter?

@kurkle
Copy link
Member

kurkle commented Mar 11, 2021

I think there is actually some other issue here, the duration might be wrong on those 2000ms animations when hovered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants