Fix initial aspect ratio when not responsive

When responsive is false and no canvas height explicitly set, the aspectRatio option wasn't applied because of the canvas default height. Prevent the retinaScale method to change the canvas display size since this method is called for none responsive charts, but instead make the resize() responsible of these changes. Also, as discussed some time ago, moved most of the core.js logic into core.controller.js. Clean up the destroy process and make sure that initial canvas values are properly saved and restored.

src/core/core.controller.js

@@ -3,6 +3,7 @@
 module.exports = function(Chart) {
 
 	var helpers = Chart.helpers;
+
 	// Create a dictionary of chart types, to allow for extension of existing types
 	Chart.types = {};
 
@@ -13,40 +14,176 @@ module.exports = function(Chart) {
 	// Controllers available for dataset visualization eg. bar, line, slice, etc.
 	Chart.controllers = {};
 
+	/**
+	 * The "used" size is the final value of a dimension property after all calculations have
+	 * been performed. This method uses the computed style of `element` but returns undefined
+	 * if the computed style is not expressed in pixels. That can happen in some cases where
+	 * `element` has a size relative to its parent and this last one is not yet displayed,
+	 * for example because of `display: none` on a parent node.
+	 * TODO(SB) Move this method in the upcoming core.platform class.
+	 * @see https://developer.mozilla.org/en-US/docs/Web/CSS/used_value
+	 * @returns {Number} Size in pixels or undefined if unknown.
+	 */
+	function readUsedSize(element, property) {
+		var value = helpers.getStyle(element, property);
+		var matches = value && value.match(/(\d+)px/);
+		return matches? Number(matches[1]) : undefined;
+	}
+
+	/**
+	 * Initializes the canvas style and render size without modifying the canvas display size,
+	 * since responsiveness is handled by the controller.resize() method. The config is used
+	 * to determine the aspect ratio to apply in case no explicit height has been specified.
+	 * TODO(SB) Move this method in the upcoming core.platform class.
+	 */
+	function initCanvas(canvas, config) {
+		var style = canvas.style;
+
+		// NOTE(SB) canvas.getAttribute('width') !== canvas.width: in the first case it
+		// returns null or '' if no explicit value has been set to the canvas attribute.
+		var renderHeight = canvas.getAttribute('height');
+		var renderWidth = canvas.getAttribute('width');
+
+		// Chart.js modifies some canvas values that we want to restore on destroy
+		canvas._chartjs = {
+			initial: {
+				height: renderHeight,
+				width: renderWidth,
+				style: {
+					display: style.display,
+					height: style.height,
+					width: style.width
+				}
+			}
+		};
+
+		// Force canvas to display as block to avoid extra space caused by inline
+		// elements, which would interfere with the responsive resize process.
+		// https://github.com/chartjs/Chart.js/issues/2538
+		style.display = style.display || 'block';
+
+		if (renderWidth === null || renderWidth === '') {
+			var displayWidth = readUsedSize(canvas, 'width');
+			if (displayWidth !== undefined) {
+				canvas.width = displayWidth;
+			}
+		}
+
+		if (renderHeight === null || renderHeight === '') {
+			if (canvas.style.height === '') {
+				// If no explicit render height and style height, let's apply the aspect ratio,
+				// which one can be specified by the user but also by charts as default option
+				// (i.e. options.aspectRatio). If not specified, use canvas aspect ratio of 2.
+				canvas.height = canvas.width / (config.options.aspectRatio || 2);
+			} else {
+				var displayHeight = readUsedSize(canvas, 'height');
+				if (displayWidth !== undefined) {
+					canvas.height = displayHeight;
+				}
+			}
+		}
+
+		return canvas;
+	}
+
+	/**
+	 * Restores the canvas initial state, such as render/display sizes and style.
+	 * TODO(SB) Move this method in the upcoming core.platform class.
+	 */
+	function releaseCanvas(canvas) {
+		if (!canvas._chartjs) {
+			return;
+		}
+
+		var initial = canvas._chartjs.initial;
+		['height', 'width'].forEach(function(prop) {
+			var value = initial[prop];
+			if (value === undefined || value === null) {
+				canvas.removeAttribute(prop);
+			} else {
+				canvas.setAttribute(prop, value);
+			}
+		});
+
+		helpers.each(initial.style || {}, function(value, key) {
+			canvas.style[key] = value;
+		});
+
+		delete canvas._chartjs;
+	}
+
+	/**
+	 * Initializes the given config with global and chart default values.
+	 */
+	function initConfig(config) {
+		config = config || {};
+		return helpers.configMerge({
+			options: helpers.configMerge(
+				Chart.defaults.global,
+				Chart.defaults[config.type],
+				config.options || {}),
+			data: {
+				datasets: [],
+				labels: []
+			}
+		}, config);
+	}
+
 	/**
 	 * @class Chart.Controller
 	 * The main controller of a chart.
 	 */
-	Chart.Controller = function(instance) {
+	Chart.Controller = function(context, config, instance) {
+		var me = this;
+		var canvas;
 
-		this.chart = instance;
-		this.config = instance.config;
-		this.options = this.config.options = helpers.configMerge(Chart.defaults.global, Chart.defaults[this.config.type], this.config.options || {});
-		this.id = helpers.uid();
+		config = initConfig(config);
+		canvas = initCanvas(context.canvas, config);
 
-		Object.defineProperty(this, 'data', {
+		instance.ctx = context;
+		instance.canvas = canvas;
+		instance.config = config;
+		instance.width = canvas.width;
+		instance.height = canvas.height;
+		instance.aspectRatio = canvas.width / canvas.height;
+
+		helpers.retinaScale(instance);
+
+		me.id = helpers.uid();
+		me.chart = instance;
+		me.config = instance.config;
+		me.options = me.config.options;
+
+		Object.defineProperty(me, 'data', {
 			get: function() {
-				return this.config.data;
+				return me.config.data;
+			}
+		});
+
+		// Always bind this so that if the responsive state changes we still work
+		helpers.addResizeListener(canvas.parentNode, function() {
+			if (me.config.options.responsive) {
+				me.resize();
 			}
 		});
 
 		// Add the chart instance to the global namespace
-		Chart.instances[this.id] = this;
+		Chart.instances[me.id] = me;
 
-		if (this.options.responsive) {
+		if (me.options.responsive) {
 			// Silent resize before chart draws
-			this.resize(true);
+			me.resize(true);
 		}
 
-		this.initialize();
+		me.initialize();
 
-		return this;
+		return me;
 	};
 
 	helpers.extend(Chart.Controller.prototype, /** @lends Chart.Controller */ {
-
 		initialize: function() {
 			var me = this;
+
 			// Before init plugin notification
 			Chart.plugins.notify('beforeInit', [me]);
 
@@ -82,15 +219,17 @@ module.exports = function(Chart) {
 		resize: function(silent) {
 			var me = this;
 			var chart = me.chart;
+			var options = me.options;
 			var canvas = chart.canvas;
-			var newWidth = helpers.getMaximumWidth(canvas);
-			var aspectRatio = chart.aspectRatio;
-			var newHeight = (me.options.maintainAspectRatio && isNaN(aspectRatio) === false && isFinite(aspectRatio) && aspectRatio !== 0) ? newWidth / aspectRatio : helpers.getMaximumHeight(canvas);
+			var aspectRatio = (options.maintainAspectRatio && chart.aspectRatio) || null;
 
-			var sizeChanged = chart.width !== newWidth || chart.height !== newHeight;
+			// the canvas render width and height will be casted to integers so make sure that
+			// the canvas display style uses the same integer values to avoid blurring effect.
+			var newWidth = Math.floor(helpers.getMaximumWidth(canvas));
+			var newHeight = Math.floor(aspectRatio? newWidth / aspectRatio : helpers.getMaximumHeight(canvas));
 
-			if (!sizeChanged) {
-				return me;
+			if (chart.width === newWidth && chart.height === newHeight) {
+				return;
 			}
 
 			canvas.width = chart.width = newWidth;
@@ -98,6 +237,9 @@ module.exports = function(Chart) {
 
 			helpers.retinaScale(chart);
 
+			canvas.style.width = newWidth + 'px';
+			canvas.style.height = newHeight + 'px';
+
 			// Notify any plugins about the resize
 			var newSize = {width: newWidth, height: newHeight};
 			Chart.plugins.notify('resize', [me, newSize]);
@@ -111,8 +253,6 @@ module.exports = function(Chart) {
 				me.stop();
 				me.update(me.options.responsiveAnimationDuration);
 			}
-
-			return me;
 		},
 
 		ensureScalesHaveIDs: function() {
@@ -539,25 +679,23 @@ module.exports = function(Chart) {
 
 		destroy: function() {
 			var me = this;
+			var canvas = me.chart.canvas;
+
 			me.stop();
 			me.clear();
-			helpers.unbindEvents(me, me.events);
-			helpers.removeResizeListener(me.chart.canvas.parentNode);
 
-			// Reset canvas height/width attributes
-			var canvas = me.chart.canvas;
-			canvas.width = me.chart.width;
-			canvas.height = me.chart.height;
+			helpers.unbindEvents(me, me.events);
+
+			if (canvas) {
+				helpers.removeResizeListener(canvas.parentNode);
+				releaseCanvas(canvas);
+			}
 
 			// if we scaled the canvas in response to a devicePixelRatio !== 1, we need to undo that transform here
 			if (me.chart.originalDevicePixelRatio !== undefined) {
 				me.chart.ctx.scale(1 / me.chart.originalDevicePixelRatio, 1 / me.chart.originalDevicePixelRatio);
 			}
 
-			// Reset to the old style since it may have been changed by the device pixel ratio changes
-			canvas.style.width = me.chart.originalCanvasStyleWidth;
-			canvas.style.height = me.chart.originalCanvasStyleHeight;
-
 			Chart.plugins.notify('destroy', [me]);
 
 			delete Chart.instances[me.id];

src/core/core.helpers.js

@@ -859,9 +859,6 @@ module.exports = function(Chart) {
 			// when destroy is called
 			chart.originalDevicePixelRatio = chart.originalDevicePixelRatio || pixelRatio;
 		}
-
-		canvas.style.width = width + 'px';
-		canvas.style.height = height + 'px';
 	};
 	// -- Canvas methods
 	helpers.clear = function(chart) {

src/core/core.js

@@ -5,12 +5,6 @@ module.exports = function() {
 	// Occupy the global variable of Chart, and create a simple base class
 	var Chart = function(context, config) {
 		var me = this;
-		var helpers = Chart.helpers;
-		me.config = config || {
-			data: {
-				datasets: []
-			}
-		};
 
 		// Support a jQuery'd canvas element
 		if (context.length && context[0].getContext) {
@@ -22,44 +16,9 @@ module.exports = function() {
 			context = context.getContext('2d');
 		}
 
-		me.ctx = context;
-		me.canvas = context.canvas;
-
-		context.canvas.style.display = context.canvas.style.display || 'block';
-
-		// Figure out what the size of the chart will be.
-		// If the canvas has a specified width and height, we use those else
-		// we look to see if the canvas node has a CSS width and height.
-		// If there is still no height, fill the parent container
-		me.width = context.canvas.width || parseInt(helpers.getStyle(context.canvas, 'width'), 10) || helpers.getMaximumWidth(context.canvas);
-		me.height = context.canvas.height || parseInt(helpers.getStyle(context.canvas, 'height'), 10) || helpers.getMaximumHeight(context.canvas);
-
-		me.aspectRatio = me.width / me.height;
-
-		if (isNaN(me.aspectRatio) || isFinite(me.aspectRatio) === false) {
-			// If the canvas has no size, try and figure out what the aspect ratio will be.
-			// Some charts prefer square canvases (pie, radar, etc). If that is specified, use that
-			// else use the canvas default ratio of 2
-			me.aspectRatio = config.aspectRatio !== undefined ? config.aspectRatio : 2;
-		}
-
-		// Store the original style of the element so we can set it back
-		me.originalCanvasStyleWidth = context.canvas.style.width;
-		me.originalCanvasStyleHeight = context.canvas.style.height;
-
-		// High pixel density displays - multiply the size of the canvas height/width by the device pixel ratio, then scale.
-		helpers.retinaScale(me);
-		me.controller = new Chart.Controller(me);
-
-		// Always bind this so that if the responsive state changes we still work
-		helpers.addResizeListener(context.canvas.parentNode, function() {
-			if (me.controller && me.controller.config.options.responsive) {
-				me.controller.resize();
-			}
-		});
-
-		return me.controller ? me.controller : me;
+		me.controller = new Chart.Controller(context, config, me);
 
+		return me.controller;
 	};
 
 	// Globally expose the defaults to allow for user updating/changing
@@ -106,5 +65,4 @@ module.exports = function() {
 	Chart.Chart = Chart;
 
 	return Chart;
-
 };