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.

Sign up for GitHub

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

Jump to bottom

Fix tooltip animation when target changes while animating #5005

Merged
merged 7 commits into from Dec 12, 2017
Merged

Fix tooltip animation when target changes while animating #5005

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
33a51ed
Fix issue #4989
jcopperfield Nov 29, 2017
ce679dc
Change: different approach for smooth tooltip animation in 'index'
jcopperfield Nov 30, 2017
982f0b4
Fix: jslint error
jcopperfield Nov 30, 2017
482e59f
Fix: remove spyOn pivot from test
jcopperfield Nov 30, 2017
38574c3
Add: setAnimating-flag in transition used to set on tooltip.transition
jcopperfield Dec 3, 2017
f7d4d54
Decrease code complexity
jcopperfield Dec 3, 2017
badc4d9
Revert transition and complexity changes
jcopperfield Dec 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 16 additions & 16 deletions src/core/core.tooltip.js
View file
Open in desktop
Expand Up @@ -852,27 +852,27 @@ module.exports = function(Chart) {
// Remember Last Actives
changed = !helpers.arrayEquals(me._active, me._lastActive);

// If tooltip didn't change, do not handle the target event
if (!changed) {
return false;
}
// Only handle target event on tooltip change
if (changed) {
me._lastActive = me._active;

me._lastActive = me._active;
if (options.enabled || options.custom) {
me._eventPosition = {
x: e.x,
y: e.y
};

if (options.enabled || options.custom) {
me._eventPosition = {
x: e.x,
y: e.y
};
var model = me._model;
me.update(true);

var model = me._model;
me.update(true);
me.pivot();

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

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

return changed;
}
});
Expand Down
4 changes: 4 additions & 0 deletions test/specs/core.tooltip.tests.js
View file
Open in desktop
Expand Up @@ -808,19 +808,23 @@ describe('Core.Tooltip', function() {

var tooltip = chart.tooltip;
spyOn(tooltip, 'update');
spyOn(tooltip, 'pivot');

/* Manually trigger rather than having an async test */

// First dispatch change event, should update tooltip
node.dispatchEvent(firstEvent);
expect(tooltip.update).toHaveBeenCalledWith(true);
expect(tooltip.pivot).toHaveBeenCalledWith();

// Reset calls
tooltip.update.calls.reset();
tooltip.pivot.calls.reset();

// Second dispatch change event (same event), should not update tooltip
node.dispatchEvent(firstEvent);
expect(tooltip.update).not.toHaveBeenCalled();
expect(tooltip.pivot).toHaveBeenCalledWith();
});

describe('positioners', function() {
Expand Down