fix(api): Ensure svg nodes to be removed from memory

Instead of resetting by innerHTML, remove nodes to make sure nodes to be removed from DOM tree. Also, add unbind all attached events. Fix #2489
naver · Jan 13, 2022 · f49ed83 · f49ed83
1 parent 92fb033
commit f49ed83
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 7 deletions.
diff --git a/src/Chart/api/chart.ts b/src/Chart/api/chart.ts
@@ -106,12 +106,18 @@ export default {
 			$$.callPluginHook("$willDestroy");
 			$$.charts.splice($$.charts.indexOf(this), 1);
 
+			// detach events
+			$$.unbindAllEvents();
+
 			// clear timers && pending transition
 			svg.select("*").interrupt();
 			$$.resizeFunction.clear();
 
 			window.removeEventListener("resize", $$.resizeFunction);
-			chart.classed("bb", false).html("");
+			chart.classed("bb", false)
+				.style("position", null)
+				.selectChildren()
+				.remove();
 
 			// releasing own references
 			Object.keys(this).forEach(key => {

diff --git a/src/ChartInternal/ChartInternal.ts b/src/ChartInternal/ChartInternal.ts
@@ -210,7 +210,9 @@ export default class ChartInternal {
 			$el.chart = d3Select(document.body.appendChild(document.createElement("div")));
 		}
 
-		$el.chart.html("").classed(bindto.classname, true);
+		$el.chart.html("")
+			.classed(bindto.classname, true)
+			.style("position", "relative");
 
 		$$.initToRender();
 	}

diff --git a/src/ChartInternal/interactions/interaction.ts b/src/ChartInternal/interactions/interaction.ts
@@ -254,10 +254,42 @@ export default {
 		const $$ = this;
 		const {$el: {eventRect, zoomResetBtn}} = $$;
 
-		eventRect
-			.on(".zoom", null)
-			.on(".drag", null);
+		eventRect?.on(".zoom wheel.zoom .drag", null);
 
-		zoomResetBtn?.style("display", "none");
+		zoomResetBtn?.on("click", null)
+			.style("display", "none");
+	},
+
+	/**
+	 * Unbind all attached events
+	 * @private
+	 */
+	unbindAllEvents():	void {
+		const $$ = this;
+		const {$el: {arcs, eventRect, legend, region, svg}, brush} = $$;
+		const list = [
+			"wheel",
+			"click",
+			"mouseover",
+			"mousemove",
+			"mouseout",
+			"touchstart",
+			"touchmove",
+			"touchend",
+			"touchstart.eventRect",
+			"touchmove.eventRect",
+			"touchend.eventRect",
+			".brush",
+			".drag",
+			".zoom",
+			"wheel.zoom",
+			"dblclick.zoom"
+		].join(" ");
+
+		// detach all possible event types
+		[svg, eventRect, region?.list, brush?.getSelection(), arcs?.selectAll("path"), legend?.selectAll("g")]
+			.forEach(v => v?.on(list, null));
+
+		$$.unbindZoomEvent?.();
 	}
 };
diff --git a/src/ChartInternal/internals/tooltip.ts b/src/ChartInternal/internals/tooltip.ts
@@ -21,7 +21,6 @@ export default {
 
 		if ($el.tooltip.empty()) {
 			$el.tooltip = $el.chart
-				.style("position", "relative")
 				.append("div")
 				.attr("class", CLASS.tooltipContainer)
 				.style("position", "absolute")

diff --git a/test/api/chart-spec.ts b/test/api/chart-spec.ts
@@ -115,6 +115,12 @@ describe("API chart", () => {
 			});
 
 			expect(bb.instance.indexOf(chart) === -1).to.be.true;
+
+			const el = document.getElementById("chart");
+
+			// should revert removing className and styles
+			expect(el.classList.contains("bb")).to.be.false;
+			expect(el.style.position).to.be.equal("");
 		});
 
 		it("should be destroyed without throwing error", done => {
@@ -139,6 +145,20 @@ describe("API chart", () => {
 			chart.destroy();
 			chart.destroy();
 		});
+
+		it("events should be unbound on destroy", () => {
+			const {internal} = chart;
+			const {$el: {eventRect}} = internal;
+
+			// all bound events are removed
+			chart.internal.unbindAllEvents();
+
+			["mouseover", "mousemove", "mouseout"].forEach(event => {
+				expect(eventRect.on(event)).to.be.undefined;
+			});
+
+			chart.destroy();
+		});
 	});
 
 	describe("config()", () => {