[BUG] onHover not triggered for doughnut chart

[ricardocosta]

The function provided on the onHover option is not being triggered for a doughnut chart. I don't know if this is also affecting other types of chart.

The issue was already reported in #3799, but was closed because nobody presented a test case.

Expected Behavior

The provided function should be called when the mouse hovers a segment of the chart.

Current Behavior

Nothing happens on hover, but the onClick event is working.

Steps to Reproduce (for bugs)

When hovering over on a segment, the tooltip shows but the onHover function is not called.
When clicking a segment, the onClick function is called.
Codepen: https://codepen.io/anon/pen/ZKNGEQ

Context

I was trying to expand (increase outerRadius) a slice of the chart when hovering. If necessary, I can provide a gist with the expansion working for the onClick event.

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

[etimberg]

@ricardocosta89 thanks for providing a test case. I quickly looked into this and the cause is that https://github.com/chartjs/Chart.js/blob/master/src/core/core.controller.js#L812-L815 should use options.onHover not hoverOptions.onHover.

I'm happy to look at a PR fixing this. The docs already list that onHover should be at the top level so no updates there are needed

[etimberg]

Before we fix this we should make sure that it's the code that's wrong and not the docs. We should look at v2.0.0 and see where the onHover option was placed then and match that since that is the major version

[etimberg]

I went back to v2.0.0 and it's always been

options: {
  hover: {
    onHover: function() {
      
    }
  }
}

So that means we need to update the docs.

[ricardocosta]

Oh bummer. I just submitted the PR fixing the code. 😅

[etimberg]

No worries, just change it to update the docs instead 😄

[ricardocosta]

Taking into account that the onClick is provided at the top level, how do you feel about something like this?

var hoverCallback = options.onHover || options.hover.onHover;
// On Hover hook
if (hoverCallback) {
  // Need to call with native event here to not break backwards compatibility
  hoverCallback.call(me, e.native, me.active);
}

It won't introduce breaking changes, but it will keep consistency regarding the onClick.

[etimberg]

@ricardocosta89 just proposed that on the PR 😄 Just waiting to see what @simonbrunel thinks but I like that idea since it's cleaner. I would go ahead and make the code change!

[ricardocosta]

I'll wait for feedback, then.

Also, one thing I just noticed is that the callback is invoked for every mouse movement, so we have a lot of calls despite the segment only being hovered once from the user perspective. Is there a way to prevent this?

[etimberg]

I believe there is an open issue for that. It really depends on what onHover is being used for. If the raw mouse coordinates are needed then it needs to be called all the time. If the user only cares about the item being hovered it only needs to be called when the hover selection changes. I don't know if there's enough consensus to make a change in the Chart.js internals for this but it could definitely be a new sample that demonstrates how to filter the events

[ricardocosta]

In my case I just wanted to increase the outerRadius, so it will be the second approach (hover selection change). I'll try to see if there's anything stating that the hover is on/off to use in my callback.

Edit: It seems the event doesn't drill down inside the canvas element. Unless there's a way to identify if a given set of mouse coordinates belongs to the item surface, it's going to be harder to do this. 😞

[etimberg]

We've already written the intersection code. When onHover is called you get the list of internal items that are hovered. You can use that list to determine if the selection has changed

[ricardocosta]

Yes, I managed to do it tinkering around with Chart.js code. I'll try to do it on my side without exposing the lastActive when calling the callback.

[simonbrunel]

I agree, for consistency I prefer having onHover at the options root (similar to onClick). I like var callback = options.onHover || options.hover.onHover; and I would deprecate hover.onHover, which basically means to not update the docs :)

[ricardocosta]

@simonbrunel Another question I have: is options.hover.animationDuration deprecated? I don't remember seeing it in the docs, but there are still references in the code. And this duration is not being respected because the code enters the bufferedRequest branch (core.controller.js#L774).

[simonbrunel]

I don't think it's deprecated, the bufferedRequest branch is entered only if there is already a render in progress, which one should have been requested with options.hover.animationDuration (core.controller.js#L781).

[ricardocosta]

I expected that, but when trying out with hover for the doughnut chart, it was always entering the bufferedRequest branch. Some underlying bug?

[etimberg]

I think it's always entering the bufferedRequest branch since something called update() during the event handlers

[ricardocosta]

Ah, indeed. I have to call _chart.update() on my onHover callback for the animation to work. Should I open a new issue about this?

[etimberg]

Yup, I think that's a new issue