Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
davidswinegar committed Jan 17, 2020
1 parent 5335709 commit c005cad
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 25 deletions.
5 changes: 3 additions & 2 deletions docs/getting-started/v3-migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ Chart.js 3.0 introduces a number of breaking changes. Chart.js 2.0 was released
* `scales.[x/y]Axes.time.max` was renamed to `scales[id].max`
* `scales.[x/y]Axes.time.min` was renamed to `scales[id].min`
* The dataset option `tension` was renamed to `lineTension`
* To override the platform class used in a chart instance, pass `platform: PlatformClass` in the config object. Note that the class should be passed, not an instance of the class.
* To disable CSS injection (necessary in some iframe contexts), `Chart.platform.disableCSSInjection = true` is no longer supported. Pass `disableCSSInjection: true` in the config object instead.

### Animations

Expand Down Expand Up @@ -161,6 +163,7 @@ Animation system was completely rewritten in Chart.js v3. Each property can now

* `helpers._alignPixel` was renamed to `helpers.canvas._alignPixel`
* `helpers._decimalPlaces` was renamed to `helpers.math._decimalPlaces`
* `chart.initialize` was renamed to `chart._initialize` (labeled as private but not named as such)

### Changed

Expand Down Expand Up @@ -211,5 +214,3 @@ Animation system was completely rewritten in Chart.js v3. Each property can now
* `Chart.platform` no longer exists. Every chart instance now has a separate platform instance, `chart.platform`.
* `Chart.platforms` is an object that contains two usable platform classes, `BasicPlatform` and `DomPlatform`. It also contains `Platform`, a class that all platforms must extend from.
* If the canvas passed in is an instance of `OffscreenCanvas`, the `BasicPlatform` is automatically used.
* T override the platform class used in a chart instance, pass `platform: PlatformClass` in the config object. Note that the class should be passed, not an instance of the class.
* To disable CSS injection (necessary in some iframe contexts), `Chart.platform.disableCSSInjection = true` is no longer supported. Pass `disableCSSInjection: true` in the config object instead.
21 changes: 9 additions & 12 deletions src/core/core.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ function onAnimationProgress(ctx) {
* Chart.js can take a string id of a canvas element, a 2d context, or a canvas element itself.
* Attempt to unwrap the item passed into the chart constructor so that it is a canvas element (if possible).
*/
function unwrapItem(item) {
function getCanvas(item) {
if (typeof document !== undefined && typeof item === 'string') {
item = document.getElementById(item);
} else if (item.length) {
Expand All @@ -172,10 +172,10 @@ class Chart {
const me = this;

config = initConfig(config);
const unwrappedItem = unwrapItem(item);
me.initializePlatform(unwrappedItem, config);
const initialCanvas = getCanvas(item);
me._initializePlatform(initialCanvas, config);

const context = me.platform.acquireContext(unwrappedItem, config);
const context = me.platform.acquireContext(initialCanvas, config);
const canvas = context && context.canvas;
const height = canvas && canvas.height;
const width = canvas && canvas.width;
Expand Down Expand Up @@ -217,14 +217,14 @@ class Chart {
Animator.listen(me, 'complete', onAnimationsComplete);
Animator.listen(me, 'progress', onAnimationProgress);

me.initialize();
me._initialize();
me.update();
}

/**
* @private
*/
initialize() {
_initialize() {
const me = this;

// Before init plugin notification
Expand All @@ -250,17 +250,14 @@ class Chart {
/**
* @private
*/
initializePlatform(item, config) {
_initializePlatform(canvas, config) {
const me = this;

if (config.platform) {
if (!(config.platform instanceof Platform)) {
throw new Error('If config.platform is used, it must be an instance of Chart.platforms.Platform or one of its descendants');
}
me.platform = new config.platform(config);
} else if (typeof window === 'undefined' || typeof document === 'undefined' || !item) {
} else if (typeof window === 'undefined' || typeof document === 'undefined') {
me.platform = new BasicPlatform(config);
} else if (window.OffscreenCanvas && item instanceof window.OffscreenCanvas) {
} else if (window.OffscreenCanvas && canvas instanceof window.OffscreenCanvas) {
me.platform = new BasicPlatform(config);
} else {
me.platform = new DomPlatform(config);
Expand Down
21 changes: 12 additions & 9 deletions src/platforms/platform.dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,10 +315,13 @@ function injectCSS(rootNode, css) {

/**
* Platform class for charts that can access the DOM and global window/document properties
* @constructor
* @extends Platform
*/
export default class DomPlatform extends Platform {
/**
* @constructor
* @param {object} options - The chart options
*/
constructor(options) {
super();

Expand All @@ -329,7 +332,7 @@ export default class DomPlatform extends Platform {
* to be manually imported to make this library compatible with any CSP.
* See https://github.com/chartjs/Chart.js/issues/5208
*/
this.disableCSSInjection = options.disableCSSInjection || false;
this.disableCSSInjection = options.disableCSSInjection;
}

/**
Expand All @@ -348,24 +351,24 @@ export default class DomPlatform extends Platform {
}
}

acquireContext(item, config) {
acquireContext(canvas, config) {
// 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 && item.getContext && item.getContext('2d');
var context = canvas && canvas.getContext && canvas.getContext('2d');

// `instanceof HTMLCanvasElement/CanvasRenderingContext2D` fails when the item is
// `instanceof HTMLCanvasElement/CanvasRenderingContext2D` fails when the canvas is
// inside an iframe or when running in a protected environment. We could guess the
// types from their toString() value but let's keep things flexible and assume it's
// a sufficient condition if the item has a context2D which has item as `canvas`.
// a sufficient condition if the canvas has a context2D which has canvas as `canvas`.
// https://github.com/chartjs/Chart.js/issues/3887
// https://github.com/chartjs/Chart.js/issues/4102
// https://github.com/chartjs/Chart.js/issues/4152
if (context && context.canvas === item) {
if (context && context.canvas === canvas) {
// Load platform resources on first chart creation, to make it possible to
// import the library before setting platform options.
this._ensureLoaded(item);
initCanvas(item, config);
this._ensureLoaded(canvas);
initCanvas(canvas, config);
return context;
}

Expand Down
6 changes: 4 additions & 2 deletions src/platforms/platform.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@
*/
export default class Platform {
/**
* @param {object} options - The chart options
* @constructor
* The Platform constructor has no required parameters, but chart options are passed in as
* the first and only parameter to all Platform constructors.
*/
constructor() {}

/**
* Called at chart construction time, returns a context2d instance implementing
* the [W3C Canvas 2D Context API standard]{@link https://www.w3.org/TR/2dcontext/}.
* @param {*} item - The native item from which to acquire context (platform specific)
* @param {canvas} canvas - The canvas from which to acquire context (platform specific)
* @param {object} options - The chart options
* @returns {CanvasRenderingContext2D} context2d instance
*/
Expand Down

0 comments on commit c005cad

Please sign in to comment.