Respect the hover mode false - bugfix (#3181)

[sasos90]

Fix for preventing the onProgress callback to be executed when hover mode is set to false in global options.

[simonbrunel]

Looks good, @etimberg do you see any cases where not rendering when hover.mode == false can be problematic?

@sasos90 would be great to update the documentation with that new hover.mode value, but also that animationDuration is ignored if hover.mode is false

[etimberg]

I think this is good. I cannot think of a case where we need to render when there is no hover. Tooltips still work, right?

[sasos90]

I agree. I will double check that tomorrow ok? I already tried to test it fully, but unfortunately I had other things to do. I am going to post the feedback in around 12 hours.

[etimberg]

ok, thanks for checking @sasos90

[sasos90]

Ok one thing that I found could be the problem:

• onComplete callback for animation is still executed when hover mode is set to false. So if you are hovering the bar, while animating, the tooltips will be shown after animation is complete and they will remain on the screen even when hover is out.

So my question is, what should it do, when you have hover mode set on false, but tooltips enabled? Keep in mind, that I still want to onComplete be executed because I am adding the bar values manually on the canvas. That's a bit tricky now. These values on the bars should be put in the core somehow, so we can completely disable the execution of onProgress and onComplete callbacks when hover mode is set to false.

What is your opinion @etimberg and @simonbrunel ?

[etimberg]

When hover mode is off but tooltips are on and not custom (html), I think we still have to render because the tooltip will need to draw. The unfortunate reality of the canvas is that we have to redraw the entire thing to erase something.

The alternate is having 2 canvases on top of each other and using a different canvas for the tooltip but that cannot easily be done right now

[simonbrunel]

You right, not calling render() here causes issues with tooltips. An alternative solution would be me.render(hoverOptions.mode === false? 0 : hoverOptions.animationDuration, true) but then why we would need hover.mode: false? You could simply use hover.animationDuration: 0 (see updated fiddle). Am I missing something?

[etimberg]

hover.mode: false also doesn't change any elements (applying the hover styling). We can probably not render if hover.mode === false && tooltips.mode === false

[simonbrunel]

Just noticed that there is already a way to disable tooltips: tooltips.enabled. To keep the API simple and consistent, I would not introduce another way to do that. If a new option is needed to disable hover style, it should be hover.enabled, right? I would also allow tooltips: false and hover: false as an enabled: false shortcut.

Agree that hover.enabled == false and tooltips.enabled == false``render() should skip rendering, however I don't think that it will solve @sasos90 issue since onComplete() will not be called.

[sasos90]

Ok then we should perhaps leave it untouched and rather fix the automation of adding values to bars. You could use a callback for that if needed. Because clearly it won't work they way I need it, therefore I will just create my own version of these fixes just to prevent the processing in the background.

[simonbrunel]

@sasos90 does hover { animationDuration: 0 } fix your flickering issue?

[sasos90]

No, actually onComplete is then executed many times, so it is still called. Actually it is not flickering but onComplete is executed. But I think it is more of my personal problem of expecting some features to work which are not meant to be implemented that way. Perhaps the only solution to my issue is, changing the core to add bar values automatically (with option to disable it of course).

[simonbrunel]

So "yes" it fixes the flickering issue, but "no", you are not happy with that solution - me neither :)

I think that the best way to implement your feature is to create a simple plugin and implement drawing in afterDatasetsDraw (draft example). I don't see any good reason to not draw for all frames during animation, especially if you make your code a bit more optimized. We can implement what @etimberg suggested (not call render() if tooltip.enabled and hover.enabled are false) so your draw method will not be called anymore on mouse move. But if tooltips or hover are enabled, your draw method must be called for each animation steps to avoid flickering.

[etimberg]

We might be able to add another check too that also only renders if the hovered items or the tooltip items changed. That way mouseover that doesn't change the chart doesn't render.

I think the plugin example might be the better way to go and it would have simpler code.

[sasos90]

Uh that afterDatasetDraw solution is pretty awesome, because the values slides with the bar while animating. In that case I don't have any problems with animation callbacks, because I can completely turn it off (I suppose). The thing is that I have many graphs on the same page, and for each of them I have some special conditions to show the values (different X and Y offsets). If I register that plugin implementation before each creation and update (replot) of data, it happens that one implementation overrides another so the values become weird. Can I specify the plugin for each instance seperately?

Thanks

[simonbrunel]

Only one global instance of the plugin is created and will be called for all charts. You don't need to have a different instance for each chart because the plugin can accept extra chart options (e.g. overlay.labels.yOffset) and then check that value in afterDatasetDraw (see fiddle). If you need different offset per datasets, then you should be able to adapt that code to match exactly what you need.

[sasos90]

Hm.. then I would have a problem. I have stacked bars (so 2 bars) where the bar above have it's own value with the sum of the bar value below. So in updating of the graph I have different data to show, that means I need to call the plugin register again, since there is different response. Kind of complicated huh? You know when you update the graph data, you need to re-implement the onComplete or onProgress callbacks for animation if you want to have new values applied to the graph.

[simonbrunel]

The plugin I wrote is draft and specific to your initial fiddle and will not accommodate to all user cases. If your plugin needs to behave differently based on some extra conditions, then add new options (or even dataset properties) specific to your case and do whatever checks/changes are needed to accomplish your goal inside this plugin.

Not sure what you mean by "update the graph data", but keep in mind that there are many more plugin APIs, so maybe in your case, you will also need to implement afterDatasetsUpdate (or another more suitable) to pre/post process graph data before rendering.

But you should never call Chart.plugins.register multiple time for the same plugin, that's how Chart.js plugins work and I don't understand why you would need to do that. Plugins are great to implement generic (but configurable) features that can apply to many charts. If you want radically different implementations for each chart, then plugins are maybe not the solution.

I hope that helps :)

[sasos90]

Yea I think I would need to do many if statements in that plugin to properly set X, Y, values and so on. It is a bit problematic. Otherwise when there is only one graph on the page, that plugin is doing the great job.

[simonbrunel]

Yeah, maybe, can't say without more details and images about what you're trying to achieve. Your case seems too specific to be implemented in the core of the library and I don't have other suggestions to help you (maybe @etimberg can help more?). Should we close this PR then?

[sasos90]

Actually integrating to put the values for bars in the core would solve the issue, instead of writing the plugin. Throwing some callbacks where we can edit the X, Y, value and that's it. Not sure how important is that for Chart.JS, but as far as I see, there are a lot of people asking about these values on the bars.

[simonbrunel]

You are really welcome to update your PR or submit a new one with the change you have made. If you need guidance about how to efficiently implement your case, please give more details and create a fiddle or some screenshots / mockups about what you are trying to do. It will help to better understand what the current limitations.

Anyway, I was suggesting to close this PR since it has deviated far from its initial purpose.

[sasos90]

Basically all I wanted is, to not repeatedly call (render) onProgress method while hovering the graph since the values were flickering. Therefore everything was a bit laggy also. Perhaps we really need to agree whether to implement that feature or not with @etimberg.

[etimberg]

I think the plugin is the best solution to your problem. I have seen lots of requests for labels built in to the core, but I do think it should probably live in a plugin because it will have its own features, etc. It's impossible for us to support every case and even simple cases become complicated when you have multiple lines to display, etc.

If you can post a fiddle with your plugin and a screenshot of what you want to achieve I can see if any ideas come to mind

[sasos90]

Huh I will give it a try if I manage to get the time. Im further in the project and my time for testing ChartJS is minimal now.

[etimberg]

Closing as out of date