From: Simon Brunel <simonbrunel@users.noreply.github.com>
Date: Sat, 24 Sep 2016 19:51:12 +0000 (+0200)
Subject: Inject iframe for responsive charts only
X-Git-Tag: v2.4.0~1^2~40
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=806a3832ef3f169547edb913130b8c394104e356;p=thirdparty%2FChart.js.git

Inject iframe for responsive charts only

Responsiveness is currently based on the use of an iframe, however this method causes performance issues and could be troublesome when used with ad blockers. So make sure that the user is still able to create a chart without iframe when responsive is false.
---

diff --git a/src/core/core.controller.js b/src/core/core.controller.js
index 1d169adea..e335c3a3a 100644
--- a/src/core/core.controller.js
+++ b/src/core/core.controller.js
@@ -151,8 +151,11 @@ module.exports = function(Chart) {
 
 		me.id = helpers.uid();
 		me.chart = instance;
-		me.config = instance.config;
-		me.options = me.config.options;
+		me.config = config;
+		me.options = config.options;
+
+		// Add the chart instance to the global namespace
+		Chart.instances[me.id] = me;
 
 		Object.defineProperty(me, 'data', {
 			get: function() {
@@ -160,18 +163,16 @@ module.exports = function(Chart) {
 			}
 		});
 
-		// Always bind this so that if the responsive state changes we still work
-		helpers.addResizeListener(canvas.parentNode, function() {
-			if (me.config.options.responsive) {
+		// Responsiveness is currently based on the use of an iframe, however this method causes
+		// performance issues and could be troublesome when used with ad blockers. So make sure
+		// that the user is still able to create a chart without iframe when responsive is false.
+		// See https://github.com/chartjs/Chart.js/issues/2210
+		if (me.options.responsive) {
+			helpers.addResizeListener(canvas.parentNode, function() {
 				me.resize();
-			}
-		});
-
-		// Add the chart instance to the global namespace
-		Chart.instances[me.id] = me;
+			});
 
-		if (me.options.responsive) {
-			// Silent resize before chart draws
+			// Initial resize before chart draws (must be silent to preserve initial animations).
 			me.resize(true);
 		}
 
@@ -684,11 +685,11 @@ module.exports = function(Chart) {
 			me.stop();
 			me.clear();
 
-			helpers.unbindEvents(me, me.events);
-
 			if (canvas) {
+				helpers.unbindEvents(me, me.events);
 				helpers.removeResizeListener(canvas.parentNode);
 				releaseCanvas(canvas);
+				me.chart.canvas = null;
 			}
 
 			// if we scaled the canvas in response to a devicePixelRatio !== 1, we need to undo that transform here
diff --git a/src/core/core.helpers.js b/src/core/core.helpers.js
index e404b61b1..6d63284a7 100644
--- a/src/core/core.helpers.js
+++ b/src/core/core.helpers.js
@@ -980,15 +980,24 @@ module.exports = function(Chart) {
 		// Insert the iframe so that contentWindow is available
 		node.insertBefore(iframe, node.firstChild);
 
+		// Let's keep track of this added iframe and thus avoid DOM query when removing it.
+		node._chartjs = {
+			resizer: iframe
+		};
+
 		this.addEvent(iframe.contentWindow || iframe, 'resize', callback);
 	};
 	helpers.removeResizeListener = function(node) {
-		var hiddenIframe = node.querySelector('.chartjs-hidden-iframe');
+		if (!node || !node._chartjs) {
+			return;
+		}
 
-		// Remove the resize detect iframe
-		if (hiddenIframe) {
-			hiddenIframe.parentNode.removeChild(hiddenIframe);
+		var iframe = node._chartjs.resizer;
+		if (iframe) {
+			iframe.parentNode.removeChild(iframe);
 		}
+
+		delete node._chartjs;
 	};
 	helpers.isArray = Array.isArray?
 		function(obj) {
diff --git a/test/core.controller.tests.js b/test/core.controller.tests.js
index 83eba3baf..37661b707 100644
--- a/test/core.controller.tests.js
+++ b/test/core.controller.tests.js
@@ -149,6 +149,18 @@ describe('Chart.Controller', function() {
 				rw: 165, rh: 85,
 			});
 		});
+
+		it('should NOT inject the resizer element', function() {
+			var chart = acquireChart({
+				options: {
+					responsive: false
+				}
+			});
+
+			var wrapper = chart.chart.canvas.parentNode;
+			expect(wrapper.childNodes.length).toBe(1);
+			expect(wrapper.firstChild.tagName).toBe('CANVAS');
+		});
 	});
 
 	describe('config.options.responsive: true (maintainAspectRatio: false)', function() {
@@ -449,7 +461,6 @@ describe('Chart.Controller', function() {
 	describe('controller.destroy', function() {
 		it('should restore canvas (and context) initial values', function(done) {
 			var chart = acquireChart({
-				type: 'line',
 				options: {
 					responsive: true,
 					maintainAspectRatio: false
@@ -484,5 +495,24 @@ describe('Chart.Controller', function() {
 				done();
 			});
 		});
+
+		it('should remove the resizer element when responsive: true', function() {
+			var chart = acquireChart({
+				options: {
+					responsive: true
+				}
+			});
+
+			var wrapper = chart.chart.canvas.parentNode;
+			var resizer = wrapper.firstChild;
+
+			expect(wrapper.childNodes.length).toBe(2);
+			expect(resizer.tagName).toBe('IFRAME');
+
+			chart.destroy();
+
+			expect(wrapper.childNodes.length).toBe(1);
+			expect(wrapper.firstChild.tagName).toBe('CANVAS');
+		});
 	});
 });
diff --git a/test/mockContext.js b/test/mockContext.js
index 48e5ce304..83b73e315 100644
--- a/test/mockContext.js
+++ b/test/mockContext.js
@@ -253,23 +253,23 @@
 		window.document.body.appendChild(wrapper);
 
 		chart = new Chart(canvas.getContext("2d"), config);
-		chart.__test_persistent = options.persistent;
+		chart._test_persistent = options.persistent;
+		chart._test_wrapper = wrapper;
 		charts[chart.id] = chart;
 		return chart;
 	}
 
 	function releaseChart(chart) {
 		chart.destroy();
-		chart.chart.canvas.parentNode.remove();
+		chart._test_wrapper.remove();
 		delete charts[chart.id];
-		delete chart;
 	}
 
 	afterEach(function() {
 		// Auto releasing acquired charts
 		for (var id in charts) {
 			var chart = charts[id];
-			if (!chart.__test_persistent) {
+			if (!chart._test_persistent) {
 				releaseChart(chart);
 			}
 		}