Fix interaction between grouping and enter edit for text (#5153)

fabricjs · Aug 12, 2018 · 8c3f677 · 8c3f677
1 parent 4c2f91d
commit 8c3f677
Show file tree

Hide file tree

Showing 8 changed files with 98 additions and 10 deletions.
diff --git a/package.json b/package.json
@@ -45,6 +45,7 @@
     "build:watch": "onchange 'src/**/**' 'HEADER.js' 'lib/**/**' -- npm run build_export",
     "build_with_gestures": "node build.js modules=ALL exclude=accessors",
     "build_export": "npm run build:fast && npm run export_dist_to_site",
+    "test:single": "./node_modules/qunit/bin/qunit test/node_test_setup.js test/lib",
     "test": "istanbul cover ./node_modules/qunit/bin/qunit test/node_test_setup.js test/lib test/unit",
     "test:visual": "./node_modules/qunit/bin/qunit test/node_test_setup.js test/lib test/visual",
     "test:all": "npm run test && npm run test:visual",

diff --git a/src/mixins/itext_click_behavior.mixin.js b/src/mixins/itext_click_behavior.mixin.js
@@ -133,13 +133,15 @@ fabric.util.object.extend(fabric.IText.prototype, /** @lends fabric.IText.protot
    */
   mouseUpHandler: function(options) {
     this.__isMousedown = false;
-    if (!this.editable ||
+    if (!this.editable || this.group ||
       (options.transform && options.transform.actionPerformed) ||
       (options.e.button && options.e.button !== 1)) {
       return;
     }
 
     if (this.__lastSelected && !this.__corner) {
+      this.selected = false;
+      this.__lastSelected = false;
       this.enterEditing(options.e);
       if (this.selectionStart === this.selectionEnd) {
         this.initDelayedCursor(true);
@@ -148,7 +150,9 @@ fabric.util.object.extend(fabric.IText.prototype, /** @lends fabric.IText.protot
         this.renderCursorOrSelection();
       }
     }
-    this.selected = true;
+    else {
+      this.selected = true;
+    }
   },
 
   /**

diff --git a/src/static_canvas.class.js b/src/static_canvas.class.js
@@ -838,6 +838,10 @@
     /**
      * Function created to be instance bound at initialization
      * used in requestAnimationFrame rendering
+     * Let the fabricJS call it. If you call it manually you could have more
+     * animationFrame stacking on to of each other
+     * for an imperative rendering, use canvas.renderAll
+     * @private
      * @return {fabric.Canvas} instance
      * @chainable
      */
@@ -848,6 +852,7 @@
 
     /**
      * Append a renderAll request to next animation frame.
+     * unless one is already in progress, in that case nothing is done
      * a boolean flag will avoid appending more.
      * @return {fabric.Canvas} instance
      * @chainable
@@ -877,6 +882,13 @@
       return points;
     },
 
+    cancelRequestedRender: function() {
+      if (this.isRendering) {
+        fabric.util.cancelAnimFrame(this.isRendering);
+        this.isRendering = 0;
+      }
+    },
+
     /**
      * Renders background, objects, overlay and controls.
      * @param {CanvasRenderingContext2D} ctx
@@ -886,10 +898,7 @@
      */
     renderCanvas: function(ctx, objects) {
       var v = this.viewportTransform;
-      if (this.isRendering) {
-        fabric.util.cancelAnimFrame(this.isRendering);
-        this.isRendering = 0;
-      }
+      this.cancelRequestedRender();
       this.calcViewportBoundaries();
       this.clearContext(ctx);
       this.fire('before:render');

diff --git a/test/unit/brushes.js b/test/unit/brushes.js
@@ -1,6 +1,10 @@
 (function() {
   var canvas = new fabric.Canvas();
-  QUnit.module('fabric.BaseBrush');
+  QUnit.module('fabric.BaseBrush', {
+    afterEach: function() {
+      canvas.cancelRequestedRender();
+    }
+  });
   QUnit.test('fabric brush constructor', function(assert) {
     assert.ok(fabric.BaseBrush);
 

diff --git a/test/unit/canvas_events.js b/test/unit/canvas_events.js
@@ -5,6 +5,7 @@
 
   QUnit.module('fabric.Canvas events mixin', {
     beforeEach: function() {
+      canvas.cancelRequestedRender();
       upperCanvasEl.style.display = '';
       canvas.controlsAboveOverlay = fabric.Canvas.prototype.controlsAboveOverlay;
       canvas.preserveObjectStacking = fabric.Canvas.prototype.preserveObjectStacking;
@@ -17,6 +18,7 @@
       canvas.off();
       canvas.calcOffset();
       upperCanvasEl.style.display = 'none';
+      canvas.cancelRequestedRender();
     }
   });
 
@@ -432,6 +434,7 @@
       fabric.document.dispatchEvent(event);
       assert.equal(counter, 1, 'listener executed once');
       fabric.Canvas.prototype._onMouseUp = originalMouseUp;
+      c.cancelRequestedRender();
       done();
     }, 200);
   });

diff --git a/test/unit/canvas_static.js b/test/unit/canvas_static.js
@@ -1771,6 +1771,15 @@
     assert.equal(svg, expectedSVG, 'svg is as expected');
   });
 
+  QUnit.test('requestRenderAll and cancelRequestedRender', function(assert) {
+    var canvas2 = new fabric.StaticCanvas();
+    assert.equal(canvas2.isRendering, undefined, 'no redering is in progress');
+    canvas2.requestRenderAll();
+    assert.notEqual(canvas2.isRendering, 0, 'a rendering is scehduled');
+    canvas2.cancelRequestedRender();
+    assert.equal(canvas2.isRendering, 0, 'rendering cancelled');
+  });
+
   // QUnit.test('backgroundImage', function(assert) {
   //   var done = assert.async();
   //   assert.deepEqual('', canvas.backgroundImage);

diff --git a/test/unit/itext.js b/test/unit/itext.js
@@ -4,6 +4,7 @@
   QUnit.module('fabric.IText', {
     afterEach: function() {
       canvas.clear();
+      canvas.cancelRequestedRender();
     }
   });
 

diff --git a/test/unit/itext_click_behaviour.js b/test/unit/itext_click_behaviour.js
@@ -1,4 +1,12 @@
 (function(){
+
+  var canvas = new fabric.Canvas();
+
+  QUnit.module('iText click interaction', {
+    afterEach: function() {
+      canvas.cancelRequestedRender();
+    }
+  });
   QUnit.test('_getNewSelectionStartFromOffset end of line', function(assert) {
     var iText = new fabric.IText('test need some word\nsecond line');
     var index = 10;
@@ -30,10 +38,59 @@
   QUnit.test('_mouseDownHandlerBefore set up selected property', function(assert) {
     var iText = new fabric.IText('test need some word\nsecond line');
     assert.equal(iText.selected, undefined, 'iText has no selected property');
-    iText.canvas = {
-      _activeObject: iText,
-    };
+    canvas.setActiveObject(iText);
+    iText.canvas = canvas;
     iText._mouseDownHandlerBefore({ e: {} });
     assert.equal(iText.selected, true, 'iText has selected property');
+    assert.equal(iText.__lastSelected, undefined, 'iText has no __lastSelected property');
+  });
+  QUnit.test('_mouseUpHandler set selected as true', function(assert) {
+    var iText = new fabric.IText('test');
+    iText.initDelayedCursor = function() {};
+    iText.renderCursorOrSelection = function() {};
+    assert.equal(iText.selected, undefined, 'iText has no selected property');
+    assert.equal(iText.__lastSelected, undefined, 'iText has no __lastSelected property');
+    canvas.setActiveObject(iText);
+    iText.canvas = canvas;
+    iText.mouseUpHandler({ e: {} });
+    assert.equal(iText.selected, true, 'iText has selected property');
+  });
+  QUnit.test('_mouseUpHandler on a selected object enter edit', function(assert) {
+    var iText = new fabric.IText('test');
+    iText.initDelayedCursor = function() {};
+    iText.renderCursorOrSelection = function() {};
+    assert.equal(iText.isEditing, false, 'iText not editing');
+    iText.canvas = canvas;
+    iText.selected = true;
+    iText.__lastSelected = true;
+    iText.mouseUpHandler({ e: {} });
+    assert.equal(iText.isEditing, true, 'iText entered editing');
+    iText.exitEditing();
+  });
+  QUnit.test('_mouseUpHandler on a selected text in a group DOES NOT enter edit', function(assert) {
+    var iText = new fabric.IText('test');
+    iText.initDelayedCursor = function() {};
+    iText.renderCursorOrSelection = function() {};
+    assert.equal(iText.isEditing, false, 'iText not editing');
+    iText.canvas = canvas;
+    iText.selected = true;
+    iText.__lastSelected = true;
+    iText.group = true;
+    iText.mouseUpHandler({ e: {} });
+    assert.equal(iText.isEditing, false, 'iText did not entered editing');
+    iText.exitEditing();
+  });
+  QUnit.test('_mouseUpHandler on a corner of selected text DOES NOT enter edit', function(assert) {
+    var iText = new fabric.IText('test');
+    iText.initDelayedCursor = function() {};
+    iText.renderCursorOrSelection = function() {};
+    assert.equal(iText.isEditing, false, 'iText not editing');
+    iText.canvas = canvas;
+    iText.selected = true;
+    iText.__lastSelected = true;
+    iText.__corner = 'mt';
+    iText.mouseUpHandler({ e: {} });
+    assert.equal(iText.isEditing, false, 'iText did not entered editing');
+    iText.exitEditing();
   });
 })();