Fix tooltip animation when target changes while animating #5005

jcopperfield · 2017-11-29T09:25:29Z

tooltip in 'index' mode doesn't animate smoothly.

Fixes #4989

- tooltip in 'index' mode doesn't animate smoothly.

simonbrunel

Thanks @jcopperfield

simonbrunel · 2017-11-30T13:40:13Z

@jcopperfield do you have a fiddle that shows the new behavior ?

jcopperfield · 2017-11-30T14:56:51Z

@simonbrunel codepen example of the new behavior.

Could be bugged actually

simonbrunel · 2017-11-30T15:16:41Z

src/core/core.tooltip.js

 			}

+			// always call me.pivot before returning to
+			// reset me._start for smooth animations issue #4989
+			me.pivot();


I don't think me.pivot() should be called if the _active target(s) didn't change, since it resets the animation start/end values. I think that's what caused the weird animation accelerations in the following gif.

@simonbrunel not calling me.pivot() codepen causes a much weirder experience by resetting all settings to their initial values. (sorry don't have any software to make a fancy gif).
Any suggestions?

I can't debug right now :\ Maybe _active is different depending on the hovered bar, so the first changed value is true because of the array order?! Though, I'm not sure why checking the latest value of changed doesn't work:

// {...} if (changed) { me.pivot(); } return changed; }

BTW, the following looks weird (line 869):

if (changed) { // {...} // See if our tooltip position changed changed |= (model.x !== me._model.x) || (model.y !== me._model.y); }

because if changed is true then changed |= true or false is necessary true, right?

@simonbrunel I agree that line 869 is obsolete, however it was there already obsolete from a previous pull.
The problem doesn't lay in this handleEvent, but in its caller, which |= its changed with the result of the tooltip.handleEvent (changed |= tooltip && tooltip.handleEvent(e);). When the caller has changed, but the tooltip hasn't, the bug presents itself, because the animation will run. I don't understand the animation system well enough to prevent the tooltip animation restarting.

I'm wondering if it should not be changed &= (model.x !== me._model.x) || (model.y !== me._model.y); instead?

The issue seems to be that we detect a change for the highlighted item resetting the current animation. Calling pivot() as you do makes things better in this case but breaks animations when hover interaction mode/intersect are the same as tooltip (master - PR).

mode, when target doesn't change.

jcopperfield · 2017-11-30T21:46:55Z

@simonbrunel The latest push is in the handler that is causing the problem. I'm not sure if the animation behaviour with hoverBackgroundColor is as it should be, as I have no clue what the preferred behaviour is. Maybe the problem is that the tooltip doesn't set the tooltip.animation or that different animations are handled by the same process.

jcopperfield · 2017-12-03T08:25:46Z

@simonbrunel The new PR seems to be working a bit better than the previous one.
codepen multiple charts v.2.7 & PR

// v2.7
changed |= tooltip && tooltip.handleEvent(e);

// PR
if (tooltip) {
    changed = tooltip.handleEvent(e);
}

simonbrunel · 2017-12-03T09:02:53Z

It doesn't work, hover events are broken (skipped) as soon as the tooltip reaches its last position (and so returns false to changed). It happens when intersects and/or mode are different between tooltips and hover.

For example: https://codepen.io/anon/pen/mqvKJB

        tooltips: {
            mode: 'index',
            intersect: false
        },
        hover: {
            mode: 'index',
            intersect: true
        },

doesn't work if you move the mouse at the same index but over and outside a bar.

jcopperfield · 2017-12-03T09:35:01Z

@simonbrunel I see. So should there be a flag tooltip.animating implemented in some part to resolve this problem?

simonbrunel · 2017-12-03T10:09:54Z

If think the problem is that animations are handled globally and thus are not independent from each others, so when there is a change anywhere, we restart all current animations (I'm not even sure we can have animations with different durations?!) Ideally, animations should be handled separately and locally: the tooltip plugin should be able to trigger an animation with its own parameters, stop/restart this animation without impacting others, etc.

But I think that would require a rewrite of the animation service which is out the scope of this PR (and could be a bit complicated). Though, I'm investigating this solution to animate stuff in the datalabels plugin. Right now I don't think of a correct way to easily fix that issue, the flag suggestion could "maybe" work but will certainly be hacky to synchronize :\

to keep track of tooltip animation.

jcopperfield · 2017-12-03T11:57:46Z

@simonbrunel I agree that a per element animation service would be the optimal solution.

Introducing a setAnimating argument in transition to flag the tooltip as animating allows for a finer grained checking to resolve the case:

tooltips: {
    mode: 'index',
    intersect: false
},
hover: {
    mode: 'index',
    intersect: true
}

example PR

simonbrunel · 2017-12-03T17:10:38Z

src/core/core.element.js

-						view[key] = c1.mix(c0, ease).rgbString();
-						continue;
-					}
+				c1 = color(target);


Why changing that part of the code? Your changes actually decrease performances because c0 and c1 are now always evaluated (color()), which is a very costly operation while transitioning values.

simonbrunel · 2017-12-03T17:12:34Z

I would avoid adding a flag to the base element class to workaround an issue with the animation service and especially with the tooltip. It's also semantically incorrect since you force the element to report an animating state while it's not when !model || ease === 1 (very confusing).

If you really want to rely on a flag, I would keep that workaround at the highest level, so in the tooltip class and private _skipUpdateWorkaround to avoid anyone from using it (it's very tempting to use element.animating but it doesn't work as expected!)

Don't take care of the newly reported "cognitive complexity" of CodeClimate, I'm going to increase it.

jcopperfield · 2017-12-03T17:21:17Z

@simonbrunel the me.animating flag only applies to the tooltip, not to the element.animating. Unfortunately the tooltip.transition code is called from inside the core.controller.js. When (!model || ease === 1) the me.animating flag is removed, because the animation/transition has finished. Don't really understand what the confusing part is. Could you explain?
I could rename the setAnimating to isTooltip if that would makes things clearer.

simonbrunel · 2017-12-03T17:36:31Z

The reviewed code was this one, I didn't notice you changed it for delete me.animating in your last commit.

jcopperfield · 2017-12-03T17:37:59Z

I will rename the setAnimating to isTooltip and me.animating to me._skipUpdateWorkaround to prevent misusing the flag.

simonbrunel · 2017-12-03T17:43:23Z

What about using (still as dirty workaround) the element._start state: if !null, that's mean it's animating:

changed = tooltip._start     // === animating
  ? tooltip.handleEvent(e)
  : changed | tooltip.handleEvent(e);

That would allow to not touch the element class.

jcopperfield · 2017-12-03T17:45:48Z

That might work. I will try that.

Add: use 'tooltip._start' as workaround check for tooltip animation status

jcopperfield · 2017-12-03T18:14:32Z

example of last PR using tooltip._start

jcopperfield · 2017-12-12T10:01:58Z

@simonbrunel Is there anything missing from this PR or PR 4959 that prevents merging?

* Fix issue chartjs#4989 - tooltip in 'index' mode doesn't animate smoothly. * Change: different approach for smooth tooltip animation in 'index' mode, when target doesn't change. * Fix: jslint error * Fix: remove spyOn pivot from test * Add: setAnimating-flag in transition used to set on tooltip.transition to keep track of tooltip animation. * Decrease code complexity * Revert transition and complexity changes Add: use 'tooltip._start' as workaround check for tooltip animation status

Fix issue chartjs#4989

33a51ed

- tooltip in 'index' mode doesn't animate smoothly.

etimberg previously approved these changes Nov 29, 2017

View reviewed changes

simonbrunel previously approved these changes Nov 30, 2017

View reviewed changes

simonbrunel added the type: bug label Nov 30, 2017

simonbrunel added this to the Version 2.8 milestone Nov 30, 2017

simonbrunel changed the title ~~Fix issue #4989~~ Fix tooltip animation when target changes while animating Nov 30, 2017

simonbrunel requested changes Nov 30, 2017

View reviewed changes

jcopperfield added 3 commits November 30, 2017 22:05

Change: different approach for smooth tooltip animation in 'index'

ce679dc

mode, when target doesn't change.

Fix: jslint error

982f0b4

Fix: remove spyOn pivot from test

482e59f

Add: setAnimating-flag in transition used to set on tooltip.transition

38574c3

to keep track of tooltip animation.

Decrease code complexity

f7d4d54

jcopperfield dismissed etimberg’s stale review via f7d4d54 December 3, 2017 17:01

simonbrunel reviewed Dec 3, 2017

View reviewed changes

Revert transition and complexity changes

badc4d9

Add: use 'tooltip._start' as workaround check for tooltip animation status

simonbrunel approved these changes Dec 12, 2017

View reviewed changes

simonbrunel requested a review from etimberg December 12, 2017 13:35

etimberg approved these changes Dec 12, 2017

View reviewed changes

etimberg merged commit 4e47c17 into chartjs:master Dec 12, 2017

jcopperfield deleted the PR-20171129-4989 branch December 13, 2017 06:27

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tooltip animation when target changes while animating #5005

Fix tooltip animation when target changes while animating #5005

jcopperfield commented Nov 29, 2017 •

edited by simonbrunel

simonbrunel left a comment

simonbrunel commented Nov 30, 2017

jcopperfield commented Nov 30, 2017

simonbrunel Nov 30, 2017

jcopperfield Nov 30, 2017

simonbrunel Nov 30, 2017

jcopperfield Nov 30, 2017

simonbrunel Nov 30, 2017

simonbrunel Nov 30, 2017

jcopperfield commented Nov 30, 2017

jcopperfield commented Dec 3, 2017

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

simonbrunel Dec 3, 2017

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017 •

edited

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

jcopperfield commented Dec 12, 2017

Fix tooltip animation when target changes while animating #5005

Fix tooltip animation when target changes while animating #5005

Conversation

jcopperfield commented Nov 29, 2017 • edited by simonbrunel

simonbrunel left a comment

Choose a reason for hiding this comment

simonbrunel commented Nov 30, 2017

jcopperfield commented Nov 30, 2017

simonbrunel Nov 30, 2017

Choose a reason for hiding this comment

jcopperfield Nov 30, 2017

Choose a reason for hiding this comment

simonbrunel Nov 30, 2017

Choose a reason for hiding this comment

jcopperfield Nov 30, 2017

Choose a reason for hiding this comment

simonbrunel Nov 30, 2017

Choose a reason for hiding this comment

simonbrunel Nov 30, 2017

Choose a reason for hiding this comment

jcopperfield commented Nov 30, 2017

jcopperfield commented Dec 3, 2017

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

simonbrunel Dec 3, 2017

Choose a reason for hiding this comment

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017 • edited

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

simonbrunel commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

jcopperfield commented Dec 3, 2017

jcopperfield commented Dec 12, 2017

jcopperfield commented Nov 29, 2017 •

edited by simonbrunel

jcopperfield commented Dec 3, 2017 •

edited