[BUG] tooltip error with stacked (group) bar chart in demo pages

[fengzhenqiong]

Description

I noticed this bug [I think it would be a BUG] in the demo pages of the downloaded zip packages from GitHub, I tried 2.6.0/2.7.0/2.7.1 and they are the same. But this issue does not exists in 2.5.0

Expected Behavior

tooltip move back and forth smoothly when moving mouse pointer between bars

Current Behavior

the tooltip moves/shakes unexpectedly/unpredictably when moving mouse pointer between bars

Possible Solution

I tried setting the intersect to true and the problem was solved, so there might be some problem with this property?

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

Steps to Reproduce (for bugs)

1. Open the stacked.html or stacked-group.html file
2. Move the mouse pointer between bars and groups

Context

I just tried to use the stacked/stacked group bar chart and test it with the demos

Environment

Chartjs 2.6.0/2.7.0/2.7.1
Chrome 60.0.3112.78 64bit and IE 11.0.9600.18738
Windows 7 64bit

[etimberg]

Interesting that this is happening. I recall seeing this a while ago, but I thought it was fixed.

My theory as to why this is happening:

1. The tooltip handleEvent is incorrectly returning true
2. the eventHandler then starts a re-render

[fengzhenqiong]

I compared the handleEvent function in core.tooltip.js of version 2.5 and 2.7 and seems you added following code:

// If tooltip didn't change, do not handle the target event
if (!changed) {
	return false;
}

And if i remove the code lines above the problem disappears. So I guess the code after that is still needed sometimes?

Really hope this information helps.

[jcopperfield]

The problem is that the function handleEvent updates the me._lastActive before all changed checks have been performed.
codepen example hovering over the border over of the first bar of the second data entry before the tooltip animation has finished resets the starting point of the animation, which causes the flickering

Changing the order of execution resolves the problem codepen.

// Remember Last Actives
changed = !helpers.arrayEquals(me._active, me._lastActive);

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

	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);
}

// If tooltip didn't change, do not handle the target event
if (!changed) {
	return false;
}

me._lastActive = me._active;

Unfortunately it fails one test due to calling update. Maybe @etimberg has some insides of what the code should do.

[etimberg]

I think the idea behind my original change was to avoid re-updating the model if the input elements have not changed since in theory the same input elements should lead to the same output model.

Your proposed modification is probably safer though. I think any performance difference could be negligible but I’d have to profile and see.

[jcopperfield]

@etimberg I have to admit that I do not completely understand why it works, so maybe there is something else causing the weird behavior.

[jcopperfield]

@etimberg it turns out that only adding me._start = {}; or calling me.pivot(); before the if (!changed) { does the job as well.
Do you have any idea why?

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

[etimberg]

_start is used for tracking the starting point of an animation. When a key to be animated is not in the start dict, no interpolation occurs and instead it just gets applied. So the animation occurs but there is nothing to change so the tooltip doesn’t move. That’s my hunch any way

[jcopperfield]

@etimberg That I understood, however I don't understand why the when _start isn't cleared in the handleEvent, the animation would restart from a previous position. Does this mean there's a bug in another part of the library or should the behavior be like this and be resolved by adding:

if (!changed) {
	me.pivot();
	return false;
}

What is the task of the pivot function? It doesn't have any JSDoc.

[etimberg]

pivot essentially just clears any animation state from the element. It creates _view if it doesn't exist so that the element can be rendered and clears _start. When a chart is created new elements are actually updated twice. First in a "reset" configuration, then pivot is called on all the items to ensure that the animation system has the reset position as the start of the animation, then another update puts the elements in their final position so that the initial animation looks correct.

I think the bug is somewhere else. I think the current tooltip code is mostly correct in that we should only call pivot after an update since there is potential for an animation at that point.

One thought is perhaps https://github.com/chartjs/Chart.js/blob/master/src/core/core.tooltip.js#L853 is too simplistic. Maybe we need to sort the array in some way so that the comparison is true when the items are the same but the order is different. The downside is that if the order does change and the user wants that the tooltip wouldn't render.

I'll try and do some debugging on this tonight and see where that gets me

[jcopperfield]

@etimberg an error on L853 cannot be the case, since the !changed is called. The only thing needed is to add a call to me.pivot(); before returning false, because in the caller to handleEvent itself is another handleEvent, which |= the result. The problem might be in the caller function.

The following code resolves the problem and passes all tests; it only takes one extra call to me.pivot();.

/**
 * Handle an event
 * @private
 * @param {IEvent} event - The event to handle
 * @returns {Boolean} true if the tooltip changed
 */
handleEvent: function(e) {
    var me = this;
    var options = me._options;
    var changed = false;

    me._lastActive = me._lastActive || [];

    // Find Active Elements for tooltips
    if (e.type === 'mouseout') {
        me._active = [];
    } else {
        me._active = me._chart.getElementsAtEventForMode(e, options.mode, options);
    }

    // Remember Last Actives
    changed = !helpers.arrayEquals(me._active, me._lastActive);

    // If tooltip didn't change, do not handle the target event
    if (!changed) {
        me.pivot(); // reset me._start for smooth animations issue #4989
        return false;
    }

    me._lastActive = me._active;

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

        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);
    }

    return changed;
}
});

[etimberg]

Good point @jcopperfield.

which |= the result. The problem might be in the caller function.

You're right! What's happening is that the hover state is changing which triggers a re-render because changed. The extra pivot solution seems fine to me. If possible, could you make a PR with it? If we want to be even safer, tooltip.pivot() could be called from https://github.com/chartjs/Chart.js/blob/master/src/core/core.controller.js#L825 if we are changing and that would limit the scope of any changes.