Skip to content

Commit

Permalink
fix(fabric.Canvas): dispose and request animation frame scheduling fix (
Browse files Browse the repository at this point in the history 
#8220)

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
  • Loading branch information
@ShaMan123 @asturur
ShaMan123 and asturur authored Sep 11, 2022
1 parent c469b7b commit eec52ef
Show file tree
Hide file tree
Showing 8 changed files with 374 additions and 160 deletions.
24 changes: 17 additions & 7 deletions src/canvas.class.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,10 @@ import { Point } from './point.class';
* @chainable
*/
renderAll: function () {
this.cancelRequestedRender();
if (this.destroyed) {
return;
}
if (this.contextTopDirty && !this._groupSelector && !this.isDrawingMode) {
this.clearContext(this.contextTop);
this.contextTopDirty = false;
Expand All @@ -519,9 +523,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;
},

Expand Down Expand Up @@ -1313,17 +1316,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;
Expand Down
3 changes: 2 additions & 1 deletion src/shapes/object.class.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1738,7 +1738,8 @@ import { runningAnimations } from '../util/animation_registry';
// 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;
Expand Down
84 changes: 65 additions & 19 deletions src/static_canvas.class.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -675,8 +676,11 @@ import { pick } from './util/misc/pick';
* @chainable
*/
renderAll: function () {
var canvasToDrawOn = this.contextContainer;
this.renderCanvas(canvasToDrawOn, this._objects);
this.cancelRequestedRender();
if (this.destroyed) {
return;
}
this.renderCanvas(this.contextContainer, this._objects);
return this;
},

Expand All @@ -691,7 +695,7 @@ import { pick } from './util/misc/pick';
* @chainable
*/
renderAndReset: function() {
this.isRendering = 0;
this.nextRenderHandle = 0;
this.renderAll();
},

Expand All @@ -703,8 +707,8 @@ import { pick } from './util/misc/pick';
* @chainable
*/
requestRenderAll: function () {
if (!this.isRendering) {
this.isRendering = fabric.util.requestAnimFrame(this.renderAndResetBound);
if (!this.nextRenderHandle && !this.disposed && !this.destroyed) {
this.nextRenderHandle = requestAnimFrame(this.renderAndResetBound);
}
return this;
},
Expand Down Expand Up @@ -734,9 +738,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;
}
},

Expand All @@ -747,12 +751,17 @@ 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();
this.clearContext(ctx);
ctx.imageSmoothingEnabled = this.imageSmoothingEnabled;
// node-canvas
ctx.patternQuality = 'best';
this.fire('before:render', { ctx: ctx, });
this._renderBackground(ctx);
Expand All @@ -778,6 +787,11 @@ import { pick } from './util/misc/pick';
this.drawControls(ctx);
}
this.fire('after:render', { ctx: ctx, });

if (this.__cleanupTask) {
this.__cleanupTask();
this.__cleanupTask = undefined;
}
},

/**
Expand Down Expand Up @@ -1609,16 +1623,49 @@ 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<boolean>} 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 consequent call
*/
dispose: function () {
// cancel eventually ongoing renders
if (this.isRendering) {
fabric.util.cancelAnimFrame(this.isRendering);
this.isRendering = 0;
}
this.disposed = true;
return new Promise<boolean>((resolve, reject) => {
const task = () => {
this.destroy();
resolve(true);
}
task.kill = reject;
if (this.__cleanupTask) {
this.__cleanupTask.kill('aborted');
}

if (this.destroyed) {
resolve(false);
}
else 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();
});
Expand All @@ -1645,7 +1692,6 @@ import { pick } from './util/misc/pick';
this.lowerCanvasEl.setAttribute('height', this.height);
fabric.util.cleanUpJsdomNode(this.lowerCanvasEl);
this.lowerCanvasEl = undefined;
return this;
},

/**
Expand Down
2 changes: 1 addition & 1 deletion test/lib/visualTestLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@
if ((!isOK && QUnit.debugVisual) || QUnit.recreateVisualRefs) {
generateGolden(getGoldeName(golden), renderedCanvas);
}
fabricCanvas.dispose();
await fabricCanvas.dispose();
done();
});
});
Expand Down
111 changes: 2 additions & 109 deletions test/unit/canvas.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2083,72 +2083,7 @@
assert.equal(aGroup._objects[3], circle2);
});

QUnit.test('dispose', 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');
canvas.add(makeRect(), makeRect(), makeRect());
canvas.item(0).animate('scaleX', 10);
assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation');
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');
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 + set dimensions', function (assert) {
//made local vars to do not dispose the external canvas
QUnit.test('set dimensions', async function (assert) {
var el = fabric.document.createElement('canvas'),
parentEl = fabric.document.createElement('div');
el.width = 200; el.height = 200;
Expand All @@ -2170,56 +2105,14 @@
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');
assert.equal(el.height, 500, 'restored height');

});

// QUnit.test('dispose', 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();

// 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');
Expand Down
Loading

0 comments on commit eec52ef

Please sign in to comment.