Refactor helpers.canvas.drawPoint() (#5623)

Bring ctx.beginPath() before switch and replace ctx.fillRect()/ctx.strokeRect() with ctx.rect()/ctx.fill() to make it consistent with the other styles. It is also preferable that helpers.canvas.roundedRect() include ctx.closePath() at the end because CanvasRenderingContext2D.rect() closes the subpath.

Get rid of ctx.closePath() for cross, crossRot, star, line and dash because these have no closed path shape, and it should be avoided that ctx.closePath() makes a round-trip path.

diff --git a/src/helpers/helpers.canvas.js b/src/helpers/helpers.canvas.js
index 26f7d37212f31da8a803011d9c78c6ec5c9f343d..60fb6e1a2991230c000d6cb495a6b3c8827ed473 100644 (file)
--- a/src/helpers/helpers.canvas.js
+++ b/src/helpers/helpers.canvas.js
@@ -41,6 +41,8 @@ var exports = module.exports = {
                        ctx.arcTo(x, y + height, x, y + height - r, r);
                        ctx.lineTo(x, y + r);
                        ctx.arcTo(x, y, x + r, y, r);
+                       ctx.closePath();
+                       ctx.moveTo(x, y);
                } else {
                        ctx.rect(x, y, width, height);
                }
@@ -65,77 +67,61 @@ var exports = module.exports = {
                ctx.save();
                ctx.translate(x, y);
                ctx.rotate(rotation * Math.PI / 180);
+               ctx.beginPath();
 
                switch (style) {
                // Default includes circle
                default:
-                       ctx.beginPath();
                        ctx.arc(0, 0, radius, 0, Math.PI * 2);
                        ctx.closePath();
-                       ctx.fill();
                        break;
                case 'triangle':
-                       ctx.beginPath();
                        edgeLength = 3 * radius / Math.sqrt(3);
                        height = edgeLength * Math.sqrt(3) / 2;
                        ctx.moveTo(-edgeLength / 2, height / 3);
                        ctx.lineTo(edgeLength / 2, height / 3);
                        ctx.lineTo(0, -2 * height / 3);
                        ctx.closePath();
-                       ctx.fill();
                        break;
                case 'rect':
                        size = 1 / Math.SQRT2 * radius;
-                       ctx.beginPath();
-                       ctx.fillRect(-size, -size, 2 * size, 2 * size);
-                       ctx.strokeRect(-size, -size, 2 * size, 2 * size);
+                       ctx.rect(-size, -size, 2 * size, 2 * size);
                        break;
                case 'rectRounded':
                        var offset = radius / Math.SQRT2;
                        var leftX = -offset;
                        var topY = -offset;
                        var sideSize = Math.SQRT2 * radius;
-                       ctx.beginPath();
 
                        // NOTE(SB) the rounded rect implementation changed to use `arcTo`
                        // instead of `quadraticCurveTo` since it generates better results
                        // when rect is almost a circle. 0.425 (instead of 0.5) produces
                        // results visually closer to the previous impl.
                        this.roundedRect(ctx, leftX, topY, sideSize, sideSize, radius * 0.425);
-
-                       ctx.closePath();
-                       ctx.fill();
                        break;
                case 'rectRot':
                        size = 1 / Math.SQRT2 * radius;
-                       ctx.beginPath();
                        ctx.moveTo(-size, 0);
                        ctx.lineTo(0, size);
                        ctx.lineTo(size, 0);
                        ctx.lineTo(0, -size);
                        ctx.closePath();
-                       ctx.fill();
                        break;
                case 'cross':
-                       ctx.beginPath();
                        ctx.moveTo(0, radius);
                        ctx.lineTo(0, -radius);
                        ctx.moveTo(-radius, 0);
                        ctx.lineTo(radius, 0);
-                       ctx.closePath();
                        break;
                case 'crossRot':
-                       ctx.beginPath();
                        xOffset = Math.cos(Math.PI / 4) * radius;
                        yOffset = Math.sin(Math.PI / 4) * radius;
                        ctx.moveTo(-xOffset, -yOffset);
                        ctx.lineTo(xOffset, yOffset);
                        ctx.moveTo(-xOffset, yOffset);
                        ctx.lineTo(xOffset, -yOffset);
-                       ctx.closePath();
                        break;
                case 'star':
-                       ctx.beginPath();
                        ctx.moveTo(0, radius);
                        ctx.lineTo(0, -radius);
                        ctx.moveTo(-radius, 0);
@@ -146,22 +132,18 @@ var exports = module.exports = {
                        ctx.lineTo(xOffset, yOffset);
                        ctx.moveTo(-xOffset, yOffset);
                        ctx.lineTo(xOffset, -yOffset);
-                       ctx.closePath();
                        break;
                case 'line':
-                       ctx.beginPath();
                        ctx.moveTo(-radius, 0);
                        ctx.lineTo(radius, 0);
-                       ctx.closePath();
                        break;
                case 'dash':
-                       ctx.beginPath();
                        ctx.moveTo(0, 0);
                        ctx.lineTo(radius, 0);
-                       ctx.closePath();
                        break;
                }
 
+               ctx.fill();
                ctx.stroke();
                ctx.restore();
        },
@@ -224,5 +206,4 @@ helpers.clear = exports.clear;
 helpers.drawRoundedRectangle = function(ctx) {
        ctx.beginPath();
        exports.roundedRect.apply(exports, arguments);
-       ctx.closePath();
 };

diff --git a/test/specs/element.point.tests.js b/test/specs/element.point.tests.js
index b2f803416b5bf2e136fdb9c996aa145296045e97..f1da35a50d4882b068089f8345548ec0bd1b716b 100644 (file)
--- a/test/specs/element.point.tests.js
+++ b/test/specs/element.point.tests.js
@@ -232,11 +232,11 @@ describe('Point element tests', function() {
                        name: 'beginPath',
                        args: []
                }, {
-                       name: 'fillRect',
+                       name: 'rect',
                        args: [0 - 1 / Math.SQRT2 * 2, 0 - 1 / Math.SQRT2 * 2, 2 / Math.SQRT2 * 2, 2 / Math.SQRT2 * 2]
                }, {
-                       name: 'strokeRect',
-                       args: [0 - 1 / Math.SQRT2 * 2, 0 - 1 / Math.SQRT2 * 2, 2 / Math.SQRT2 * 2, 2 / Math.SQRT2 * 2]
+                       name: 'fill',
+                       args: []
                }, {
                        name: 'stroke',
                        args: []
@@ -358,8 +358,8 @@ describe('Point element tests', function() {
                        name: 'lineTo',
                        args: [2, 0],
                }, {
-                       name: 'closePath',
-                       args: [],
+                       name: 'fill',
+                       args: []
                }, {
                        name: 'stroke',
                        args: []
@@ -406,8 +406,8 @@ describe('Point element tests', function() {
                        name: 'lineTo',
                        args: [0 + Math.cos(Math.PI / 4) * 2, 0 - Math.sin(Math.PI / 4) * 2],
                }, {
-                       name: 'closePath',
-                       args: [],
+                       name: 'fill',
+                       args: []
                }, {
                        name: 'stroke',
                        args: []
@@ -466,8 +466,8 @@ describe('Point element tests', function() {
                        name: 'lineTo',
                        args: [0 + Math.cos(Math.PI / 4) * 2, 0 - Math.sin(Math.PI / 4) * 2],
                }, {
-                       name: 'closePath',
-                       args: [],
+                       name: 'fill',
+                       args: []
                }, {
                        name: 'stroke',
                        args: []
@@ -508,8 +508,8 @@ describe('Point element tests', function() {
                        name: 'lineTo',
                        args: [2, 0],
                }, {
-                       name: 'closePath',
-                       args: [],
+                       name: 'fill',
+                       args: []
                }, {
                        name: 'stroke',
                        args: []
@@ -550,8 +550,8 @@ describe('Point element tests', function() {
                        name: 'lineTo',
                        args: [2, 0],
                }, {
-                       name: 'closePath',
-                       args: [],
+                       name: 'fill',
+                       args: []
                }, {
                        name: 'stroke',
                        args: []

diff --git a/test/specs/global.deprecations.tests.js b/test/specs/global.deprecations.tests.js
index 535b9af3307041896c242d5c64c7b0bacbc62d38..11d25d1d9b69ce6d200d41f988f82920f95454cd 100644 (file)
--- a/test/specs/global.deprecations.tests.js
+++ b/test/specs/global.deprecations.tests.js
@@ -113,7 +113,6 @@ describe('Deprecations', function() {
 
                                var calls = ctx.getCalls();
                                expect(calls[0]).toEqual({name: 'beginPath', args: []});
-                               expect(calls[calls.length - 1]).toEqual({name: 'closePath', args: []});
                                expect(Chart.helpers.canvas.roundedRect).toHaveBeenCalledWith(ctx, 10, 20, 30, 40, 5);
                        });
                });

diff --git a/test/specs/helpers.canvas.tests.js b/test/specs/helpers.canvas.tests.js
index 0c20628d456ef7a1c71741af142260371e2bdafd..1a342c1cb3bf741a8daad806b4673c984562e98a 100644 (file)
--- a/test/specs/helpers.canvas.tests.js
+++ b/test/specs/helpers.canvas.tests.js
@@ -36,7 +36,9 @@ describe('Chart.helpers.canvas', function() {
                                {name: 'lineTo', args: [15, 60]},
                                {name: 'arcTo', args: [10, 60, 10, 55, 5]},
                                {name: 'lineTo', args: [10, 25]},
-                               {name: 'arcTo', args: [10, 20, 15, 20, 5]}
+                               {name: 'arcTo', args: [10, 20, 15, 20, 5]},
+                               {name: 'closePath', args: []},
+                               {name: 'moveTo', args: [10, 20]}
                        ]);
                });
                it('should optimize path if radius is 0', function() {