Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance context acquisition on chart creation #3474

Merged
merged 1 commit into from
Oct 16, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@

.DS_Store
.idea
.vscode
bower.json
1 change: 1 addition & 0 deletions docs/00-Getting-Started.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ To create a chart, we need to instantiate the `Chart` class. To do this, we need
var ctx = document.getElementById("myChart");
var ctx = document.getElementById("myChart").getContext("2d");
var ctx = $("#myChart");
var ctx = "myChart";
```

Once you have the element or context, you're ready to instantiate a pre-defined chart-type or create your own!
Expand Down
61 changes: 52 additions & 9 deletions src/core/core.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,36 @@ module.exports = function(Chart) {
delete canvas._chartjs;
}

/**
* TODO(SB) Move this method in the upcoming core.platform class.
*/
function acquireContext(item, config) {
if (typeof item === 'string') {
item = document.getElementById(item);
} else if (item.length) {
// Support for array based queries (such as jQuery)
item = item[0];
}

if (item && item.canvas) {
// Support for any object associated to a canvas (including a context2d)
item = item.canvas;
}

if (item instanceof HTMLCanvasElement) {
// To prevent canvas fingerprinting, some add-ons undefine the getContext
// method, for example: https://github.com/kkapsner/CanvasBlocker
// https://github.com/chartjs/Chart.js/issues/2807
var context = item.getContext && item.getContext('2d');
if (context instanceof CanvasRenderingContext2D) {
initCanvas(item, config);
return context;
}
}

return null;
}

/**
* Initializes the given config with global and chart default values.
*/
Expand All @@ -136,21 +166,22 @@ module.exports = function(Chart) {
* @class Chart.Controller
* The main controller of a chart.
*/
Chart.Controller = function(context, config, instance) {
Chart.Controller = function(item, config, instance) {
var me = this;
var canvas;

config = initConfig(config);
canvas = initCanvas(context.canvas, config);

var context = acquireContext(item, config);
var canvas = context && context.canvas;
var height = canvas && canvas.height;
var width = canvas && canvas.width;

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);
instance.width = width;
instance.height = height;
instance.aspectRatio = height? width / height : null;

me.id = helpers.uid();
me.chart = instance;
Expand All @@ -167,6 +198,17 @@ module.exports = function(Chart) {
}
});

if (!context || !canvas) {
// The given item is not a compatible context2d element, let's return before finalizing
// the chart initialization but after setting basic chart / controller properties that
// can help to figure out that the chart is not valid (e.g chart.canvas !== null);
// https://github.com/chartjs/Chart.js/issues/2807
console.error("Failed to create chart: can't acquire context from the given item");
return me;
}

helpers.retinaScale(instance);

// 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.
Expand Down Expand Up @@ -593,7 +635,6 @@ module.exports = function(Chart) {
var meta, i, ilen;

me.stop();
me.clear();

// dataset controllers need to cleanup associated data
for (i = 0, ilen = me.data.datasets.length; i < ilen; ++i) {
Expand All @@ -607,8 +648,10 @@ module.exports = function(Chart) {
if (canvas) {
helpers.unbindEvents(me, me.events);
helpers.removeResizeListener(canvas.parentNode);
helpers.clear(me.chart);
releaseCanvas(canvas);
me.chart.canvas = null;
me.chart.ctx = null;
}

// if we scaled the canvas in response to a devicePixelRatio !== 1, we need to undo that transform here
Expand Down
19 changes: 3 additions & 16 deletions src/core/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,9 @@
module.exports = function() {

// Occupy the global variable of Chart, and create a simple base class
var Chart = function(context, config) {
var me = this;

// Support a jQuery'd canvas element
if (context.length && context[0].getContext) {
context = context[0];
}

// Support a canvas domnode
if (context.getContext) {
context = context.getContext('2d');
}

me.controller = new Chart.Controller(context, config, me);

return me.controller;
var Chart = function(item, config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if Chart.Controller can eventually just become the Chart variable. #2481 asked about this and now that everything is in the controller, we might be able to do it.

Only issue I see is with any dependencies that the controller needs but we need the Chart variable to be defined first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, Chart.Controller could become Chart (maybe Chart.DatasetController could also simply be Chart.Controller). I can investigate how to do that change and keep backward compatibility but that would be for another PR :)

this.controller = new Chart.Controller(item, config, this);
return this.controller;
};

// Globally expose the defaults to allow for user updating/changing
Expand Down
70 changes: 69 additions & 1 deletion test/core.controller.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,75 @@ describe('Chart.Controller', function() {
setTimeout(callback, 100);
}

describe('config', function() {
describe('context acquisition', function() {
var canvasId = 'chartjs-canvas';

beforeEach(function() {
var canvas = document.createElement('canvas');
canvas.setAttribute('id', canvasId);
window.document.body.appendChild(canvas);
});

afterEach(function() {
document.getElementById(canvasId).remove();
});

// see https://github.com/chartjs/Chart.js/issues/2807
it('should gracefully handle invalid item', function() {
var chart = new Chart('foobar');

expect(chart).not.toBeValidChart();

chart.destroy();
});

it('should accept a DOM element id', function() {
var canvas = document.getElementById(canvasId);
var chart = new Chart(canvasId);

expect(chart).toBeValidChart();
expect(chart.chart.canvas).toBe(canvas);
expect(chart.chart.ctx).toBe(canvas.getContext('2d'));

chart.destroy();
});

it('should accept a canvas element', function() {
var canvas = document.getElementById(canvasId);
var chart = new Chart(canvas);

expect(chart).toBeValidChart();
expect(chart.chart.canvas).toBe(canvas);
expect(chart.chart.ctx).toBe(canvas.getContext('2d'));

chart.destroy();
});

it('should accept a canvas context2D', function() {
var canvas = document.getElementById(canvasId);
var context = canvas.getContext('2d');
var chart = new Chart(context);

expect(chart).toBeValidChart();
expect(chart.chart.canvas).toBe(canvas);
expect(chart.chart.ctx).toBe(context);

chart.destroy();
});

it('should accept an array containing canvas', function() {
var canvas = document.getElementById(canvasId);
var chart = new Chart([canvas]);

expect(chart).toBeValidChart();
expect(chart.chart.canvas).toBe(canvas);
expect(chart.chart.ctx).toBe(canvas.getContext('2d'));

chart.destroy();
});
});

describe('config initialization', function() {
it('should create missing config.data properties', function() {
var chart = acquireChart({});
var data = chart.data;
Expand Down
90 changes: 59 additions & 31 deletions test/mockContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,54 +158,82 @@
};
}

function toBeValidChart() {
return {
compare: function(actual) {
var chart = actual && actual.chart;
var message = null;

if (!(actual instanceof Chart.Controller)) {
message = 'Expected ' + actual + ' to be an instance of Chart.Controller';
} else if (!(chart instanceof Chart)) {
message = 'Expected chart to be an instance of Chart';
} else if (!(chart.canvas instanceof HTMLCanvasElement)) {
message = 'Expected canvas to be an instance of HTMLCanvasElement';
} else if (!(chart.ctx instanceof CanvasRenderingContext2D)) {
message = 'Expected context to be an instance of CanvasRenderingContext2D';
} else if (typeof chart.height !== 'number' || !isFinite(chart.height)) {
message = 'Expected height to be a strict finite number';
} else if (typeof chart.width !== 'number' || !isFinite(chart.width)) {
message = 'Expected width to be a strict finite number';
}

return {
message: message? message : 'Expected ' + actual + ' to be valid chart',
pass: !message
};
}
};
}

function toBeChartOfSize() {
return {
compare: function(actual, expected) {
var res = toBeValidChart().compare(actual);
if (!res.pass) {
return res;
}

var message = null;
var chart, canvas, style, dh, dw, rh, rw;

if (!actual || !(actual instanceof Chart.Controller)) {
message = 'Expected ' + actual + ' to be an instance of Chart.Controller.';
} else {
chart = actual.chart;
canvas = chart.ctx.canvas;
style = getComputedStyle(canvas);
dh = parseInt(style.height);
dw = parseInt(style.width);
rh = canvas.height;
rw = canvas.width;

// sanity checks
if (chart.height !== rh) {
message = 'Expected chart height ' + chart.height + ' to be equal to render height ' + rh;
} else if (chart.width !== rw) {
message = 'Expected chart width ' + chart.width + ' to be equal to render width ' + rw;
}
var chart = actual.chart;
var canvas = chart.ctx.canvas;
var style = getComputedStyle(canvas);
var dh = parseInt(style.height, 10);
var dw = parseInt(style.width, 10);
var rh = canvas.height;
var rw = canvas.width;

// sanity checks
if (chart.height !== rh) {
message = 'Expected chart height ' + chart.height + ' to be equal to render height ' + rh;
} else if (chart.width !== rw) {
message = 'Expected chart width ' + chart.width + ' to be equal to render width ' + rw;
}

// validity checks
if (dh !== expected.dh) {
message = 'Expected display height ' + dh + ' to be equal to ' + expected.dh;
} else if (dw !== expected.dw) {
message = 'Expected display width ' + dw + ' to be equal to ' + expected.dw;
} else if (rh !== expected.rh) {
message = 'Expected render height ' + rh + ' to be equal to ' + expected.rh;
} else if (rw !== expected.rw) {
message = 'Expected render width ' + rw + ' to be equal to ' + expected.rw;
}
// validity checks
if (dh !== expected.dh) {
message = 'Expected display height ' + dh + ' to be equal to ' + expected.dh;
} else if (dw !== expected.dw) {
message = 'Expected display width ' + dw + ' to be equal to ' + expected.dw;
} else if (rh !== expected.rh) {
message = 'Expected render height ' + rh + ' to be equal to ' + expected.rh;
} else if (rw !== expected.rw) {
message = 'Expected render width ' + rw + ' to be equal to ' + expected.rw;
}

return {
message: message? message : 'Expected ' + actual + ' to be a chart of size ' + expected,
pass: !message
}
};
}
}
};
}

beforeEach(function() {
jasmine.addMatchers({
toBeCloseToPixel: toBeCloseToPixel,
toEqualOneOf: toEqualOneOf,
toBeValidChart: toBeValidChart,
toBeChartOfSize: toBeChartOfSize
});
});
Expand Down