Skip to content

Commit

Permalink
[FIX] ChartJs: Properly destroy chartJs object on component wrapper d…
Browse files Browse the repository at this point in the history
…eletion

How to reproduce:

- Make a chartjs chart (line/bar/pie)
- Mousedown on a datapoint/bar/part of the pie
- move your mouse
- mouseup (lift your finger) while still moving your mouse
-> crash

We were not properly destroying the chart js item and their linked
eventListeners persisted. Specifically, the eventHandler of the tooltip
plugin would still try to handle the mousemove event while its internal
state was partially invalidated.

closes #4137

Task: 3777754
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
  • Loading branch information
rrahir authored and LucasLefevre committed May 3, 2024
1 parent 0cfca7e commit 1a0254b
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 2 deletions.
3 changes: 2 additions & 1 deletion src/components/figures/chart/chartJs/chartjs.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Component, onMounted, useEffect, useRef } from "@odoo/owl";
import { Component, onMounted, onWillUnmount, useEffect, useRef } from "@odoo/owl";
import type { Chart, ChartConfiguration } from "chart.js";
import { deepCopy, deepEquals } from "../../../../helpers";
import { Figure, SpreadsheetChildEnv } from "../../../../types";
Expand Down Expand Up @@ -42,6 +42,7 @@ export class ChartJsComponent extends Component<Props, SpreadsheetChildEnv> {
// Note: chartJS modify the runtime in place, so it's important to give it a copy
this.createChart(deepCopy(runtime.chartJsConfig));
});
onWillUnmount(() => this.chart?.destroy());
useEffect(() => {
const runtime = this.chartRuntime;
if (!deepEquals(runtime, this.currentRuntime, "ignoreFunctions")) {
Expand Down
10 changes: 10 additions & 0 deletions tests/figures/chart/charts_component.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1224,3 +1224,13 @@ describe("Default background on runtime tests", () => {
expect(model.getters.getChartRuntime("1").background).toBe("#FA0000");
});
});

test("ChartJS charts are correctly destroyed on chart deletion", async () => {
({ parent, fixture, model } = await mountSpreadsheet({ model: new Model() }));
createChart(model, { dataSets: ["A1"], type: "bar" }, "1");
await nextTick();
const spyDelete = jest.spyOn((window as any).Chart.prototype, "destroy");
model.dispatch("DELETE_FIGURE", { id: "1", sheetId: model.getters.getActiveSheetId() });
await nextTick();
expect(spyDelete).toHaveBeenCalled();
});
2 changes: 1 addition & 1 deletion tests/test_helpers/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ export const mockChart = () => {
return mockChartData.data;
}
toBase64Image = () => "";
destroy = () => {};
destroy() {}
update() {}
options = mockChartData.options;
config = mockChartData;
Expand Down

0 comments on commit 1a0254b

Please sign in to comment.