Fix bugs related to events firing during block create (#4278)

Fix bugs related to events firing during block create and add test for expected block init behavior

core/block.js

@@ -200,29 +200,40 @@ Blockly.Block = function(workspace, prototypeName, opt_id) {
   workspace.addTopBlock(this);
   workspace.addTypedBlock(this);
 
-  // Call an initialization function, if it exists.
-  if (typeof this.init == 'function') {
-    this.init();
+  // All events fired should be part of the same group.
+  // Any events fired during init should not be undoable,
+  // so that block creation is atomic.
+  var existingGroup = Blockly.Events.getGroup();
+  if (!existingGroup) {
+    Blockly.Events.setGroup(true);
   }
+  var initialUndoFlag = Blockly.Events.recordUndo;
+
+  try {
+    // Call an initialization function, if it exists.
+    if (typeof this.init == 'function') {
+      Blockly.Events.recordUndo = false;
+      this.init();
+      Blockly.Events.recordUndo = initialUndoFlag;
+    }
+
+    // Fire a create event.
+    if (Blockly.Events.isEnabled()) {
+      Blockly.Events.fire(new Blockly.Events.BlockCreate(this));
+    }
+
+  } finally {
+    if (!existingGroup) {
+      Blockly.Events.setGroup(false);
+    }
+    // In case init threw, recordUndo flag should still be reset.
+    Blockly.Events.recordUndo = initialUndoFlag;
+  }
+  
   // Record initial inline state.
   /** @type {boolean|undefined} */
   this.inputsInlineDefault = this.inputsInline;
 
-  // Fire a create event.
-  if (Blockly.Events.isEnabled()) {
-    var existingGroup = Blockly.Events.getGroup();
-    if (!existingGroup) {
-      Blockly.Events.setGroup(true);
-    }
-    try {
-      Blockly.Events.fire(new Blockly.Events.BlockCreate(this));
-    } finally {
-      if (!existingGroup) {
-        Blockly.Events.setGroup(false);
-      }
-    }
-
-  }
   // Bind an onchange function, if it exists.
   if (typeof this.onchange == 'function') {
     this.setOnChange(this.onchange);

core/flyout_base.js

@@ -801,12 +801,15 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) {
 
   if (Blockly.Events.isEnabled()) {
     Blockly.Events.setGroup(true);
-    Blockly.Events.fire(new Blockly.Events.Create(newBlock));
     // Fire a VarCreate event for each (if any) new variable created.
     for (var i = 0; i < newVariables.length; i++) {
       var thisVariable = newVariables[i];
       Blockly.Events.fire(new Blockly.Events.VarCreate(thisVariable));
     }
+
+    // Block events come after var events, in case they refer to newly created
+    // variables.
+    Blockly.Events.fire(new Blockly.Events.Create(newBlock));
   }
   if (this.autoClose) {
     this.hide();

tests/mocha/block_test.js

@@ -1750,4 +1750,34 @@ suite('Blocks', function() {
       });
     });
   });
+
+  suite('Initialization', function() {
+    setup(function() {
+      Blockly.defineBlocksWithJsonArray([
+        {
+          "type": "init_test_block",
+          "message0": ""
+        },
+      ]);
+    });
+    test('recordUndo is reset even if init throws', function() {
+      // The test could pass if init is never called,
+      // so we assert init was called to be safe.
+      var initCalled = false;
+      var recordUndoDuringInit;
+      Blockly.Blocks['init_test_block'].init = function() {
+        initCalled = true;
+        recordUndoDuringInit = Blockly.Events.recordUndo;
+        throw new Error();
+      };
+      chai.assert.throws(function() {
+        this.workspace.newBlock('init_test_block');
+      }.bind(this));
+      chai.assert.isFalse(recordUndoDuringInit,
+          'recordUndo should be false during block init function');
+      chai.assert.isTrue(Blockly.Events.recordUndo,
+          'recordUndo should be reset to true after init');
+      chai.assert.isTrue(initCalled, 'expected init function to be called');
+    });
+  });
 });

tests/mocha/event_test.js

@@ -32,10 +32,16 @@ suite('Events', function() {
 
   function createSimpleTestBlock(workspace) {
     // Disable events while constructing the block: this is a test of the
-    // Blockly.Event constructors, not the block constructor.s
+    // Blockly.Event constructors, not the block constructors.
+    // Set the group id to avoid an extra call to genUid.
     Blockly.Events.disable();
-    var block = new Blockly.Block(
-        workspace, 'simple_test_block');
+    try {
+      Blockly.Events.setGroup('unused');
+      var block = new Blockly.Block(
+          workspace, 'simple_test_block');
+    } finally {
+      Blockly.Events.setGroup(false);
+    }
     Blockly.Events.enable();
     return block;
   }