Skip to content

Commit

Permalink
Fix cleaning up metasets (#9656)
Browse files Browse the repository at this point in the history
* Fix cleaning up metasets

I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced.

This is a minimal fix for #9653; as discussed there, it may also be worth updating `updateHoverStyle`.

As of #7030, `this._metasets` should always be defined, so checking whether it's undefined is no longer necessary.

* Add a test covering metaset behavior

* Add a regression test for #9653; fix `toHaveSize` usage

* Fix test failure
  • Loading branch information
joshkel committed Oct 23, 2021
1 parent 1199415 commit aac0bef
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 6 deletions.
7 changes: 3 additions & 4 deletions src/core/core.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -799,12 +799,11 @@ class Chart {
* @private
*/
_destroyDatasetMeta(datasetIndex) {
const meta = this._metasets && this._metasets[datasetIndex];

const meta = this._metasets[datasetIndex];
if (meta && meta.controller) {
meta.controller._destroy();
delete this._metasets[datasetIndex];
}
delete this._metasets[datasetIndex];
}

_stop() {
Expand Down Expand Up @@ -916,7 +915,7 @@ class Chart {

_remove('resize', listener);

// Stop animating and remove metasets, so when re-attached, the animations start from begining.
// Stop animating and remove metasets, so when re-attached, the animations start from beginning.
this._stop();
this._resize(0, 0);

Expand Down
24 changes: 22 additions & 2 deletions test/specs/core.controller.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -1121,7 +1121,7 @@ describe('Chart', function() {
});
});

it('should resize the canvas if attached to the DOM after construction with mutiple parents', function(done) {
it('should resize the canvas if attached to the DOM after construction with multiple parents', function(done) {
var canvas = document.createElement('canvas');
var wrapper = document.createElement('div');
var wrapper2 = document.createElement('div');
Expand Down Expand Up @@ -1848,7 +1848,7 @@ describe('Chart', function() {
expect(metasets[2].order).toEqual(3);
expect(metasets[3].order).toEqual(4);
});
it('should be moved when datasets are removed from begining', function() {
it('should be moved when datasets are removed from beginning', function() {
this.chart.data.datasets.splice(0, 2);
this.chart.update();
const metasets = this.chart._metasets;
Expand Down Expand Up @@ -1910,6 +1910,26 @@ describe('Chart', function() {
});
});

describe('_destroyDatasetMeta', function() {
beforeEach(function() {
this.chart = acquireChart({
type: 'line',
data: {
datasets: [
{label: '1', order: 2},
{label: '2', order: 1},
{label: '3', order: 4},
{label: '4', order: 3},
]
}
});
});
it('cleans up metasets when the chart is destroyed', function() {
this.chart.destroy();
expect(this.chart._metasets).toEqual([undefined, undefined, undefined, undefined]);
});
});

describe('data visibility', function() {
it('should hide a dataset', function() {
var chart = acquireChart({
Expand Down
42 changes: 42 additions & 0 deletions test/specs/plugin.tooltip.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -1555,6 +1555,48 @@ describe('Plugin.Tooltip', function() {
chart.tooltip.setActiveElements([{datasetIndex: 0, index: 0}], {x: 0, y: 0});
expect(chart.tooltip.getActiveElements()[0].element).toBe(meta.data[0]);
});

it('should update active elements when datasets are removed and added', async function() {
var dataset = {
label: 'Dataset 1',
data: [10, 20, 30],
pointHoverBorderColor: 'rgb(255, 0, 0)',
pointHoverBackgroundColor: 'rgb(0, 255, 0)'
};
var chart = window.acquireChart({
type: 'line',
data: {
datasets: [dataset],
labels: ['Point 1', 'Point 2', 'Point 3']
},
options: {
plugins: {
tooltip: {
mode: 'nearest',
intersect: true
}
}
}
});

var meta = chart.getDatasetMeta(0);
var point = meta.data[1];
var expectedPoint = jasmine.objectContaining({datasetIndex: 0, index: 1});

await jasmine.triggerMouseEvent(chart, 'mousemove', point);

expect(chart.tooltip.getActiveElements()).toEqual([expectedPoint]);

chart.data.datasets = [];
chart.update();

expect(chart.tooltip.getActiveElements()).toEqual([]);

chart.data.datasets = [dataset];
chart.update();

expect(chart.tooltip.getActiveElements()).toEqual([expectedPoint]);
});
});

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

0 comments on commit aac0bef

Please sign in to comment.