From b97a0a09014c732fce4b3f752f2968b24f48441b Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 19:36:36 +0300 Subject: [PATCH 01/24] fix(): call dispose after render cycle rename `isRendering` => `nextRenderHandle` introduce `destroy` instread of `dispose refactor `dispose` to call destroy after rendering finished --- src/canvas.class.ts | 17 +++++++--- src/shapes/object.class.ts | 3 +- src/static_canvas.class.ts | 67 +++++++++++++++++++++++++++++--------- 3 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/canvas.class.ts b/src/canvas.class.ts index 5ad7f28a555..5d73499540d 100644 --- a/src/canvas.class.ts +++ b/src/canvas.class.ts @@ -1312,17 +1312,24 @@ import { Point } from './point.class'; }, /** - * Clears a canvas element and removes all event listeners - * @return {fabric.Canvas} thisArg - * @chainable + * Clears the canvas element, disposes objects, removes all event listeners and frees resources + * + * **CAUTION**: + * + * This method is **UNSAFE**. + * You may encounter a race condition using it if there's a requested render. + * Call this method only if you are sure rendering has settled. + * Consider using {@link dispose} as it is **SAFE** + * + * @private */ - dispose: function () { + destroy: function () { var wrapperEl = this.wrapperEl, lowerCanvasEl = this.lowerCanvasEl, upperCanvasEl = this.upperCanvasEl, cacheCanvasEl = this.cacheCanvasEl; this.removeListeners(); - this.callSuper('dispose'); + this.callSuper('destroy'); wrapperEl.removeChild(upperCanvasEl); wrapperEl.removeChild(lowerCanvasEl); this.contextCache = null; diff --git a/src/shapes/object.class.ts b/src/shapes/object.class.ts index 8370146e374..13190b600f7 100644 --- a/src/shapes/object.class.ts +++ b/src/shapes/object.class.ts @@ -1738,7 +1738,8 @@ import { pick } from '../util/misc/pick'; // since this canvas is a simple element for the process, we remove references // to objects in this way in order to avoid object trashing. canvas._objects = []; - canvas.dispose(); + // since render has settled it is safe to destroy canvas + canvas.destroy(); canvas = null; return canvasEl; diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index a2b65cc3ae9..89fc1a36891 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -691,7 +691,7 @@ import { pick } from './util/misc/pick'; * @chainable */ renderAndReset: function() { - this.isRendering = 0; + this.nextRenderHandle = 0; this.renderAll(); }, @@ -703,8 +703,8 @@ import { pick } from './util/misc/pick'; * @chainable */ requestRenderAll: function () { - if (!this.isRendering) { - this.isRendering = fabric.util.requestAnimFrame(this.renderAndResetBound); + if (!this.nextRenderHandle) { + this.nextRenderHandle = fabric.util.requestAnimFrame(this.renderAndResetBound); } return this; }, @@ -734,9 +734,9 @@ import { pick } from './util/misc/pick'; }, cancelRequestedRender: function() { - if (this.isRendering) { - fabric.util.cancelAnimFrame(this.isRendering); - this.isRendering = 0; + if (this.nextRenderHandle) { + fabric.util.cancelAnimFrame(this.nextRenderHandle); + this.nextRenderHandle = 0; } }, @@ -747,7 +747,12 @@ import { pick } from './util/misc/pick'; * @return {fabric.Canvas} instance * @chainable */ - renderCanvas: function(ctx, objects) { + renderCanvas: function (ctx, objects) { + + if (this.destroyed) { + return; + } + var v = this.viewportTransform, path = this.clipPath; this.cancelRequestedRender(); this.calcViewportBoundaries(); @@ -778,6 +783,10 @@ import { pick } from './util/misc/pick'; this.drawControls(ctx); } this.fire('after:render', { ctx: ctx, }); + if (this.__cleanupTask) { + this.__cleanupTask(); + this.__cleanupTask = undefined; + } }, /** @@ -1609,16 +1618,43 @@ import { pick } from './util/misc/pick'; }, /** - * Clears a canvas element and dispose objects - * @return {fabric.Canvas} thisArg - * @chainable + * Waits until rendering has settled to destroy the canvas + * @returns {Promise} a promise resolving once the canvas has been destroyed */ dispose: function () { - // cancel eventually ongoing renders - if (this.isRendering) { - fabric.util.cancelAnimFrame(this.isRendering); - this.isRendering = 0; - } + return new Promise((resolve, reject) => { + const task = () => { + this.destroy(); + resolve(); + } + task.kill = reject; + if (this.__cleanupTask) { + this.__cleanupTask.kill(); + } + if (this.nextRenderHandle) { + this.__cleanupTask = task; + } + else { + task(); + } + }); + }, + + /** + * Clears the canvas element, disposes objects and frees resources + * + * **CAUTION**: + * + * This method is **UNSAFE**. + * You may encounter a race condition using it if there's a requested render. + * Call this method only if you are sure rendering has settled. + * Consider using {@link dispose} as it is **SAFE** + * + * @private + */ + destroy: function () { + this.destroyed = true; + this.cancelRequestedRender(); this.forEachObject(function(object) { object.dispose && object.dispose(); }); @@ -1645,7 +1681,6 @@ import { pick } from './util/misc/pick'; this.lowerCanvasEl.setAttribute('height', this.height); fabric.util.cleanUpJsdomNode(this.lowerCanvasEl); this.lowerCanvasEl = undefined; - return this; }, /** From c4d5532e73223f24c931c38d36600b6567d2096f Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 19:39:47 +0300 Subject: [PATCH 02/24] block requestRenderAll --- src/static_canvas.class.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index 89fc1a36891..d8fc25ac4c5 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -703,7 +703,7 @@ import { pick } from './util/misc/pick'; * @chainable */ requestRenderAll: function () { - if (!this.nextRenderHandle) { + if (!this.nextRenderHandle && !this.destroyed) { this.nextRenderHandle = fabric.util.requestAnimFrame(this.renderAndResetBound); } return this; From b12491dbbb725b00943629bb3e7d4917188cdb29 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 19:42:30 +0300 Subject: [PATCH 03/24] fix(): `renderCanvas` `canvas#toCanvasElement` used to kill requested renders --- src/static_canvas.class.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index d8fc25ac4c5..1b4dcf978aa 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -754,10 +754,10 @@ import { pick } from './util/misc/pick'; } var v = this.viewportTransform, path = this.clipPath; - this.cancelRequestedRender(); this.calcViewportBoundaries(); this.clearContext(ctx); fabric.util.setImageSmoothing(ctx, this.imageSmoothingEnabled); + // node-canvas ctx.patternQuality = 'best'; this.fire('before:render', { ctx: ctx, }); this._renderBackground(ctx); From ba6c4d055e89f46629973aba701c3dc5b013c57c Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 19:44:05 +0300 Subject: [PATCH 04/24] fix tests --- test/lib/visualTestLoop.js | 4 ++-- test/unit/canvas.js | 13 +++++++------ test/unit/canvas_events.js | 14 +++++++------- test/unit/canvas_static.js | 11 ++++++----- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/test/lib/visualTestLoop.js b/test/lib/visualTestLoop.js index 45845a7fd65..73ab11c2d77 100644 --- a/test/lib/visualTestLoop.js +++ b/test/lib/visualTestLoop.js @@ -156,7 +156,7 @@ canvas.height = height; var ctx = canvas.getContext('2d'); var output = ctx.getImageData(0, 0, width, height); - getImage(getGoldeName(golden), renderedCanvas, function(goldenImage) { + getImage(getGoldeName(golden), renderedCanvas, async (goldenImage) => { ctx.drawImage(goldenImage, 0, 0); visualCallback.addArguments({ enabled: true, @@ -181,7 +181,7 @@ if ((!isOK && QUnit.debugVisual) || QUnit.recreateVisualRefs) { generateGolden(getGoldeName(golden), renderedCanvas); } - fabricCanvas.dispose(); + await fabricCanvas.dispose(); done(); }); }); diff --git a/test/unit/canvas.js b/test/unit/canvas.js index 838af5a4167..181626fb063 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -2083,7 +2083,7 @@ assert.equal(aGroup._objects[3], circle2); }); - QUnit.test('dispose', function(assert) { + QUnit.test('dispose', async function(assert) { //made local vars to do not dispose the external canvas var el = fabric.document.createElement('canvas'), parentEl = fabric.document.createElement('div'), @@ -2123,10 +2123,11 @@ assert.equal(parentEl.childNodes.length, 1, 'parent div should have 1 child'); assert.notEqual(parentEl.firstChild, canvas.getElement(), 'canvas should not be parent div firstChild'); assert.ok(typeof canvas.dispose === 'function'); + assert.ok(typeof canvas.destroy === 'function'); canvas.add(makeRect(), makeRect(), makeRect()); canvas.item(0).animate('scaleX', 10); assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation'); - canvas.dispose(); + await canvas.dispose(); canvas.cancelRequestedRender(); assert.equal(fabric.runningAnimations.length, 0, 'dispose should clear running animations'); assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); @@ -2147,7 +2148,7 @@ }); - QUnit.test('dispose + set dimensions', function (assert) { + QUnit.test('dispose + set dimensions', async function (assert) { //made local vars to do not dispose the external canvas var el = fabric.document.createElement('canvas'), parentEl = fabric.document.createElement('div'); @@ -2170,7 +2171,7 @@ assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); - canvas.dispose(); + await canvas.dispose(); assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); assert.equal(el.width, 500, 'restored width'); @@ -2178,7 +2179,7 @@ }); - // QUnit.test('dispose', function(assert) { + // QUnit.test('dispose', async function(assert) { // function invokeEventsOnCanvas() { // // nextSibling because we need to invoke events on upper canvas // simulateEvent(canvas.getElement().nextSibling, 'mousedown'); @@ -2213,7 +2214,7 @@ // invokeEventsOnCanvas(); // assertInvocationsCount(); - // canvas.dispose(); + // await canvas.dispose(); // assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); // invokeEventsOnCanvas(); diff --git a/test/unit/canvas_events.js b/test/unit/canvas_events.js index a965f438753..5ae00c1a588 100644 --- a/test/unit/canvas_events.js +++ b/test/unit/canvas_events.js @@ -562,8 +562,8 @@ assert.deepEqual(fired, eventNames, 'bad drop event fired'); }); - QUnit.test('drag event cycle', function (assert) { - function testDragCycle(cycle, canDrop) { + QUnit.test('drag event cycle', async function (assert) { + async function testDragCycle(cycle, canDrop) { var c = new fabric.Canvas(); var rect = new fabric.Rect({ width: 10, height: 10 }); rect.canDrop = function () { @@ -585,23 +585,23 @@ event.clientY = 5; c.upperCanvasEl.dispatchEvent(event); }); - c.dispose(); + await c.dispose(); assert.equal(canvasRegistry.length, cycle.length, 'should fire cycle on canvas'); assert.deepEqual(canvasRegistry, cycle, 'should fire all events on canvas'); return registery } var cycle, res; cycle = ['dragenter', 'dragover', 'dragover', 'dragover', 'drop']; - res = testDragCycle(cycle, true); + res = await testDragCycle(cycle, true); assert.deepEqual(res, cycle, 'should fire all events on rect'); cycle = ['dragenter', 'dragover', 'dragover', 'dragover', 'dragleave']; - res = testDragCycle(cycle, true); + res = await testDragCycle(cycle, true); assert.deepEqual(res, cycle, 'should fire all events on rect'); cycle = ['dragenter', 'dragover', 'dragover', 'dragover', 'drop']; - res = testDragCycle(cycle); + res = await testDragCycle(cycle); assert.deepEqual(res, cycle, 'should fire all events on rect'); cycle = ['dragenter', 'dragover', 'dragover', 'dragover', 'dragleave']; - res = testDragCycle(cycle); + res = await testDragCycle(cycle); assert.deepEqual(res, cycle, 'should fire all events on rect'); }); diff --git a/test/unit/canvas_static.js b/test/unit/canvas_static.js index cd85ef7de7e..12f6a2a909b 100644 --- a/test/unit/canvas_static.js +++ b/test/unit/canvas_static.js @@ -1610,13 +1610,14 @@ assert.equal(canvas.toString(), '#'); }); - QUnit.test('dispose clear references', function(assert) { + QUnit.test('dispose clear references', async function(assert) { var canvas2 = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); assert.ok(typeof canvas2.dispose === 'function'); + assert.ok(typeof canvas2.destroy === 'function'); canvas2.add(makeRect(), makeRect(), makeRect()); var lowerCanvas = canvas2.lowerCanvasEl; assert.equal(lowerCanvas.getAttribute('data-fabric'), 'main', 'lowerCanvasEl should be marked by fabric'); - canvas2.dispose(); + await canvas2.dispose(); assert.equal(canvas2.getObjects().length, 0, 'dispose should clear canvas'); assert.equal(canvas2.lowerCanvasEl, null, 'dispose should clear lowerCanvasEl'); assert.equal(lowerCanvas.hasAttribute('data-fabric'), false, 'dispose should clear lowerCanvasEl data-fabric attr'); @@ -2003,11 +2004,11 @@ QUnit.test('requestRenderAll and cancelRequestedRender', function(assert) { var canvas2 = new fabric.StaticCanvas(); - assert.equal(canvas2.isRendering, undefined, 'no redering is in progress'); + assert.equal(canvas2.nextRenderHandle, undefined, 'no redering is in progress'); canvas2.requestRenderAll(); - assert.notEqual(canvas2.isRendering, 0, 'a rendering is scehduled'); + assert.notEqual(canvas2.nextRenderHandle, 0, 'a rendering is scehduled'); canvas2.cancelRequestedRender(); - assert.equal(canvas2.isRendering, 0, 'rendering cancelled'); + assert.equal(canvas2.nextRenderHandle, 0, 'rendering cancelled'); }); // QUnit.test('backgroundImage', function(assert) { From c45395c371a9d8070efeb9eaf8f8d14d05eed7d9 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 20:41:35 +0300 Subject: [PATCH 05/24] add test --- test/unit/canvas_static.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/unit/canvas_static.js b/test/unit/canvas_static.js index 12f6a2a909b..8faef94d0d2 100644 --- a/test/unit/canvas_static.js +++ b/test/unit/canvas_static.js @@ -1618,12 +1618,25 @@ var lowerCanvas = canvas2.lowerCanvasEl; assert.equal(lowerCanvas.getAttribute('data-fabric'), 'main', 'lowerCanvasEl should be marked by fabric'); await canvas2.dispose(); + assert.equal(canvas.destroyed, true, 'dispose should flag destroyed'); assert.equal(canvas2.getObjects().length, 0, 'dispose should clear canvas'); assert.equal(canvas2.lowerCanvasEl, null, 'dispose should clear lowerCanvasEl'); assert.equal(lowerCanvas.hasAttribute('data-fabric'), false, 'dispose should clear lowerCanvasEl data-fabric attr'); assert.equal(canvas2.contextContainer, null, 'dispose should clear contextContainer'); }); + QUnit.test('dispose edge cases', async function (assert) { + const done = assert.async(); + const canvas2 = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); + canvas2.requestRenderAll(); + assert.rejects(canvas2.dispose()); + assert.rejects(canvas2.dispose()); + assert.notOk(canvas2.destroyed, 'should not have been destroyed yet'); + await canvas2.dispose(); + assert.ok(canvas2.destroyed, 'should not have been destroyed'); + done(); + }); + QUnit.test('clone', function(assert) { var done = assert.async(); var canvas2 = new fabric.StaticCanvas(null, { renderOnAddRemove: false, width: 10, height: 10 }); From b56685d7545595d56697bc006a559e63f9423e12 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 21:05:45 +0300 Subject: [PATCH 06/24] fix(): stop requestRenderAll after dispose --- src/static_canvas.class.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index 1b4dcf978aa..06327d90634 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -703,7 +703,7 @@ import { pick } from './util/misc/pick'; * @chainable */ requestRenderAll: function () { - if (!this.nextRenderHandle && !this.destroyed) { + if (!this.nextRenderHandle && !this.disposed && !this.destroyed) { this.nextRenderHandle = fabric.util.requestAnimFrame(this.renderAndResetBound); } return this; @@ -1622,6 +1622,7 @@ import { pick } from './util/misc/pick'; * @returns {Promise} a promise resolving once the canvas has been destroyed */ dispose: function () { + this.disposed = true; return new Promise((resolve, reject) => { const task = () => { this.destroy(); @@ -1631,7 +1632,11 @@ import { pick } from './util/misc/pick'; if (this.__cleanupTask) { this.__cleanupTask.kill(); } - if (this.nextRenderHandle) { + + if (this.destroyed) { + return; + } + else if (this.nextRenderHandle) { this.__cleanupTask = task; } else { From 384fb8d2a4ce37ee7760131e2e451f6133d659f2 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 21:24:57 +0300 Subject: [PATCH 07/24] fix if destroyed --- src/static_canvas.class.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index 06327d90634..aadccbab366 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -1619,14 +1619,15 @@ import { pick } from './util/misc/pick'; /** * Waits until rendering has settled to destroy the canvas - * @returns {Promise} a promise resolving once the canvas has been destroyed + * @returns {Promise} a promise resolving to true once the canvas has been destroyed or to `false` if the canvas has was already destroyed + * @throws if aborted by a successive call */ dispose: function () { this.disposed = true; - return new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { const task = () => { this.destroy(); - resolve(); + resolve(true); } task.kill = reject; if (this.__cleanupTask) { @@ -1634,7 +1635,7 @@ import { pick } from './util/misc/pick'; } if (this.destroyed) { - return; + resolve(false); } else if (this.nextRenderHandle) { this.__cleanupTask = task; From 52985ef9d6a31e93d91b432638ec035876087714 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 21:51:26 +0300 Subject: [PATCH 08/24] reason --- src/static_canvas.class.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index aadccbab366..aebf0f3398f 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -1631,7 +1631,7 @@ import { pick } from './util/misc/pick'; } task.kill = reject; if (this.__cleanupTask) { - this.__cleanupTask.kill(); + this.__cleanupTask.kill('aborted'); } if (this.destroyed) { From 6a8f057633b9d29fd19de5cd225f96dfcfca4541 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 21:52:22 +0300 Subject: [PATCH 09/24] tests --- test/unit/canvas.js | 5 +-- test/unit/canvas_static.js | 84 ++++++++++++++++++++++++++++++-------- 2 files changed, 69 insertions(+), 20 deletions(-) diff --git a/test/unit/canvas.js b/test/unit/canvas.js index 181626fb063..6371929e3e3 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -2083,7 +2083,7 @@ assert.equal(aGroup._objects[3], circle2); }); - QUnit.test('dispose', async function(assert) { + QUnit.test('dispose: clear refs', async function(assert) { //made local vars to do not dispose the external canvas var el = fabric.document.createElement('canvas'), parentEl = fabric.document.createElement('div'), @@ -2128,7 +2128,6 @@ canvas.item(0).animate('scaleX', 10); assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation'); await canvas.dispose(); - canvas.cancelRequestedRender(); assert.equal(fabric.runningAnimations.length, 0, 'dispose should clear running animations'); assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); assert.equal(parentEl.childNodes.length, 1, 'parent has always 1 child'); @@ -2148,7 +2147,7 @@ }); - QUnit.test('dispose + set dimensions', async function (assert) { + QUnit.test('dispose edge case: `setDimensions` invoking `requestRenderAll`', async function (assert) { //made local vars to do not dispose the external canvas var el = fabric.document.createElement('canvas'), parentEl = fabric.document.createElement('div'); diff --git a/test/unit/canvas_static.js b/test/unit/canvas_static.js index 8faef94d0d2..1a5c1644216 100644 --- a/test/unit/canvas_static.js +++ b/test/unit/canvas_static.js @@ -1611,29 +1611,79 @@ }); QUnit.test('dispose clear references', async function(assert) { - var canvas2 = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); - assert.ok(typeof canvas2.dispose === 'function'); - assert.ok(typeof canvas2.destroy === 'function'); - canvas2.add(makeRect(), makeRect(), makeRect()); - var lowerCanvas = canvas2.lowerCanvasEl; + const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); + assert.ok(typeof canvas.dispose === 'function'); + assert.ok(typeof canvas.destroy === 'function'); + canvas.add(makeRect(), makeRect(), makeRect()); + const lowerCanvas = canvas.lowerCanvasEl; assert.equal(lowerCanvas.getAttribute('data-fabric'), 'main', 'lowerCanvasEl should be marked by fabric'); - await canvas2.dispose(); + await canvas.dispose(); assert.equal(canvas.destroyed, true, 'dispose should flag destroyed'); - assert.equal(canvas2.getObjects().length, 0, 'dispose should clear canvas'); - assert.equal(canvas2.lowerCanvasEl, null, 'dispose should clear lowerCanvasEl'); + assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); + assert.equal(canvas.lowerCanvasEl, null, 'dispose should clear lowerCanvasEl'); assert.equal(lowerCanvas.hasAttribute('data-fabric'), false, 'dispose should clear lowerCanvasEl data-fabric attr'); - assert.equal(canvas2.contextContainer, null, 'dispose should clear contextContainer'); + assert.equal(canvas.contextContainer, null, 'dispose should clear contextContainer'); }); - QUnit.test('dispose edge cases', async function (assert) { + QUnit.test('dispose', async function (assert) { const done = assert.async(); - const canvas2 = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); - canvas2.requestRenderAll(); - assert.rejects(canvas2.dispose()); - assert.rejects(canvas2.dispose()); - assert.notOk(canvas2.destroyed, 'should not have been destroyed yet'); - await canvas2.dispose(); - assert.ok(canvas2.destroyed, 'should not have been destroyed'); + const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); + assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); + await canvas.dispose(); + assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); + done(); + }); + + QUnit.test('dispose edge cases: multiple calls', async function (assert) { + const done = assert.async(); + const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); + assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); + const res = await Promise.all([ + canvas.dispose(), + canvas.dispose(), + canvas.dispose(), + ]); + assert.ok(canvas.disposed, 'should have flagged `disposed`'); + assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); + assert.deepEqual(res, [true, false, false], 'should have disposed in the first call'); + done(); + }); + + QUnit.test('dispose edge cases: multiple calls after `requestRenderAll', async function (assert) { + const done = assert.async(); + const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); + assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); + canvas.requestRenderAll(); + const res = await Promise.allSettled([ + canvas.dispose(), + canvas.dispose(), + canvas.dispose(), + ]); + assert.ok(canvas.disposed, 'should have flagged `disposed`'); + assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); + assert.deepEqual(res, [ + { status: 'rejected', reason: 'aborted' }, + { status: 'rejected', reason: 'aborted' }, + { status: 'fulfilled', value: true } + ], 'should have disposed in the last call, aborting the other calls'); + done(); + }); + + QUnit.test.only('dispose edge cases: rendering after dispose', async function (assert) { + const done = assert.async(); + const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); + let called = 0; + assert.ok(await canvas.dispose(), 'should dispose'); + canvas.on('after:render', () => { + called++; + }) + canvas.fire('after:render'); + assert.equal(canvas.nextRenderHandle, undefined); + canvas.requestRenderAll(); + assert.equal(canvas.nextRenderHandle, undefined, '`requestRenderAll` should have no affect'); + assert.equal(called, 1, 'should not have rendered'); + canvas.renderAll(); + assert.equal(called, 1, 'should not have rendered'); done(); }); From 4634a72ef41c7c747a44f28325992f58db2221d6 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 22:02:43 +0300 Subject: [PATCH 10/24] create `canvas_dispose` --- test/unit/canvas.js | 137 --------------------- test/unit/canvas_dispose.js | 229 ++++++++++++++++++++++++++++++++++++ test/unit/canvas_static.js | 77 ------------ 3 files changed, 229 insertions(+), 214 deletions(-) create mode 100644 test/unit/canvas_dispose.js diff --git a/test/unit/canvas.js b/test/unit/canvas.js index 6371929e3e3..4d0142d3c4f 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -2083,143 +2083,6 @@ assert.equal(aGroup._objects[3], circle2); }); - QUnit.test('dispose: clear refs', async function(assert) { - //made local vars to do not dispose the external canvas - var el = fabric.document.createElement('canvas'), - parentEl = fabric.document.createElement('div'), - wrapperEl, lowerCanvasEl, upperCanvasEl; - el.width = 200; el.height = 200; - parentEl.className = 'rootNode'; - parentEl.appendChild(el); - - fabric.config.configure({ devicePixelRatio: 1.25 }); - - assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); - assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); - - el.style.position = 'relative'; - var elStyle = el.style.cssText; - assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); - - var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); - wrapperEl = canvas.wrapperEl; - lowerCanvasEl = canvas.lowerCanvasEl; - upperCanvasEl = canvas.upperCanvasEl; - assert.equal(parentEl.childNodes.length, 1, 'parentEl has still 1 child only'); - assert.equal(wrapperEl.childNodes.length, 2, 'wrapper should have 2 children'); - assert.equal(wrapperEl.tagName, 'DIV', 'We wrapped canvas with DIV'); - assert.equal(wrapperEl.className, canvas.containerClass, 'DIV class should be set'); - assert.equal(wrapperEl.childNodes[0], lowerCanvasEl, 'First child should be lowerCanvas'); - assert.equal(wrapperEl.childNodes[1], upperCanvasEl, 'Second child should be upperCanvas'); - assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); - assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); - if (!fabric.isLikelyNode) { - assert.equal(parentEl.childNodes[0], wrapperEl, 'wrapperEl is appendend to rootNode'); - } - //looks like i cannot use parentNode - //equal(wrapperEl, lowerCanvasEl.parentNode, 'lowerCanvas is appended to wrapperEl'); - //equal(wrapperEl, upperCanvasEl.parentNode, 'upperCanvas is appended to wrapperEl'); - //equal(parentEl, wrapperEl.parentNode, 'wrapperEl is appendend to rootNode'); - assert.equal(parentEl.childNodes.length, 1, 'parent div should have 1 child'); - assert.notEqual(parentEl.firstChild, canvas.getElement(), 'canvas should not be parent div firstChild'); - assert.ok(typeof canvas.dispose === 'function'); - assert.ok(typeof canvas.destroy === 'function'); - canvas.add(makeRect(), makeRect(), makeRect()); - canvas.item(0).animate('scaleX', 10); - assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation'); - await canvas.dispose(); - assert.equal(fabric.runningAnimations.length, 0, 'dispose should clear running animations'); - assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); - assert.equal(parentEl.childNodes.length, 1, 'parent has always 1 child'); - if (!fabric.isLikelyNode) { - assert.equal(parentEl.childNodes[0], lowerCanvasEl, 'canvas should be back to its firstChild place'); - } - assert.equal(canvas.wrapperEl, null, 'wrapperEl should be deleted'); - assert.equal(canvas.upperCanvasEl, null, 'upperCanvas should be deleted'); - assert.equal(canvas.lowerCanvasEl, null, 'lowerCanvasEl should be deleted'); - assert.equal(canvas.cacheCanvasEl, null, 'cacheCanvasEl should be deleted'); - assert.equal(canvas.contextTop, null, 'contextTop should be deleted'); - assert.equal(canvas.contextCache, null, 'contextCache should be deleted'); - assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); - assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); - assert.equal(el.width, 200, 'restored width'); - assert.equal(el.height, 200, 'restored height'); - - }); - - QUnit.test('dispose edge case: `setDimensions` invoking `requestRenderAll`', async function (assert) { - //made local vars to do not dispose the external canvas - var el = fabric.document.createElement('canvas'), - parentEl = fabric.document.createElement('div'); - el.width = 200; el.height = 200; - parentEl.className = 'rootNode'; - parentEl.appendChild(el); - - fabric.config.configure({ devicePixelRatio: 1.25 }); - - assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); - assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); - - el.style.position = 'relative'; - var elStyle = el.style.cssText; - assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); - - var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); - - canvas.setDimensions({ width: 500, height: 500 }); - assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); - assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); - - await canvas.dispose(); - assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); - assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); - assert.equal(el.width, 500, 'restored width'); - assert.equal(el.height, 500, 'restored height'); - - }); - - // QUnit.test('dispose', async function(assert) { - // function invokeEventsOnCanvas() { - // // nextSibling because we need to invoke events on upper canvas - // simulateEvent(canvas.getElement().nextSibling, 'mousedown'); - // simulateEvent(canvas.getElement().nextSibling, 'mouseup'); - // simulateEvent(canvas.getElement().nextSibling, 'mousemove'); - // } - // var assertInvocationsCount = function() { - // var message = 'event handler should not be invoked after `dispose`'; - // assert.equal(handlerInvocationCounts.__onMouseDown, 1); - // assert.equal(handlerInvocationCounts.__onMouseUp, 1); - // assert.equal(handlerInvocationCounts.__onMouseMove, 1); - // }; - - // assert.ok(typeof canvas.dispose === 'function'); - // canvas.add(makeRect(), makeRect(), makeRect()); - - // var handlerInvocationCounts = { - // __onMouseDown: 0, __onMouseUp: 0, __onMouseMove: 0 - // }; - - // // hijack event handlers - // canvas.__onMouseDown = function() { - // handlerInvocationCounts.__onMouseDown++; - // }; - // canvas.__onMouseUp = function() { - // handlerInvocationCounts.__onMouseUp++; - // }; - // canvas.__onMouseMove = function() { - // handlerInvocationCounts.__onMouseMove++; - // }; - - // invokeEventsOnCanvas(); - // assertInvocationsCount(); - - // await canvas.dispose(); - // assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); - - // invokeEventsOnCanvas(); - // assertInvocationsCount(); - // }); - QUnit.test('clone', function(assert) { var done = assert.async(); assert.ok(typeof canvas.clone === 'function'); diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js new file mode 100644 index 00000000000..cc0b19ba60e --- /dev/null +++ b/test/unit/canvas_dispose.js @@ -0,0 +1,229 @@ + + +function makeRect(options = {}) { + return new fabric.Rect({ width: 10, height: 10, ...options }); +} + +function assertCanvasDisposing(klass) { + + QUnit.test('dispose', async function (assert) { + const done = assert.async(); + const canvas = new klass(null, { renderOnAddRemove: false }); + assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); + await canvas.dispose(); + assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); + done(); + }); + + QUnit.test('dispose: clear references', async function (assert) { + const canvas = new klass(null, { renderOnAddRemove: false }); + assert.ok(typeof canvas.dispose === 'function'); + assert.ok(typeof canvas.destroy === 'function'); + canvas.add(makeRect(), makeRect(), makeRect()); + const lowerCanvas = canvas.lowerCanvasEl; + assert.equal(lowerCanvas.getAttribute('data-fabric'), 'main', 'lowerCanvasEl should be marked by fabric'); + await canvas.dispose(); + assert.equal(canvas.destroyed, true, 'dispose should flag destroyed'); + assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); + assert.equal(canvas.lowerCanvasEl, null, 'dispose should clear lowerCanvasEl'); + assert.equal(lowerCanvas.hasAttribute('data-fabric'), false, 'dispose should clear lowerCanvasEl data-fabric attr'); + assert.equal(canvas.contextContainer, null, 'dispose should clear contextContainer'); + }); + + QUnit.test('dispose edge cases: multiple calls', async function (assert) { + const done = assert.async(); + const canvas = new klass(null, { renderOnAddRemove: false }); + assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); + const res = await Promise.all([ + canvas.dispose(), + canvas.dispose(), + canvas.dispose(), + ]); + assert.ok(canvas.disposed, 'should have flagged `disposed`'); + assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); + assert.deepEqual(res, [true, false, false], 'should have disposed in the first call'); + done(); + }); + + QUnit.test('dispose edge cases: multiple calls after `requestRenderAll', async function (assert) { + const done = assert.async(); + const canvas = new klass(null, { renderOnAddRemove: false }); + assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); + canvas.requestRenderAll(); + const res = await Promise.allSettled([ + canvas.dispose(), + canvas.dispose(), + canvas.dispose(), + ]); + assert.ok(canvas.disposed, 'should have flagged `disposed`'); + assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); + assert.deepEqual(res, [ + { status: 'rejected', reason: 'aborted' }, + { status: 'rejected', reason: 'aborted' }, + { status: 'fulfilled', value: true } + ], 'should have disposed in the last call, aborting the other calls'); + done(); + }); + + QUnit.test('dispose edge cases: rendering after dispose', async function (assert) { + const done = assert.async(); + const canvas = new klass(null, { renderOnAddRemove: false }); + let called = 0; + assert.ok(await canvas.dispose(), 'should dispose'); + canvas.on('after:render', () => { + called++; + }) + canvas.fire('after:render'); + assert.equal(canvas.nextRenderHandle, undefined); + canvas.requestRenderAll(); + assert.equal(canvas.nextRenderHandle, undefined, '`requestRenderAll` should have no affect'); + assert.equal(called, 1, 'should not have rendered'); + canvas.renderAll(); + assert.equal(called, 1, 'should not have rendered'); + done(); + }); +} + +QUnit.module('StaticCanvas dispose') +assertCanvasDisposing(fabric.StaticCanvas); + + +QUnit.module('Canvas dispose') +assertCanvasDisposing(fabric.StaticCanvas); + +QUnit.test('dispose: clear refs', async function (assert) { + //made local vars to do not dispose the external canvas + var el = fabric.document.createElement('canvas'), + parentEl = fabric.document.createElement('div'), + wrapperEl, lowerCanvasEl, upperCanvasEl; + el.width = 200; el.height = 200; + parentEl.className = 'rootNode'; + parentEl.appendChild(el); + + fabric.config.configure({ devicePixelRatio: 1.25 }); + + assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); + assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); + + el.style.position = 'relative'; + var elStyle = el.style.cssText; + assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); + + var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); + wrapperEl = canvas.wrapperEl; + lowerCanvasEl = canvas.lowerCanvasEl; + upperCanvasEl = canvas.upperCanvasEl; + assert.equal(parentEl.childNodes.length, 1, 'parentEl has still 1 child only'); + assert.equal(wrapperEl.childNodes.length, 2, 'wrapper should have 2 children'); + assert.equal(wrapperEl.tagName, 'DIV', 'We wrapped canvas with DIV'); + assert.equal(wrapperEl.className, canvas.containerClass, 'DIV class should be set'); + assert.equal(wrapperEl.childNodes[0], lowerCanvasEl, 'First child should be lowerCanvas'); + assert.equal(wrapperEl.childNodes[1], upperCanvasEl, 'Second child should be upperCanvas'); + assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); + assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); + if (!fabric.isLikelyNode) { + assert.equal(parentEl.childNodes[0], wrapperEl, 'wrapperEl is appendend to rootNode'); + } + //looks like i cannot use parentNode + //equal(wrapperEl, lowerCanvasEl.parentNode, 'lowerCanvas is appended to wrapperEl'); + //equal(wrapperEl, upperCanvasEl.parentNode, 'upperCanvas is appended to wrapperEl'); + //equal(parentEl, wrapperEl.parentNode, 'wrapperEl is appendend to rootNode'); + assert.equal(parentEl.childNodes.length, 1, 'parent div should have 1 child'); + assert.notEqual(parentEl.firstChild, canvas.getElement(), 'canvas should not be parent div firstChild'); + assert.ok(typeof canvas.dispose === 'function'); + assert.ok(typeof canvas.destroy === 'function'); + canvas.add(makeRect(), makeRect(), makeRect()); + canvas.item(0).animate('scaleX', 10); + assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation'); + await canvas.dispose(); + assert.equal(fabric.runningAnimations.length, 0, 'dispose should clear running animations'); + assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); + assert.equal(parentEl.childNodes.length, 1, 'parent has always 1 child'); + if (!fabric.isLikelyNode) { + assert.equal(parentEl.childNodes[0], lowerCanvasEl, 'canvas should be back to its firstChild place'); + } + assert.equal(canvas.wrapperEl, null, 'wrapperEl should be deleted'); + assert.equal(canvas.upperCanvasEl, null, 'upperCanvas should be deleted'); + assert.equal(canvas.lowerCanvasEl, null, 'lowerCanvasEl should be deleted'); + assert.equal(canvas.cacheCanvasEl, null, 'cacheCanvasEl should be deleted'); + assert.equal(canvas.contextTop, null, 'contextTop should be deleted'); + assert.equal(canvas.contextCache, null, 'contextCache should be deleted'); + assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); + assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); + assert.equal(el.width, 200, 'restored width'); + assert.equal(el.height, 200, 'restored height'); + +}); + +QUnit.test('dispose edge case: `setDimensions` invoking `requestRenderAll`', async function (assert) { + //made local vars to do not dispose the external canvas + var el = fabric.document.createElement('canvas'), + parentEl = fabric.document.createElement('div'); + el.width = 200; el.height = 200; + parentEl.className = 'rootNode'; + parentEl.appendChild(el); + + fabric.config.configure({ devicePixelRatio: 1.25 }); + + assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); + assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); + + el.style.position = 'relative'; + var elStyle = el.style.cssText; + assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); + + var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); + + canvas.setDimensions({ width: 500, height: 500 }); + assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); + assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); + + await canvas.dispose(); + assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); + assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); + assert.equal(el.width, 500, 'restored width'); + assert.equal(el.height, 500, 'restored height'); + +}); + + // QUnit.test('dispose', async function(assert) { + // function invokeEventsOnCanvas() { + // // nextSibling because we need to invoke events on upper canvas + // simulateEvent(canvas.getElement().nextSibling, 'mousedown'); + // simulateEvent(canvas.getElement().nextSibling, 'mouseup'); + // simulateEvent(canvas.getElement().nextSibling, 'mousemove'); + // } + // var assertInvocationsCount = function() { + // var message = 'event handler should not be invoked after `dispose`'; + // assert.equal(handlerInvocationCounts.__onMouseDown, 1); + // assert.equal(handlerInvocationCounts.__onMouseUp, 1); + // assert.equal(handlerInvocationCounts.__onMouseMove, 1); + // }; + + // assert.ok(typeof canvas.dispose === 'function'); + // canvas.add(makeRect(), makeRect(), makeRect()); + + // var handlerInvocationCounts = { + // __onMouseDown: 0, __onMouseUp: 0, __onMouseMove: 0 + // }; + + // // hijack event handlers + // canvas.__onMouseDown = function() { + // handlerInvocationCounts.__onMouseDown++; + // }; + // canvas.__onMouseUp = function() { + // handlerInvocationCounts.__onMouseUp++; + // }; + // canvas.__onMouseMove = function() { + // handlerInvocationCounts.__onMouseMove++; + // }; + + // invokeEventsOnCanvas(); + // assertInvocationsCount(); + + // await canvas.dispose(); + // assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); + + // invokeEventsOnCanvas(); + // assertInvocationsCount(); + // }); \ No newline at end of file diff --git a/test/unit/canvas_static.js b/test/unit/canvas_static.js index 1a5c1644216..3f760bea1f2 100644 --- a/test/unit/canvas_static.js +++ b/test/unit/canvas_static.js @@ -1610,83 +1610,6 @@ assert.equal(canvas.toString(), '#'); }); - QUnit.test('dispose clear references', async function(assert) { - const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); - assert.ok(typeof canvas.dispose === 'function'); - assert.ok(typeof canvas.destroy === 'function'); - canvas.add(makeRect(), makeRect(), makeRect()); - const lowerCanvas = canvas.lowerCanvasEl; - assert.equal(lowerCanvas.getAttribute('data-fabric'), 'main', 'lowerCanvasEl should be marked by fabric'); - await canvas.dispose(); - assert.equal(canvas.destroyed, true, 'dispose should flag destroyed'); - assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); - assert.equal(canvas.lowerCanvasEl, null, 'dispose should clear lowerCanvasEl'); - assert.equal(lowerCanvas.hasAttribute('data-fabric'), false, 'dispose should clear lowerCanvasEl data-fabric attr'); - assert.equal(canvas.contextContainer, null, 'dispose should clear contextContainer'); - }); - - QUnit.test('dispose', async function (assert) { - const done = assert.async(); - const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); - assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); - await canvas.dispose(); - assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); - done(); - }); - - QUnit.test('dispose edge cases: multiple calls', async function (assert) { - const done = assert.async(); - const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); - assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); - const res = await Promise.all([ - canvas.dispose(), - canvas.dispose(), - canvas.dispose(), - ]); - assert.ok(canvas.disposed, 'should have flagged `disposed`'); - assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); - assert.deepEqual(res, [true, false, false], 'should have disposed in the first call'); - done(); - }); - - QUnit.test('dispose edge cases: multiple calls after `requestRenderAll', async function (assert) { - const done = assert.async(); - const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); - assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); - canvas.requestRenderAll(); - const res = await Promise.allSettled([ - canvas.dispose(), - canvas.dispose(), - canvas.dispose(), - ]); - assert.ok(canvas.disposed, 'should have flagged `disposed`'); - assert.ok(canvas.destroyed, 'should have flagged `destroyed`'); - assert.deepEqual(res, [ - { status: 'rejected', reason: 'aborted' }, - { status: 'rejected', reason: 'aborted' }, - { status: 'fulfilled', value: true } - ], 'should have disposed in the last call, aborting the other calls'); - done(); - }); - - QUnit.test.only('dispose edge cases: rendering after dispose', async function (assert) { - const done = assert.async(); - const canvas = new fabric.StaticCanvas(null, { renderOnAddRemove: false }); - let called = 0; - assert.ok(await canvas.dispose(), 'should dispose'); - canvas.on('after:render', () => { - called++; - }) - canvas.fire('after:render'); - assert.equal(canvas.nextRenderHandle, undefined); - canvas.requestRenderAll(); - assert.equal(canvas.nextRenderHandle, undefined, '`requestRenderAll` should have no affect'); - assert.equal(called, 1, 'should not have rendered'); - canvas.renderAll(); - assert.equal(called, 1, 'should not have rendered'); - done(); - }); - QUnit.test('clone', function(assert) { var done = assert.async(); var canvas2 = new fabric.StaticCanvas(null, { renderOnAddRemove: false, width: 10, height: 10 }); From 30c326d08d00940ce36db9064c19735a5f837dcb Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 22:31:04 +0300 Subject: [PATCH 11/24] Update canvas_dispose.js --- test/unit/canvas_dispose.js | 245 ++++++++++++++++++++---------------- 1 file changed, 140 insertions(+), 105 deletions(-) diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index cc0b19ba60e..7a7ab2eeae0 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -30,7 +30,7 @@ function assertCanvasDisposing(klass) { assert.equal(canvas.contextContainer, null, 'dispose should clear contextContainer'); }); - QUnit.test('dispose edge cases: multiple calls', async function (assert) { + QUnit.test('dispose edge case: multiple calls', async function (assert) { const done = assert.async(); const canvas = new klass(null, { renderOnAddRemove: false }); assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); @@ -45,7 +45,7 @@ function assertCanvasDisposing(klass) { done(); }); - QUnit.test('dispose edge cases: multiple calls after `requestRenderAll', async function (assert) { + QUnit.test('dispose edge case: multiple calls after `requestRenderAll', async function (assert) { const done = assert.async(); const canvas = new klass(null, { renderOnAddRemove: false }); assert.notOk(canvas.destroyed, 'should not have been destroyed yet'); @@ -65,7 +65,7 @@ function assertCanvasDisposing(klass) { done(); }); - QUnit.test('dispose edge cases: rendering after dispose', async function (assert) { + QUnit.test('dispose edge case: rendering after dispose', async function (assert) { const done = assert.async(); const canvas = new klass(null, { renderOnAddRemove: false }); let called = 0; @@ -82,109 +82,140 @@ function assertCanvasDisposing(klass) { assert.equal(called, 1, 'should not have rendered'); done(); }); + + QUnit.test('dispose edge case: `toCanvasElement` interrupting `requestRenderAll`', function (assert) { + const done = assert.async(); + const canvas = new klass(null, { renderOnAddRemove: false }); + assert.equal(canvas.nextRenderHandle, undefined); + canvas.nextRenderHandle = 1; + canvas.toCanvasElement(); + assert.equal(canvas.nextRenderHandle, 1, 'should request rendering'); + done(); + }); + + QUnit.test('dispose edge case: `toCanvasElement` after dispose', async function (assert) { + const done = assert.async(); + const canvas = new klass(null, { renderOnAddRemove: false }); + const testImageData = colorByteVal => { + return canvas + .toCanvasElement() + .getContext('2d') + .getImageData(0, 0, 20, 20) + .data + .filter((x, i) => i % 4 === 0) + .every(x => x === colorByteVal); + } + canvas.add(makeRect({ fill: 'red', width: 20, height: 20 })); + assert.ok(testImageData(255)); + assert.ok(await canvas.dispose()); + assert.ok(testImageData(0)); + done(); + }); +} + +function testStaticCanvasDisposing() { + QUnit.module('Static Canvas disposing') + assertCanvasDisposing(fabric.StaticCanvas); } -QUnit.module('StaticCanvas dispose') -assertCanvasDisposing(fabric.StaticCanvas); - - -QUnit.module('Canvas dispose') -assertCanvasDisposing(fabric.StaticCanvas); - -QUnit.test('dispose: clear refs', async function (assert) { - //made local vars to do not dispose the external canvas - var el = fabric.document.createElement('canvas'), - parentEl = fabric.document.createElement('div'), - wrapperEl, lowerCanvasEl, upperCanvasEl; - el.width = 200; el.height = 200; - parentEl.className = 'rootNode'; - parentEl.appendChild(el); - - fabric.config.configure({ devicePixelRatio: 1.25 }); - - assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); - assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); - - el.style.position = 'relative'; - var elStyle = el.style.cssText; - assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); - - var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); - wrapperEl = canvas.wrapperEl; - lowerCanvasEl = canvas.lowerCanvasEl; - upperCanvasEl = canvas.upperCanvasEl; - assert.equal(parentEl.childNodes.length, 1, 'parentEl has still 1 child only'); - assert.equal(wrapperEl.childNodes.length, 2, 'wrapper should have 2 children'); - assert.equal(wrapperEl.tagName, 'DIV', 'We wrapped canvas with DIV'); - assert.equal(wrapperEl.className, canvas.containerClass, 'DIV class should be set'); - assert.equal(wrapperEl.childNodes[0], lowerCanvasEl, 'First child should be lowerCanvas'); - assert.equal(wrapperEl.childNodes[1], upperCanvasEl, 'Second child should be upperCanvas'); - assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); - assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); - if (!fabric.isLikelyNode) { - assert.equal(parentEl.childNodes[0], wrapperEl, 'wrapperEl is appendend to rootNode'); - } - //looks like i cannot use parentNode - //equal(wrapperEl, lowerCanvasEl.parentNode, 'lowerCanvas is appended to wrapperEl'); - //equal(wrapperEl, upperCanvasEl.parentNode, 'upperCanvas is appended to wrapperEl'); - //equal(parentEl, wrapperEl.parentNode, 'wrapperEl is appendend to rootNode'); - assert.equal(parentEl.childNodes.length, 1, 'parent div should have 1 child'); - assert.notEqual(parentEl.firstChild, canvas.getElement(), 'canvas should not be parent div firstChild'); - assert.ok(typeof canvas.dispose === 'function'); - assert.ok(typeof canvas.destroy === 'function'); - canvas.add(makeRect(), makeRect(), makeRect()); - canvas.item(0).animate('scaleX', 10); - assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation'); - await canvas.dispose(); - assert.equal(fabric.runningAnimations.length, 0, 'dispose should clear running animations'); - assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); - assert.equal(parentEl.childNodes.length, 1, 'parent has always 1 child'); - if (!fabric.isLikelyNode) { - assert.equal(parentEl.childNodes[0], lowerCanvasEl, 'canvas should be back to its firstChild place'); - } - assert.equal(canvas.wrapperEl, null, 'wrapperEl should be deleted'); - assert.equal(canvas.upperCanvasEl, null, 'upperCanvas should be deleted'); - assert.equal(canvas.lowerCanvasEl, null, 'lowerCanvasEl should be deleted'); - assert.equal(canvas.cacheCanvasEl, null, 'cacheCanvasEl should be deleted'); - assert.equal(canvas.contextTop, null, 'contextTop should be deleted'); - assert.equal(canvas.contextCache, null, 'contextCache should be deleted'); - assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); - assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); - assert.equal(el.width, 200, 'restored width'); - assert.equal(el.height, 200, 'restored height'); - -}); - -QUnit.test('dispose edge case: `setDimensions` invoking `requestRenderAll`', async function (assert) { - //made local vars to do not dispose the external canvas - var el = fabric.document.createElement('canvas'), - parentEl = fabric.document.createElement('div'); - el.width = 200; el.height = 200; - parentEl.className = 'rootNode'; - parentEl.appendChild(el); - - fabric.config.configure({ devicePixelRatio: 1.25 }); - - assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); - assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); - - el.style.position = 'relative'; - var elStyle = el.style.cssText; - assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); - - var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); - - canvas.setDimensions({ width: 500, height: 500 }); - assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); - assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); - - await canvas.dispose(); - assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); - assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); - assert.equal(el.width, 500, 'restored width'); - assert.equal(el.height, 500, 'restored height'); - -}); +function testCanvasDisposing() { + QUnit.module('Canvas disposing') + assertCanvasDisposing(fabric.Canvas); + + QUnit.test('dispose: clear refs', async function (assert) { + //made local vars to do not dispose the external canvas + var el = fabric.document.createElement('canvas'), + parentEl = fabric.document.createElement('div'), + wrapperEl, lowerCanvasEl, upperCanvasEl; + el.width = 200; el.height = 200; + parentEl.className = 'rootNode'; + parentEl.appendChild(el); + + fabric.config.configure({ devicePixelRatio: 1.25 }); + + assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); + assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); + + el.style.position = 'relative'; + var elStyle = el.style.cssText; + assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); + + var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); + wrapperEl = canvas.wrapperEl; + lowerCanvasEl = canvas.lowerCanvasEl; + upperCanvasEl = canvas.upperCanvasEl; + assert.equal(parentEl.childNodes.length, 1, 'parentEl has still 1 child only'); + assert.equal(wrapperEl.childNodes.length, 2, 'wrapper should have 2 children'); + assert.equal(wrapperEl.tagName, 'DIV', 'We wrapped canvas with DIV'); + assert.equal(wrapperEl.className, canvas.containerClass, 'DIV class should be set'); + assert.equal(wrapperEl.childNodes[0], lowerCanvasEl, 'First child should be lowerCanvas'); + assert.equal(wrapperEl.childNodes[1], upperCanvasEl, 'Second child should be upperCanvas'); + assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); + assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); + if (!fabric.isLikelyNode) { + assert.equal(parentEl.childNodes[0], wrapperEl, 'wrapperEl is appendend to rootNode'); + } + //looks like i cannot use parentNode + //equal(wrapperEl, lowerCanvasEl.parentNode, 'lowerCanvas is appended to wrapperEl'); + //equal(wrapperEl, upperCanvasEl.parentNode, 'upperCanvas is appended to wrapperEl'); + //equal(parentEl, wrapperEl.parentNode, 'wrapperEl is appendend to rootNode'); + assert.equal(parentEl.childNodes.length, 1, 'parent div should have 1 child'); + assert.notEqual(parentEl.firstChild, canvas.getElement(), 'canvas should not be parent div firstChild'); + assert.ok(typeof canvas.dispose === 'function'); + assert.ok(typeof canvas.destroy === 'function'); + canvas.add(makeRect(), makeRect(), makeRect()); + canvas.item(0).animate('scaleX', 10); + assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation'); + await canvas.dispose(); + assert.equal(fabric.runningAnimations.length, 0, 'dispose should clear running animations'); + assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); + assert.equal(parentEl.childNodes.length, 1, 'parent has always 1 child'); + if (!fabric.isLikelyNode) { + assert.equal(parentEl.childNodes[0], lowerCanvasEl, 'canvas should be back to its firstChild place'); + } + assert.equal(canvas.wrapperEl, null, 'wrapperEl should be deleted'); + assert.equal(canvas.upperCanvasEl, null, 'upperCanvas should be deleted'); + assert.equal(canvas.lowerCanvasEl, null, 'lowerCanvasEl should be deleted'); + assert.equal(canvas.cacheCanvasEl, null, 'cacheCanvasEl should be deleted'); + assert.equal(canvas.contextTop, null, 'contextTop should be deleted'); + assert.equal(canvas.contextCache, null, 'contextCache should be deleted'); + assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); + assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); + assert.equal(el.width, 200, 'restored width'); + assert.equal(el.height, 200, 'restored height'); + + }); + + QUnit.test('dispose edge case: `setDimensions` invoking `requestRenderAll`', async function (assert) { + //made local vars to do not dispose the external canvas + var el = fabric.document.createElement('canvas'), + parentEl = fabric.document.createElement('div'); + el.width = 200; el.height = 200; + parentEl.className = 'rootNode'; + parentEl.appendChild(el); + + fabric.config.configure({ devicePixelRatio: 1.25 }); + + assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); + assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); + + el.style.position = 'relative'; + var elStyle = el.style.cssText; + assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); + + var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); + + canvas.setDimensions({ width: 500, height: 500 }); + assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); + assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); + + await canvas.dispose(); + assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); + assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); + assert.equal(el.width, 500, 'restored width'); + assert.equal(el.height, 500, 'restored height'); + + }); // QUnit.test('dispose', async function(assert) { // function invokeEventsOnCanvas() { @@ -226,4 +257,8 @@ QUnit.test('dispose edge case: `setDimensions` invoking `requestRenderAll`', asy // invokeEventsOnCanvas(); // assertInvocationsCount(); - // }); \ No newline at end of file + // }); +} + +testStaticCanvasDisposing(); +testCanvasDisposing(); \ No newline at end of file From 4be0daa75cf6c47691b713519466b45b0cd3fce2 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 22:51:06 +0300 Subject: [PATCH 12/24] fix: test cleanup --- test/unit/canvas_dispose.js | 103 +++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 44 deletions(-) diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index 7a7ab2eeae0..3e8a498b5cc 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -114,12 +114,27 @@ function assertCanvasDisposing(klass) { } function testStaticCanvasDisposing() { - QUnit.module('Static Canvas disposing') + QUnit.module('Static Canvas disposing', { + beforeEach: function () { + + }, + afterEach: function () { + fabric.config.restoreDefaults(); + } + }) assertCanvasDisposing(fabric.StaticCanvas); } function testCanvasDisposing() { - QUnit.module('Canvas disposing') + QUnit.module('Canvas disposing', { + beforeEach: function () { + + }, + afterEach: function () { + fabric.config.restoreDefaults(); + } + }); + assertCanvasDisposing(fabric.Canvas); QUnit.test('dispose: clear refs', async function (assert) { @@ -217,48 +232,48 @@ function testCanvasDisposing() { }); - // QUnit.test('dispose', async function(assert) { - // function invokeEventsOnCanvas() { - // // nextSibling because we need to invoke events on upper canvas - // simulateEvent(canvas.getElement().nextSibling, 'mousedown'); - // simulateEvent(canvas.getElement().nextSibling, 'mouseup'); - // simulateEvent(canvas.getElement().nextSibling, 'mousemove'); - // } - // var assertInvocationsCount = function() { - // var message = 'event handler should not be invoked after `dispose`'; - // assert.equal(handlerInvocationCounts.__onMouseDown, 1); - // assert.equal(handlerInvocationCounts.__onMouseUp, 1); - // assert.equal(handlerInvocationCounts.__onMouseMove, 1); - // }; - - // assert.ok(typeof canvas.dispose === 'function'); - // canvas.add(makeRect(), makeRect(), makeRect()); - - // var handlerInvocationCounts = { - // __onMouseDown: 0, __onMouseUp: 0, __onMouseMove: 0 - // }; - - // // hijack event handlers - // canvas.__onMouseDown = function() { - // handlerInvocationCounts.__onMouseDown++; - // }; - // canvas.__onMouseUp = function() { - // handlerInvocationCounts.__onMouseUp++; - // }; - // canvas.__onMouseMove = function() { - // handlerInvocationCounts.__onMouseMove++; - // }; - - // invokeEventsOnCanvas(); - // assertInvocationsCount(); - - // await canvas.dispose(); - // assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); - - // invokeEventsOnCanvas(); - // assertInvocationsCount(); - // }); + // QUnit.test('dispose: events', async function(assert) { + // function invokeEventsOnCanvas() { + // // nextSibling because we need to invoke events on upper canvas + // simulateEvent(canvas.getElement().nextSibling, 'mousedown'); + // simulateEvent(canvas.getElement().nextSibling, 'mouseup'); + // simulateEvent(canvas.getElement().nextSibling, 'mousemove'); + // } + // var assertInvocationsCount = function() { + // var message = 'event handler should not be invoked after `dispose`'; + // assert.equal(handlerInvocationCounts.__onMouseDown, 1); + // assert.equal(handlerInvocationCounts.__onMouseUp, 1); + // assert.equal(handlerInvocationCounts.__onMouseMove, 1); + // }; + + // assert.ok(typeof canvas.dispose === 'function'); + // canvas.add(makeRect(), makeRect(), makeRect()); + + // var handlerInvocationCounts = { + // __onMouseDown: 0, __onMouseUp: 0, __onMouseMove: 0 + // }; + + // // hijack event handlers + // canvas.__onMouseDown = function() { + // handlerInvocationCounts.__onMouseDown++; + // }; + // canvas.__onMouseUp = function() { + // handlerInvocationCounts.__onMouseUp++; + // }; + // canvas.__onMouseMove = function() { + // handlerInvocationCounts.__onMouseMove++; + // }; + + // invokeEventsOnCanvas(); + // assertInvocationsCount(); + + // await canvas.dispose(); + // assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas'); + + // invokeEventsOnCanvas(); + // assertInvocationsCount(); + // }); } testStaticCanvasDisposing(); -testCanvasDisposing(); \ No newline at end of file +testCanvasDisposing(); From 335bd391d0a050a486f2e683fd737f37b8c2d8bd Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 22:58:15 +0300 Subject: [PATCH 13/24] Update canvas_dispose.js --- test/unit/canvas_dispose.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index 3e8a498b5cc..1ef69be6d66 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -201,8 +201,7 @@ function testCanvasDisposing() { }); - QUnit.test('dispose edge case: `setDimensions` invoking `requestRenderAll`', async function (assert) { - //made local vars to do not dispose the external canvas + QUnit.test('dispose: `setDimensions`', async function (assert) { var el = fabric.document.createElement('canvas'), parentEl = fabric.document.createElement('div'); el.width = 200; el.height = 200; From 1eafc15055ce0a53af24d7cc17a214301a9898f9 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 23:02:45 +0300 Subject: [PATCH 14/24] cosmetics --- src/static_canvas.class.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index aebf0f3398f..02d696730b6 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -783,6 +783,7 @@ import { pick } from './util/misc/pick'; this.drawControls(ctx); } this.fire('after:render', { ctx: ctx, }); + if (this.__cleanupTask) { this.__cleanupTask(); this.__cleanupTask = undefined; @@ -1619,7 +1620,7 @@ import { pick } from './util/misc/pick'; /** * Waits until rendering has settled to destroy the canvas - * @returns {Promise} a promise resolving to true once the canvas has been destroyed or to `false` if the canvas has was already destroyed + * @returns {Promise} a promise resolving to `true` once the canvas has been destroyed or to `false` if the canvas has was already destroyed * @throws if aborted by a successive call */ dispose: function () { From a97566e159910fb4f8b6b6629d906beb753b857e Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Tue, 30 Aug 2022 23:08:45 +0300 Subject: [PATCH 15/24] english --- src/static_canvas.class.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index 02d696730b6..141b8757851 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -783,7 +783,7 @@ import { pick } from './util/misc/pick'; this.drawControls(ctx); } this.fire('after:render', { ctx: ctx, }); - + if (this.__cleanupTask) { this.__cleanupTask(); this.__cleanupTask = undefined; @@ -1621,7 +1621,7 @@ import { pick } from './util/misc/pick'; /** * Waits until rendering has settled to destroy the canvas * @returns {Promise} a promise resolving to `true` once the canvas has been destroyed or to `false` if the canvas has was already destroyed - * @throws if aborted by a successive call + * @throws if aborted by a consequent call */ dispose: function () { this.disposed = true; From 109b40e0cb99e427a8e6131b03177ad22a0639d5 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Wed, 31 Aug 2022 01:11:13 +0300 Subject: [PATCH 16/24] move back where it belongs --- test/unit/canvas.js | 30 ++++++++++++++++++++++++++++++ test/unit/canvas_dispose.js | 30 ------------------------------ 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/test/unit/canvas.js b/test/unit/canvas.js index 4d0142d3c4f..406b0347819 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -2083,6 +2083,36 @@ assert.equal(aGroup._objects[3], circle2); }); + QUnit.test('set dimensions', async function (assert) { + var el = fabric.document.createElement('canvas'), + parentEl = fabric.document.createElement('div'); + el.width = 200; el.height = 200; + parentEl.className = 'rootNode'; + parentEl.appendChild(el); + + fabric.config.configure({ devicePixelRatio: 1.25 }); + + assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); + assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); + + el.style.position = 'relative'; + var elStyle = el.style.cssText; + assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); + + var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); + + canvas.setDimensions({ width: 500, height: 500 }); + assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); + assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); + + await canvas.dispose(); + assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); + assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); + assert.equal(el.width, 500, 'restored width'); + assert.equal(el.height, 500, 'restored height'); + + }); + QUnit.test('clone', function(assert) { var done = assert.async(); assert.ok(typeof canvas.clone === 'function'); diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index 1ef69be6d66..418aa2be3d9 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -201,36 +201,6 @@ function testCanvasDisposing() { }); - QUnit.test('dispose: `setDimensions`', async function (assert) { - var el = fabric.document.createElement('canvas'), - parentEl = fabric.document.createElement('div'); - el.width = 200; el.height = 200; - parentEl.className = 'rootNode'; - parentEl.appendChild(el); - - fabric.config.configure({ devicePixelRatio: 1.25 }); - - assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl'); - assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only'); - - el.style.position = 'relative'; - var elStyle = el.style.cssText; - assert.equal(elStyle, 'position: relative;', 'el style should not be empty'); - - var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false }); - - canvas.setDimensions({ width: 500, height: 500 }); - assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal'); - assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed'); - - await canvas.dispose(); - assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style'); - assert.equal(el.style.cssText, elStyle, 'restored original canvas style'); - assert.equal(el.width, 500, 'restored width'); - assert.equal(el.height, 500, 'restored height'); - - }); - // QUnit.test('dispose: events', async function(assert) { // function invokeEventsOnCanvas() { // // nextSibling because we need to invoke events on upper canvas From 7a276914cfe566f3ee2c03505d1bb0ea76bbea02 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Wed, 31 Aug 2022 01:13:49 +0300 Subject: [PATCH 17/24] Update canvas_dispose.js --- test/unit/canvas_dispose.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index 418aa2be3d9..e8ee0a5ab5f 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -106,9 +106,11 @@ function assertCanvasDisposing(klass) { .every(x => x === colorByteVal); } canvas.add(makeRect({ fill: 'red', width: 20, height: 20 })); - assert.ok(testImageData(255)); - assert.ok(await canvas.dispose()); - assert.ok(testImageData(0)); + assert.ok(testImageData(255), 'control'); + canvas.disposed = true; + assert.ok(testImageData(255), 'should render canvas'); + canvas.destroyed = true; + assert.ok(testImageData(0), 'should have disabled canvas rendering'); done(); }); } From 18664f648254a063c418caf8dab63292d779ffa2 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Wed, 31 Aug 2022 01:15:57 +0300 Subject: [PATCH 18/24] dispose --- test/unit/canvas_dispose.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index e8ee0a5ab5f..e4ff8ecc3c3 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -111,6 +111,8 @@ function assertCanvasDisposing(klass) { assert.ok(testImageData(255), 'should render canvas'); canvas.destroyed = true; assert.ok(testImageData(0), 'should have disabled canvas rendering'); + canvas.destroyed = false; + assert.ok(await canvas.dispose(), 'dispose'); done(); }); } From 0ea2761d22bbc7668f042f44ba33c28c2c16a3b5 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Sun, 4 Sep 2022 06:55:05 +0300 Subject: [PATCH 19/24] fix(): `cancelRequestedRender` from `renderAll` handles the case in which there is a requested render and the dev calls `renderAll` --- src/canvas.class.ts | 4 ++-- src/static_canvas.class.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/canvas.class.ts b/src/canvas.class.ts index 1f2a63da9bf..1bb2a82d4e3 100644 --- a/src/canvas.class.ts +++ b/src/canvas.class.ts @@ -511,6 +511,7 @@ import { Point } from './point.class'; * @chainable */ renderAll: function () { + this.cancelRequestedRender(); if (this.contextTopDirty && !this._groupSelector && !this.isDrawingMode) { this.clearContext(this.contextTop); this.contextTopDirty = false; @@ -519,9 +520,8 @@ import { Point } from './point.class'; this.renderTopLayer(this.contextTop); this.hasLostContext = false; } - var canvasToDrawOn = this.contextContainer; !this._objectsToRender && (this._objectsToRender = this._chooseObjectsToRender()); - this.renderCanvas(canvasToDrawOn, this._objectsToRender); + this.renderCanvas(this.contextContainer, this._objectsToRender); return this; }, diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index 08a5efe8db3..97c5f25a1f5 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -675,8 +675,8 @@ import { pick } from './util/misc/pick'; * @chainable */ renderAll: function () { - var canvasToDrawOn = this.contextContainer; - this.renderCanvas(canvasToDrawOn, this._objects); + this.cancelRequestedRender(); + this.renderCanvas(this.contextContainer, this._objects); return this; }, From 118c86132a5763583b8f5e7b42c5c522725f6edb Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Sun, 4 Sep 2022 07:01:43 +0300 Subject: [PATCH 20/24] Update canvas_dispose.js --- test/unit/canvas_dispose.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index e4ff8ecc3c3..466cc12cdad 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -74,12 +74,12 @@ function assertCanvasDisposing(klass) { called++; }) canvas.fire('after:render'); + assert.equal(called, 1, 'should have fired'); assert.equal(canvas.nextRenderHandle, undefined); canvas.requestRenderAll(); assert.equal(canvas.nextRenderHandle, undefined, '`requestRenderAll` should have no affect'); - assert.equal(called, 1, 'should not have rendered'); canvas.renderAll(); - assert.equal(called, 1, 'should not have rendered'); + assert.equal(called, 1, 'should not have rendered, should still equal 1'); done(); }); From 21246b263196c1e76e835147167185686a1e8fdc Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Sun, 4 Sep 2022 07:31:05 +0300 Subject: [PATCH 21/24] fix(): animate edge case --- src/canvas.class.ts | 3 +++ src/static_canvas.class.ts | 3 +++ test/unit/canvas_dispose.js | 25 +++++++++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/src/canvas.class.ts b/src/canvas.class.ts index 1bb2a82d4e3..aebc9ff812b 100644 --- a/src/canvas.class.ts +++ b/src/canvas.class.ts @@ -512,6 +512,9 @@ import { Point } from './point.class'; */ renderAll: function () { this.cancelRequestedRender(); + if (this.destroyed) { + return; + } if (this.contextTopDirty && !this._groupSelector && !this.isDrawingMode) { this.clearContext(this.contextTop); this.contextTopDirty = false; diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index 97c5f25a1f5..366776b5063 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -676,6 +676,9 @@ import { pick } from './util/misc/pick'; */ renderAll: function () { this.cancelRequestedRender(); + if (this.destroyed) { + return; + } this.renderCanvas(this.contextContainer, this._objects); return this; }, diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index 466cc12cdad..073d3a96fd1 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -115,6 +115,31 @@ function assertCanvasDisposing(klass) { assert.ok(await canvas.dispose(), 'dispose'); done(); }); + + QUnit.test('dispose edge case: animate', function (assert) { + const done = assert.async(); + const canvas = new klass(null, { renderOnAddRemove: false }); + let called = 0; + const animate = () => fabric.util.animate({ + onChange() { + if (called === 50) { + canvas.dispose().then(() => { + fabric.runningAnimations.cancelAll(); + done(); + }); + assert.ok(canvas.disposed, 'should flag `disposed`'); + } + called++; + canvas.contextTopDirty = true; + canvas.hasLostContext = true; + canvas.renderAll(); + }, + onComplete() { + animate(); + } + }); + animate(); + }); } function testStaticCanvasDisposing() { From 23420dae583380a19be1acdcf77f7a58b7185627 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Sun, 4 Sep 2022 07:33:26 +0300 Subject: [PATCH 22/24] Update canvas_dispose.js --- test/unit/canvas_dispose.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index 073d3a96fd1..62546f3c88d 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -122,7 +122,7 @@ function assertCanvasDisposing(klass) { let called = 0; const animate = () => fabric.util.animate({ onChange() { - if (called === 50) { + if (called === 1) { canvas.dispose().then(() => { fabric.runningAnimations.cancelAll(); done(); From 54bde04bd026685dd680e855bbd5d877b1df11b4 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Sun, 4 Sep 2022 07:37:23 +0300 Subject: [PATCH 23/24] Update static_canvas.class.ts --- src/static_canvas.class.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/static_canvas.class.ts b/src/static_canvas.class.ts index 366776b5063..1850feabf65 100644 --- a/src/static_canvas.class.ts +++ b/src/static_canvas.class.ts @@ -2,6 +2,7 @@ import { config } from './config'; import { VERSION } from './constants'; import { Point } from './point.class'; +import { requestAnimFrame } from './util/animate'; import { removeFromArray } from './util/internals'; import { pick } from './util/misc/pick'; @@ -707,7 +708,7 @@ import { pick } from './util/misc/pick'; */ requestRenderAll: function () { if (!this.nextRenderHandle && !this.disposed && !this.destroyed) { - this.nextRenderHandle = fabric.util.requestAnimFrame(this.renderAndResetBound); + this.nextRenderHandle = requestAnimFrame(this.renderAndResetBound); } return this; }, From f961dca3900828e6545cb0e359e7c5da707c77c7 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Sun, 4 Sep 2022 07:40:18 +0300 Subject: [PATCH 24/24] rename test --- test/unit/canvas_dispose.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/canvas_dispose.js b/test/unit/canvas_dispose.js index 62546f3c88d..84b9c3822e4 100644 --- a/test/unit/canvas_dispose.js +++ b/test/unit/canvas_dispose.js @@ -116,7 +116,7 @@ function assertCanvasDisposing(klass) { done(); }); - QUnit.test('dispose edge case: animate', function (assert) { + QUnit.test('dispose edge case: during animation', function (assert) { const done = assert.async(); const canvas = new klass(null, { renderOnAddRemove: false }); let called = 0;