Use element options for removeHoverStyle(), even when inheriting

[loicbourgois]

Found an issue with removeHoverStyle() while working on chartjs-chart-financial
See chartjs/chartjs-chart-financial#24

The element used in Chart.controllers.bar.removeHoverStyle() is hardcoded, so if a new class inherits the bar controller and change the default values, these new default values are not applied.

I could not figure how to get the element type associated with a controller, so I added a new attribute : elementType.

It is needed because for the bar controller, the element and the controller type are different.

For a candlestick, controller and element have the same name, so it is not necessary to set elementType.

jsfiddle before : after a hover on the second chart, the border disappears.
jsfiddle after
The difference is in the external resource for Chart.js.

[benmccann]

Thanks for tracking down what's happening in that issue. The core of the issue appears that candlestick subclasses the bar chart and overrides updateElement, but not removeHoverStyle

There's a lot of duplicated code right now between updateElement, setHoverStyle, and removeHoverStyle. There should probably be just a single method that applies the styles.

Now that I have more details, I think we should primarily fix this in the candlestick chart. However, it would be nice to fix the code duplication here that I mentioned. Would you be able to update this PR to create a method to apply the default styles to the rectangle model that's used by both updateElement and removeHoverStyle? We could then possibly override just that method in the candlestick chart instead of both updateElement and removeHoverStyle

[loicbourgois]

I made changes to updateElement and removeHoverStyle to use a common function to update rectangle.

If a subclass has different default options, the new options can be used by either overriding getElementOptions or calling the superclass updateRectangle method and providing the new options as a parameter.

Example for Chart.Financial.js : loicbourgois/chartjs-chart-financial@b5eb159

[benmccann]

I was only suggesting just to create a method to update the styles. Not the entire rectangle. Setting rectangle._xScale, etc. you shouldn't need to do in removeHoverStyle

[loicbourgois]

So just rectangle._model ?

[benmccann]

Just the last few lines that edit the styles starting with borderSkipped. E.g. you don't need to update the label.

[benmccann]

How about calling this method setDefaultStyles or something that indicates how the rectangle is being updated

Neither caller of this function passes in the second parameter...

[loicbourgois]

I agree updateRectangle is not the best, but I would expect setDefaultStyles to set all default styles. Is this the case here ? If not, it's a bit misleading.

As for the second parameter, it can be used by a subclass to supply different default options without having to reimplement the complete method.
Like this : loicbourgois/chartjs-chart-financial@b5eb159
It's just an idea, but maybe that's not the best way to do it.
Also, when trying to set a default value like (rectangle, options = null), the build fails.

[simonbrunel]

Scriptable options are coming and will hopefully give more power on configuring our charts (I would love to see these hover* options deprecated in favor of context.active). The way to build options will change a lot and these new public methods updateRectangle, setDefaultStyles, getElementOptions will certainly be removed. I'm always very careful when modifying the public API, especially when it's to workaround an internal issue.

I didn't test the following solution, but what about simply store the "previous" state:

setHoverStyle: function(element) {
    var dataset = this.chart.data.datasets[element._datasetIndex];
    var index = element._index;
    var custom = element.custom || {};
    var model = element._model;

    element.$previousStyle = {
        backgroundColor: model.backgroundColor,
        borderColor: model.borderColor,
        borderWidth: model.borderWidth
    };

    model.backgroundColor = custom.hoverBackgroundColor ? custom.hoverBackgroundColor : helpers.valueAtIndexOrDefault(dataset.hoverBackgroundColor, index, helpers.getHoverColor(model.backgroundColor));
    model.borderColor = custom.hoverBorderColor ? custom.hoverBorderColor : helpers.valueAtIndexOrDefault(dataset.hoverBorderColor, index, helpers.getHoverColor(model.borderColor));
    model.borderWidth = custom.hoverBorderWidth ? custom.hoverBorderWidth : helpers.valueAtIndexOrDefault(dataset.hoverBorderWidth, index, model.borderWidth);
},

removeHoverStyle: function(element) {
    helpers.merge(element._model, element.$previousStyle || {});
    delete element.$previousStyle;
}

Does that solve your issue with the financial chart?

Note that setHoverStyle will be rewritten when scriptable options will be implemented.

[benmccann]

Yes, that would work. What do you think about changing to the solution that Simon suggested @loicbourgois ?

[loicbourgois]

@benmccann you mentioned a bug with borderSkipped
Should it be included in previousStyle even though it's not modified in setHoverStyle ?

[simonbrunel]

$previousStyle (prefixed by $ which mean attached private properties - expando), should only contain properties modified by setHoverStyle.

[benmccann]

I'm fine with leaving out borderSkipped. I was wondering if maybe we should include it because it's set in updateElement and someone could override setHoverStyle to change the value, but I'm fine either way. I think it's also be fine to say that if someone is going to override setHoverStyle they need to override removeHoverStyle as well

[simonbrunel]

I agree, if someone overrides setHoverStyle, it should also override removeHoverStyle. I think it's easy enough to call the overridden method and avoid duplicating the whole original methods to add a single property. Anyway, I don't think hover interactions should be handled that way, it's not flexible and duplicates code, so I would prefer not to add more logic in it.

[loicbourgois]

New jsfiddle
Issue is fixed with Financial.Chart.js
@simonbrunel your solution failed on 1 test Chart.controllers.bar should remove a hover style from a bar so I mixed it with the original implementation.

[simonbrunel]

It may be better to fix the failing unit test instead.

For each use case (defaults, array options and custom style), it should:

• setup chart (or dataset or custom) options and call chart.update()
• check that bar model have initial values
• call setHoverStyle on bar and check that values have changed
• call removeHoverStyle on bar and check that it matches the original values

[benmccann]

do you need to check if $previousStyle is defined here? it would seem to me that there's no way for it to be undefined

[benmccann]

this line also seems like it would be unnecessary. It doesn't hurt to leave it I suppose, but I'm not sure it ever has any effect

[simonbrunel]

This seems like a bug everywhere as soon as you will need to override other controllers? I think I prefer to keep track of these changes in the same PR, more consistent and easier to track later. Better to release 2.7.2 with the full fix anyway.

[etimberg]

If this goes into 2.7.2 I think it needs to be in a single PR. If we want to defer until after 2.7.2, then I'm more open to multiple PRs.

[loicbourgois]

I'll keep working in this PR then.

[loicbourgois]

I've been able to improve some controllers for setHoverStyle() and removeHoverStyle(), but I'm still struggling with doughnut and polarArea.

With doughnut, only backgroundColor, borderColor, borderWidth are affected, just like for the bar controller.
However, when using the default implementation of removeHoverStyle() from the super class (like done with bar controller), tests fail, and i can't figure why. I added call to update().

• Travis log when using custom removeHoverStyle()
• Block affected by commit 9cd0686 to make the change
• Travis log when using superclass removeHoverStyle()

edit : inverted first and third link.

[loicbourgois]

@benmccann indeed, my bad. I inverted the 2 Travis logs. Fixed previous comment.

[benmccann]

@loicbourgois I'd love to see this PR get finished if you have some more time to spend on it!

[benmccann]

@loicbourgois will you be able to finish up this PR?

[benmccann]

@loicbourgois any chance you'll be able to finish up this PR? it may be closed as inactive if you're not going to come back to it

[benmccann]

Closing as inactive. Please feel free to update and reopen