fix(Canvas): safeguard dispose (#7775)

Fix bug introduced by restoring canvas element styles improperly
fabricjs · Apr 1, 2022 · 6b64132 · 6b64132
1 parent 41c411d
commit 6b64132
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 5 deletions.
diff --git a/src/static_canvas.class.js b/src/static_canvas.class.js
@@ -320,8 +320,8 @@
       }
       fabric.util.addClass(this.lowerCanvasEl, 'lower-canvas');
       this.lowerCanvasEl.setAttribute('data-fabric', 'main');
-      this._originalCanvasStyle = this.lowerCanvasEl.style;
       if (this.interactive) {
+        this._originalCanvasStyle = this.lowerCanvasEl.style.cssText;
         this._applyCanvasStyle(this.lowerCanvasEl);
       }
 
@@ -1574,11 +1574,13 @@
       this.overlayImage = null;
       this._iTextInstances = null;
       this.contextContainer = null;
-      // restore canvas style
+      // restore canvas style and attributes
       this.lowerCanvasEl.classList.remove('lower-canvas');
       this.lowerCanvasEl.removeAttribute('data-fabric');
-      fabric.util.setStyle(this.lowerCanvasEl, this._originalCanvasStyle);
-      delete this._originalCanvasStyle;
+      if (this.interactive) {
+        this.lowerCanvasEl.style.cssText = this._originalCanvasStyle;
+        delete this._originalCanvasStyle;
+      }
       // restore canvas size to original size in case retina scaling was applied
       this.lowerCanvasEl.setAttribute('width', this.width);
       this.lowerCanvasEl.setAttribute('height', this.height);

diff --git a/test/unit/canvas.js b/test/unit/canvas.js
@@ -2072,10 +2072,17 @@
     parentEl.className = 'rootNode';
     parentEl.appendChild(el);
 
+    var originalDevicePixelRatio = fabric.devicePixelRatio;
+    fabric.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');
 
-    var canvas = new fabric.Canvas(el, {enableRetinaScaling: false, renderOnAddRemove: false });
+    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;
@@ -2085,6 +2092,8 @@
     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');
     }
@@ -2108,9 +2117,57 @@
     }
     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');
+
+    fabric.devicePixelRatio = originalDevicePixelRatio;
+  });
+
+  QUnit.test('dispose + set dimensions', function (assert) {
+    var done = assert.async();
+    //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);
+
+    var originalDevicePixelRatio = fabric.devicePixelRatio;
+    fabric.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 });
+
+    //  prevent a race condition
+    //  setDimensions requests rendering while disposing which throws an error
+    canvas.on('after:render', () => {
+      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();
+      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');
+
+      fabric.devicePixelRatio = originalDevicePixelRatio;
+      done();
+    });
+
+    canvas.setDimensions({ width: 500, height: 500 });
+
   });
 
   // QUnit.test('dispose', function(assert) {