fix: insertion markers and change events to work with JSO hooks (#5378)

* fix: add tests for fixing change events

* fix: change events and insertion markers

* fix: build:

* fix: remove duplicate code

* fix: requires

core/events/events_block_change.js

@@ -15,6 +15,8 @@ goog.module.declareLegacyNamespace();
 
 /* eslint-disable-next-line no-unused-vars */
 const Block = goog.requireType('Blockly.Block');
+/* eslint-disable-next-line no-unused-vars */
+const BlockSvg = goog.requireType('Blockly.BlockSvg');
 const Events = goog.require('Blockly.Events');
 const Xml = goog.require('Blockly.Xml');
 const object = goog.require('Blockly.utils.object');
@@ -125,17 +127,15 @@ BlockChange.prototype.run = function(forward) {
       block.setInputsInline(!!value);
       break;
     case 'mutation': {
-      let oldMutation = '';
-      if (block.mutationToDom) {
-        const oldMutationDom = block.mutationToDom();
-        oldMutation = oldMutationDom && Xml.domToText(oldMutationDom);
+      const oldState = BlockChange.getExtraBlockState_(
+          /** @type {!BlockSvg} */ (block));
+      if (block.loadExtraState) {
+        block.loadExtraState(JSON.parse(/** @type {string} */ (value) || '{}'));
+      } else if (block.domToMutation) {
+        block.domToMutation(
+            Xml.textToDom(/** @type {string} */ (value) || '<mutation/>'));
       }
-      if (block.domToMutation) {
-        const dom = Xml.textToDom(/** @type {string} */
-                                  (value) || '<mutation/>');
-        block.domToMutation(dom);
-      }
-      Events.fire(new BlockChange(block, 'mutation', null, oldMutation, value));
+      Events.fire(new BlockChange(block, 'mutation', null, oldState, value));
       break;
     }
     default:
@@ -143,6 +143,26 @@ BlockChange.prototype.run = function(forward) {
   }
 };
 
+// TODO (#5397): Encapsulate this in the BlocklyMutationChange event when
+//    refactoring change events.
+/**
+ * Returns the extra state of the given block (either as XML or a JSO, depending
+ * on the block's definition).
+ * @param {!BlockSvg} block The block to get the extra state of.
+ * @return {string} A stringified version of the extra state of the given block.
+ * @package
+ */
+BlockChange.getExtraBlockState_ = function(block) {
+  if (block.saveExtraState) {
+    const state = block.saveExtraState();
+    return state ? JSON.stringify(state) : '';
+  } else if (block.mutationToDom) {
+    const state = block.mutationToDom();
+    return state ? Xml.domToText(state) : '';
+  }
+  return '';
+};
+
 registry.register(registry.Type.EVENT, Events.CHANGE, BlockChange);
 
 exports = BlockChange;

core/field.js

@@ -897,11 +897,11 @@ Field.prototype.setValue = function(newValue) {
     return;
   }
 
+  this.doValueUpdate_(newValue);
   if (source && Events.isEnabled()) {
     Events.fire(new (Events.get(Events.BLOCK_CHANGE))(
         source, 'field', this.name || null, oldValue, newValue));
   }
-  this.doValueUpdate_(newValue);
   if (this.isDirty_) {
     this.forceRerender();
   }

core/insertion_marker_manager.js

@@ -289,7 +289,12 @@ InsertionMarkerManager.prototype.createMarkerBlock_ = function(sourceBlock) {
   try {
     result = this.workspace_.newBlock(imType);
     result.setInsertionMarker(true);
-    if (sourceBlock.mutationToDom) {
+    if (sourceBlock.saveExtraState) {
+      const state = sourceBlock.saveExtraState();
+      if (state) {
+        result.loadExtraState(state);
+      }
+    } else if (sourceBlock.mutationToDom) {
       const oldMutationDom = sourceBlock.mutationToDom();
       if (oldMutationDom) {
         result.domToMutation(oldMutationDom);

core/mutator.js

@@ -34,7 +34,6 @@ const Svg = goog.require('Blockly.utils.Svg');
 /* eslint-disable-next-line no-unused-vars */
 const Workspace = goog.requireType('Blockly.Workspace');
 const WorkspaceSvg = goog.require('Blockly.WorkspaceSvg');
-const Xml = goog.require('Blockly.Xml');
 const dom = goog.require('Blockly.utils.dom');
 const internalConstants = goog.require('Blockly.internalConstants');
 const object = goog.require('Blockly.utils.object');
@@ -408,9 +407,8 @@ Mutator.prototype.workspaceChanged_ = function(e) {
   // When the mutator's workspace changes, update the source block.
   if (this.rootBlock_.workspace == this.workspace_) {
     Events.setGroup(true);
-    const block = this.block_;
-    const oldMutationDom = block.mutationToDom();
-    const oldMutation = oldMutationDom && Xml.domToText(oldMutationDom);
+    const block = /** @type {!BlockSvg} */ (this.block_);
+    const oldExtraState = Events.BlockChange.getExtraBlockState_(block);
 
     // Switch off rendering while the source block is rebuilt.
     const savedRendered = block.rendered;
@@ -428,11 +426,10 @@ Mutator.prototype.workspaceChanged_ = function(e) {
       block.render();
     }
 
-    const newMutationDom = block.mutationToDom();
-    const newMutation = newMutationDom && Xml.domToText(newMutationDom);
-    if (oldMutation != newMutation) {
+    const newExtraState = Events.BlockChange.getExtraBlockState_(block);
+    if (oldExtraState != newExtraState) {
       Events.fire(new (Events.get(Events.BLOCK_CHANGE))(
-          block, 'mutation', null, oldMutation, newMutation));
+          block, 'mutation', null, oldExtraState, newExtraState));
       // Ensure that any bump is part of this mutation's event group.
       const group = Events.getGroup();
       setTimeout(function() {

tests/deps.js

@@ -130,7 +130,7 @@ goog.addDependency('../../core/menu.js', ['Blockly.Menu'], ['Blockly.browserEven
 goog.addDependency('../../core/menuitem.js', ['Blockly.MenuItem'], ['Blockly.utils.aria', 'Blockly.utils.dom', 'Blockly.utils.idGenerator'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/metrics_manager.js', ['Blockly.MetricsManager'], ['Blockly.registry', 'Blockly.utils.Size', 'Blockly.utils.toolbox'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/msg.js', ['Blockly.Msg'], ['Blockly.utils.global'], {'lang': 'es6', 'module': 'goog'});
-goog.addDependency('../../core/mutator.js', ['Blockly.Mutator'], ['Blockly.Bubble', 'Blockly.Events', 'Blockly.Events.BlockChange', 'Blockly.Events.BubbleOpen', 'Blockly.Icon', 'Blockly.Options', 'Blockly.WorkspaceSvg', 'Blockly.Xml', 'Blockly.internalConstants', 'Blockly.utils.Svg', 'Blockly.utils.dom', 'Blockly.utils.object', 'Blockly.utils.toolbox', 'Blockly.utils.xml'], {'lang': 'es6', 'module': 'goog'});
+goog.addDependency('../../core/mutator.js', ['Blockly.Mutator'], ['Blockly.Bubble', 'Blockly.Events', 'Blockly.Events.BlockChange', 'Blockly.Events.BubbleOpen', 'Blockly.Icon', 'Blockly.Options', 'Blockly.WorkspaceSvg', 'Blockly.internalConstants', 'Blockly.utils.Svg', 'Blockly.utils.dom', 'Blockly.utils.object', 'Blockly.utils.toolbox', 'Blockly.utils.xml'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/names.js', ['Blockly.Names'], ['Blockly.Msg', 'Blockly.Variables', 'Blockly.internalConstants'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/options.js', ['Blockly.Options'], ['Blockly.Theme', 'Blockly.Themes.Classic', 'Blockly.registry', 'Blockly.utils.deprecation', 'Blockly.utils.idGenerator', 'Blockly.utils.toolbox'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/positionable_helpers.js', ['Blockly.uiPosition'], ['Blockly.Scrollbar', 'Blockly.utils.Rect', 'Blockly.utils.toolbox'], {'lang': 'es6', 'module': 'goog'});

tests/deps.mocha.js

@@ -130,7 +130,7 @@ goog.addDependency('../../core/menu.js', ['Blockly.Menu'], ['Blockly.browserEven
 goog.addDependency('../../core/menuitem.js', ['Blockly.MenuItem'], ['Blockly.utils.aria', 'Blockly.utils.dom', 'Blockly.utils.idGenerator'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/metrics_manager.js', ['Blockly.MetricsManager'], ['Blockly.registry', 'Blockly.utils.Size', 'Blockly.utils.toolbox'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/msg.js', ['Blockly.Msg'], ['Blockly.utils.global'], {'lang': 'es6', 'module': 'goog'});
-goog.addDependency('../../core/mutator.js', ['Blockly.Mutator'], ['Blockly.Bubble', 'Blockly.Events', 'Blockly.Events.BlockChange', 'Blockly.Events.BubbleOpen', 'Blockly.Icon', 'Blockly.Options', 'Blockly.WorkspaceSvg', 'Blockly.Xml', 'Blockly.internalConstants', 'Blockly.utils.Svg', 'Blockly.utils.dom', 'Blockly.utils.object', 'Blockly.utils.toolbox', 'Blockly.utils.xml'], {'lang': 'es6', 'module': 'goog'});
+goog.addDependency('../../core/mutator.js', ['Blockly.Mutator'], ['Blockly.Bubble', 'Blockly.Events', 'Blockly.Events.BlockChange', 'Blockly.Events.BubbleOpen', 'Blockly.Icon', 'Blockly.Options', 'Blockly.WorkspaceSvg', 'Blockly.internalConstants', 'Blockly.utils.Svg', 'Blockly.utils.dom', 'Blockly.utils.object', 'Blockly.utils.toolbox', 'Blockly.utils.xml'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/names.js', ['Blockly.Names'], ['Blockly.Msg', 'Blockly.Variables', 'Blockly.internalConstants'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/options.js', ['Blockly.Options'], ['Blockly.Theme', 'Blockly.Themes.Classic', 'Blockly.registry', 'Blockly.utils.deprecation', 'Blockly.utils.idGenerator', 'Blockly.utils.toolbox'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/positionable_helpers.js', ['Blockly.uiPosition'], ['Blockly.Scrollbar', 'Blockly.utils.Rect', 'Blockly.utils.toolbox'], {'lang': 'es6', 'module': 'goog'});
@@ -303,6 +303,7 @@ goog.addDependency('../../generators/python/variables.js', ['Blockly.Python.vari
 goog.addDependency('../../generators/python/variables_dynamic.js', ['Blockly.Python.variablesDynamic'], ['Blockly.Python', 'Blockly.Python.variables']);
 goog.addDependency('../../tests/mocha/.mocharc.js', [], []);
 goog.addDependency('../../tests/mocha/astnode_test.js', ['Blockly.test.astNode'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
+goog.addDependency('../../tests/mocha/block_change_event_test.js', ['Blockly.test.blockChangeEvent'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../tests/mocha/block_json_test.js', ['Blockly.test.blockJson'], [], {'lang': 'es5', 'module': 'goog'});
 goog.addDependency('../../tests/mocha/block_test.js', ['Blockly.test.blocks'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../tests/mocha/comment_test.js', ['Blockly.test.comments'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
@@ -338,6 +339,7 @@ goog.addDependency('../../tests/mocha/json_test.js', ['Blockly.test.json'], ['Bl
 goog.addDependency('../../tests/mocha/keydown_test.js', ['Blockly.test.keydown'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../tests/mocha/logic_ternary_test.js', ['Blockly.test.logicTernary'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../tests/mocha/metrics_test.js', ['Blockly.test.metrics'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
+goog.addDependency('../../tests/mocha/mutator_test.js', ['Blockly.test.mutator'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../tests/mocha/names_test.js', ['Blockly.test.names'], ['Blockly.test.helpers'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../tests/mocha/procedures_test.js', ['Blockly.test.procedures'], ['Blockly.Blocks.procedures', 'Blockly.Msg', 'Blockly.test.helpers', 'Blockly.test.procedureHelpers'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../tests/mocha/procedures_test_helpers.js', ['Blockly.test.procedureHelpers'], [], {'lang': 'es6', 'module': 'goog'});

tests/mocha/block_change_event_test.js

@@ -0,0 +1,74 @@
+
+/**
+ * @license
+ * Copyright 2021 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+goog.module('Blockly.test.blockChangeEvent');
+
+const {defineMutatorBlocks, sharedTestSetup, sharedTestTeardown} = goog.require('Blockly.test.helpers');
+
+
+suite('Block Change Event', function() {
+  setup(function() {
+    sharedTestSetup.call(this);
+    this.workspace = new Blockly.Workspace();
+  });
+
+  teardown(function() {
+    sharedTestTeardown.call(this);
+  });
+
+  suite('Undo and Redo', function() {
+    suite('Mutation', function() {
+      setup(function() {
+        defineMutatorBlocks();
+      });
+  
+      teardown(function() {
+        Blockly.Extensions.unregister('xml_mutator');
+        Blockly.Extensions.unregister('jso_mutator');
+      });
+      
+      suite('XML', function() {
+        test('Undo', function() {
+          const block = this.workspace.newBlock('xml_block', 'block_id');
+          block.domToMutation(
+              Blockly.Xml.textToDom('<mutation hasInput="true"/>'));
+          const blockChange = new Blockly.Events.BlockChange(
+              block, 'mutation', null, '', '<mutation hasInput="true"/>');
+          blockChange.run(false);
+          chai.assert.isFalse(block.hasInput);
+        });
+
+        test('Redo', function() {
+          const block = this.workspace.newBlock('xml_block', 'block_id');
+          const blockChange = new Blockly.Events.BlockChange(
+              block, 'mutation', null, '', '<mutation hasInput="true"/>');
+          blockChange.run(true);
+          chai.assert.isTrue(block.hasInput);
+        });
+      });
+
+      suite('JSO', function() {
+        test('Undo', function() {
+          const block = this.workspace.newBlock('jso_block', 'block_id');
+          block.loadExtraState({hasInput: true});
+          const blockChange = new Blockly.Events.BlockChange(
+              block, 'mutation', null, '', '{"hasInput":true}');
+          blockChange.run(false);
+          chai.assert.isFalse(block.hasInput);
+        });
+
+        test('Redo', function() {
+          const block = this.workspace.newBlock('jso_block', 'block_id');
+          const blockChange = new Blockly.Events.BlockChange(
+              block, 'mutation', null, '', '{"hasInput":true}');
+          blockChange.run(true);
+          chai.assert.isTrue(block.hasInput);
+        });
+      });
+    });
+  });
+});

tests/mocha/index.html

@@ -53,6 +53,7 @@
 
     <script>
       goog.require('Blockly.test.astNode');
+      goog.require('Blockly.test.blockChangeEvent');
       goog.require('Blockly.test.blockJson');
       goog.require('Blockly.test.blocks');
       goog.require('Blockly.test.comments');
@@ -88,6 +89,7 @@
       goog.require('Blockly.test.keydown');
       goog.require('Blockly.test.logicTernary');
       goog.require('Blockly.test.metrics');
+      goog.require('Blockly.test.mutator');
       goog.require('Blockly.test.names');
       goog.require('Blockly.test.procedures');
       goog.require('Blockly.test.registry');

tests/mocha/mutator_test.js

@@ -0,0 +1,71 @@
+
+/**
+ * @license
+ * Copyright 2021 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+goog.module('Blockly.test.mutator');
+
+const {createRenderedBlock, defineMutatorBlocks, sharedTestSetup, sharedTestTeardown} = goog.require('Blockly.test.helpers');
+
+
+suite('Mutator', function() {
+  setup(function() {
+    sharedTestSetup.call(this);
+  });
+
+  suite('Firing change event', function() {
+    setup(function() {
+      this.workspace = Blockly.inject('blocklyDiv', {});
+      defineMutatorBlocks();
+    });
+
+    teardown(function() {
+      Blockly.Extensions.unregister('xml_mutator');
+      Blockly.Extensions.unregister('jso_mutator');
+      sharedTestTeardown.call(this);
+    });
+
+    test('No change', function() {
+      var block = createRenderedBlock(this.workspace, 'xml_block');
+      block.mutator.setVisible(true);
+      var mutatorWorkspace = block.mutator.getWorkspace();
+      // Trigger mutator change listener.
+      createRenderedBlock(mutatorWorkspace, 'checkbox_block');
+      chai.assert.isTrue(
+          this.eventsFireStub.getCalls().every(
+              ({args}) =>
+                args[0].type !== Blockly.Events.BLOCK_CHANGE ||
+                args[0].element !== 'mutation'));
+    });
+
+    test('XML', function() {
+      var block = createRenderedBlock(this.workspace, 'xml_block');
+      block.mutator.setVisible(true);
+      var mutatorWorkspace = block.mutator.getWorkspace();
+      mutatorWorkspace.getBlockById('check_block')
+          .setFieldValue('TRUE', 'CHECK');
+      chai.assert.isTrue(
+          this.eventsFireStub.getCalls().some(
+              ({args}) =>
+                args[0].type === Blockly.Events.BLOCK_CHANGE &&
+                args[0].element === 'mutation' &&
+                /<mutation.*><\/mutation>/.test(args[0].newValue)));
+    });
+
+    test('JSO', function() {
+      var block = createRenderedBlock(this.workspace, 'jso_block');
+      block.mutator.setVisible(true);
+      var mutatorWorkspace = block.mutator.getWorkspace();
+      mutatorWorkspace.getBlockById('check_block')
+          .setFieldValue('TRUE', 'CHECK');
+      chai.assert.isTrue(
+          this.eventsFireStub.getCalls().some(
+              ({args}) =>
+                args[0].type === Blockly.Events.BLOCK_CHANGE &&
+                args[0].element === 'mutation' &&
+                args[0].newValue === '{"hasInput":true}'));
+    });
+  });
+});

tests/mocha/test_helpers.js

@@ -540,6 +540,100 @@ function defineBasicBlockWithField() {
 }
 exports.defineBasicBlockWithField = defineBasicBlockWithField;
 
+function defineMutatorBlocks() {
+  Blockly.defineBlocksWithJsonArray([
+    {
+      'type': 'xml_block',
+      'mutator': 'xml_mutator'
+    },
+    {
+      'type': 'jso_block',
+      'mutator': 'jso_mutator'
+    },
+    {
+      'type': 'checkbox_block',
+      'message0': '%1',
+      'args0': [
+        {
+          'type': 'field_checkbox',
+          'name': 'CHECK'
+        }
+      ]
+    }
+  ]);
+
+  const xmlMutator = {
+    hasInput: false,
+
+    mutationToDom: function() {
+      var mutation = Blockly.utils.xml.createElement('mutation');
+      mutation.setAttribute('hasInput', this.hasInput);
+      return mutation;
+    },
+
+    domToMutation: function(mutation) {
+      this.hasInput = mutation.getAttribute('hasInput') == 'true';
+      this.updateShape();
+    },
+
+    decompose: function(workspace) {
+      var topBlock = workspace.newBlock('checkbox_block', 'check_block');
+      topBlock.initSvg();
+      topBlock.render();
+      return topBlock;
+    },
+
+    compose: function(topBlock) {
+      this.hasInput = topBlock.getFieldValue('CHECK') == 'TRUE';
+      this.updateShape();
+    },
+
+    updateShape: function() {
+      if (this.hasInput && !this.getInput('INPUT')) {
+        this.appendValueInput('INPUT');
+      } else if (!this.hasInput && this.getInput('INPUT')) {
+        this.removeInput('INPUT');
+      }
+    }
+  };
+  Blockly.Extensions.registerMutator('xml_mutator', xmlMutator);
+
+  const jsoMutator = {
+    hasInput: false,
+
+    saveExtraState: function() {
+      return {hasInput: this.hasInput};
+    },
+
+    loadExtraState: function(state) {
+      this.hasInput = state.hasInput || false;
+      this.updateShape();
+    },
+
+    decompose: function(workspace) {
+      var topBlock = workspace.newBlock('checkbox_block', 'check_block');
+      topBlock.initSvg();
+      topBlock.render();
+      return topBlock;
+    },
+
+    compose: function(topBlock) {
+      this.hasInput = topBlock.getFieldValue('CHECK') == 'TRUE';
+      this.updateShape();
+    },
+
+    updateShape: function() {
+      if (this.hasInput && !this.getInput('INPUT')) {
+        this.appendValueInput('INPUT');
+      } else if (!this.hasInput && this.getInput('INPUT')) {
+        this.removeInput('INPUT');
+      }
+    }
+  };
+  Blockly.Extensions.registerMutator('jso_mutator', jsoMutator);
+}
+exports.defineMutatorBlocks = defineMutatorBlocks;
+
 function createTestBlock() {
   return {
     id: 'test',