[BUG] options.hover.animationDuration not respected

[ricardocosta]

When the onHover/options.hover.onHover callback invokes _chart.update() the duration of the hover animation is not being respected and it uses the animation.duration value.

Expected Behavior

The duration of the hover animation should be the one provided in options.hover.animationDuration instead of animation.duration.

Current Behavior

The hover animation has the same duration has the initial loading animation.

Possible Solution

When _chart.update() is called, the code enters the bufferedRequest branch in core.controller.js#L772. Therefore, the render method is called with a duration that is not the one provided for the hover effect.

Steps to Reproduce (for bugs)

Example gist: https://gist.github.com/ricardocosta89/f4751f9a73113df32c9be5cda10ddecf

Context

I wish to have a different animation duration for the hover effect.

Environment

• Chart.js version: 2.6.0
• Browser name and version: Chrome 58.0.3029.110 (64-bit)
• Operating System: macOS Sierra 10.12.5

[ricardocosta]

@etimberg @simonbrunel I've been looking at possible solutions for this, and this is what I could understand from the codebase:

• Since this happens inside eventHandler, _bufferedRender will be set to true.
• Manually calling update without any arguments, will lead the code execution through the _bufferedRender branch on L369, resulting in the creation of a _bufferedRequest with lazy and duration set to undefined.
• Because _bufferedRequest is truthy, it will enter the bufferedRequest branch on L772, invoking render with undefined parameters. Therefore, the numSteps calculation falls back to animationOptions.duration (L460).

I'd like your opinion regarding possible ways to fix this:

1. Fallback to me.options.hover.animationDuration on L774, and therefore duration will defined when setting numSteps.
2. Since update is being called manually without parameters, establish that it is the caller's responsibility to provide a duration and therefore no code changes are needed.
3. Change update to receive an animation object instead of/in addition to animationDuration. This would allow render to be called with a full object with all the animation properties specific for the hover situation (different duration and easing function) - falling back to options.animation - that would be provided to Chart.Animation.

Personally, I prefer 3 since it provides more flexibility and decouples the "global" animation from the "hover" animation, but you guys are the ones with the final word.

Let me know what you think.

[etimberg]

I like option 3,where the animation is a new property to keep backwards compatibility with the previous code.

[simonbrunel]

In this specific case, I would vote for option 2: if we implement option 3, you will still need to manually call update({animation_config}) so at this point why not simply call update(100)?

In option 3, what is the animation object as argument of update? is it a Chart.Animation or a plain object containing {duration: number, easing: string}? I would definitely not expose a Chart.Animation to the public API, user are not supposed to create this kind of object manually.

If it's a plain object, then it will need to propagate to the render() method. Not sure it will fully solve your issue because calling update() while there is a global animation, will stop the current one and trigger the "hover" one (could use the lazy argument as workaround):

I deeply think that all the chart animations should be independent and each element should be animable separately. However it would certainly require a huge refactor, maybe breaking changes, so would be better to wait v3 for that.

Anyway, if we are going to implement option 3, I would not add an extra parameter but instead gather all options under the first argument as plain object (same for render):

update: function(config) {
    if (!config || typeof config !== "object") {
        // backward compatibility
        config = {
            duration: config,
            lazy: arguments[1]
        };
    }

    var duration = config.duration;
    var lazy = config.lazy;
    // {...}
}

[ricardocosta]

Yes, my idea was to have a plain animation object as argument to update. Thanks, @simonbrunel, for mentioning the side effect of triggering the hover animation during the global one. You mentioned lazy could be a workaround, but I've tried it with both true and false values and the issue still occurs...

However, this issue already exists in the current Chart.js version: if update is triggered during the global animation, the rest of the animation will take the defined duration (you can see this on my gist). Would it be something easy to fix? 🤔

[simonbrunel]

You right, lazy doesn't work. I don't know if it's easy to fix, my first guess would be it's not because it would require to animate elements (and properties) independently: trigger a new animation for the hovered element only, and just for properties affected by the hover event (e.g. the element color). I don't see any obvious workaround for this issue (which as you said already exists in current version) :\

[ricardocosta]

I got this working for my use case: different duration and easing function for the hover event.
However, when trying to run gulp test locally it fails a lot of tests. Most of them are related with canvas resizing. Is this something "expected" or do I need to configure something else locally?

[etimberg]

Resolved in #4362