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

Fix tooltip animation when target changes while animating #5005

Merged
merged 7 commits into from
Dec 12, 2017

Conversation

jcopperfield
Copy link
Contributor

@jcopperfield jcopperfield commented Nov 29, 2017

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

Fixes #4989

 - tooltip in 'index' mode doesn't animate smoothly.
etimberg
etimberg previously approved these changes Nov 29, 2017
simonbrunel
simonbrunel previously approved these changes Nov 30, 2017
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jcopperfield

@simonbrunel simonbrunel added this to the Version 2.8 milestone Nov 30, 2017
@simonbrunel simonbrunel changed the title Fix issue #4989 Fix tooltip animation when target changes while animating Nov 30, 2017
@simonbrunel
Copy link
Member

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

@jcopperfield
Copy link
Contributor Author

@simonbrunel codepen example of the new behavior.

@simonbrunel simonbrunel dismissed their stale review November 30, 2017 15:10

Could be bugged actually

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

chartjs 5005

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jcopperfield
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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

@simonbrunel
Copy link
Member

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 :\

@jcopperfield
Copy link
Contributor Author

@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

view[key] = c1.mix(c0, ease).rgbString();
continue;
}
c1 = color(target);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Contributor Author

jcopperfield commented Dec 3, 2017

@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
Copy link
Member

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

@jcopperfield
Copy link
Contributor Author

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

@simonbrunel
Copy link
Member

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
Copy link
Contributor Author

That might work. I will try that.

Add: use 'tooltip._start' as workaround check for tooltip animation status
@jcopperfield
Copy link
Contributor Author

@jcopperfield
Copy link
Contributor Author

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

@etimberg etimberg merged commit 4e47c17 into chartjs:master Dec 12, 2017
@jcopperfield jcopperfield deleted the PR-20171129-4989 branch December 13, 2017 06:27
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
* 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
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants