2.0 custom procedure calls get imported with extra block

[kchadha]

When custom procedure calls that have number or string inputs are imported from Scratch 2.0, an extra (orphan/unreachable) shadow block is created. This block (with opcode text or math_number) has a reference to the procedure call block as its parent, but the original procedure_call block doesn't have any reference to this block (it references a different text/math_number block as its shadow instead). This creates problems during project validation as there shouldn't be unreferenced text/math_number blocks at the top level since they're actually only used as fields of other blocks.

Take as an example, this minimum repro project:

When we load this project into 3.0 from a 2.0 project, we see the following in the Sprites' blocks:

When we create this project from scratch in 3.0, we see the following in the Sprite's blocks:

Notice that there's an extra block with opcode 'text' in the imported project vs. the 3.0 project.

[kchadha]

@paulkaplan, @rachel-fenichel, after a lot of debugging, I've found that this seems to be a bug in the scratch-blocks/scratch-vm boundary instead of in the scratch-vm sb2 import code. I think ScratchBlocks is generating new shadow blocks for procedure call inputs, but VM never deletes the old shadows (nor updates the existing shadow reference to refer to the new shadow block). This just results in adding a new, unreachable shadow block to the VM. I would appreciate any help I can get in debugging this issue as it's a blocker for save/load work.

[paulkaplan]

@rachel-fenichel karishma and I did some more investigation, it looks like there is a specific issue with the way scratch-blocks imports procedure call blocks from XML.

Using this XML, you can see what is going on:

<xml xmlns="http://www.w3.org/1999/xhtml">
  <variables></variables>
  <block type="motion_movesteps" id="move_block" x="115" y="212">
    <value name="STEPS">
      <shadow type="math_number" id="move_shadow">
        <field name="NUM">10</field>
      </shadow>
      <block type="sensing_mousey" id="mouse_y"></block>
    </value>
  </block>
  <block type="procedures_definition" id="definition_block" x="163" y="353">
    <statement name="custom_block">
      <shadow type="procedures_prototype" id="prototype_shadow">
        <mutation proccode="my block %n" argumentids="[&quot;input0&quot;]" argumentnames="[&quot;number1&quot;]" argumentdefaults="[1]" warp="false"></mutation>
        <value name="input0">
          <shadow type="argument_reporter_string_number" id="arg_reporter_shadow">
            <field name="VALUE">number1</field>
          </shadow>
        </value>
      </shadow>
    </statement>
  </block>
  <block type="procedures_call" id="call_block" x="168" y="574">
    <mutation proccode="my block %n" argumentids="[&quot;input0&quot;]" warp="null"></mutation>
    <value name="input0">
      <shadow type="math_number" id="call_shadow">
        <field name="NUM">1</field>
      </shadow>
      <block type="sensing_mousex" id="mousex"></block>
    </value>
  </block>
</xml>

This XML has a call block, a definition block and a move block. Both the call and the move have obscured shadows. The IDs have been given readable names for all blocks including shadows. If you turn on event logging in the vertical playground and import the XML, you can see create events for the call, definition and move blocks. The move block is created with the shadow ID that is given in the XML. However the call block creates a new ID for the shadow, one that is not given in the XML.

This is causing the VM and blocks to get out of sync, leaving an orphaned block in the VM that isn't pointed to from anywhere.

Because the move block maintains the shadow ID given in the XML, it looks like there is a bug with the way the procedure call block is dealing with being created from XML.

[rachel-fenichel]

I dug a little deeper to find where the shadow block is coming from. I still need to figure out how to get the correct shadow block to be there instead. Here's the code.

Relevant code is in blocks_vertical/procedures.js.

The call stack looks like this:

Blockly.ScratchBlocks.ProcedureUtils.callerDomToMutation
Blockly.ScratchBlocks.ProcedureUtils.updateDisplay_
Blockly.ScratchBlocks.ProcedureUtils.createAllInputs_
Blockly.ScratchBlocks.ProcedureUtils.populateArgumentOnCaller_
Blockly.ScratchBlocks.ProcedureUtils.buildShadowDom_

populateArgumentOnCaller_:

/**
 * Populate the argument by attaching the correct child block or shadow to the
 * given input.
 * @param {string} type One of 'b' (boolean), 's' (string) or 'n' (number).
 * @param {number} index The index of this argument into the argument id array.
 * @param {!Object.<string, Blockly.Block>} connectionMap An object mapping
 *     argument IDs to the blocks that were connected to those IDs at the
 *     beginning of the mutation.
 * @param {Blockly.BlockSvg} oldBlock The block that was previously connected to
 *     this input, or null.
 * @param {!Blockly.Input} input The newly created input to populate.
 * @private
 * @this Blockly.Block
 */
Blockly.ScratchBlocks.ProcedureUtils.populateArgumentOnCaller_ = function(type,
    index, connectionMap, oldBlock, input) {
  if (connectionMap && oldBlock) {
    // Reattach the old block.
    connectionMap[input.name] = null;
    oldBlock.outputConnection.connect(input.connection);
    if (type != 'b') {
      // TODO: Preserve old shadow DOM.
      input.connection.setShadowDom(this.buildShadowDom_(type));
    }
  } else {
    this.attachShadow_(input, type);
  }
};

That TODO looks suspiciously relevant.
It's calling this.buildShadowDom_(type):

/**
 * Build a DOM node representing a shadow block of the given type.
 * @param {string} type One of 's' (string) or 'n' (number).
 * @return {!Element} The DOM node representing the new shadow block.
 * @private
 * @this Blockly.Block
 */
Blockly.ScratchBlocks.ProcedureUtils.buildShadowDom_ = function(type) {
  var shadowDom = goog.dom.createDom('shadow');
  if (type == 'n') {
    var shadowType = 'math_number';
    var fieldName = 'NUM';
    var fieldValue = '1';
  } else {
    var shadowType = 'text';
    var fieldName = 'TEXT';
    var fieldValue = '';
  }
  shadowDom.setAttribute('type', shadowType);
  var fieldDom = goog.dom.createDom('field', null, fieldValue);
  fieldDom.setAttribute('name', fieldName);
  shadowDom.appendChild(fieldDom);
  return shadowDom;
};

[rachel-fenichel]

In the else of the populateArgumentOnCaller_ code, it will call Blockly.ScratchBlocks.ProcedureUtils.attachShadow_. That also won't set the ID of the shadow block properly:

/**
 * Create a new shadow block and attach it to the given input.
 * @param {!Blockly.Input} input The value input to attach a block to.
 * @param {string} argumentType One of 'b' (boolean), 's' (string) or
 *     'n' (number).
 * @private
 * @this Blockly.Block
 */
Blockly.ScratchBlocks.ProcedureUtils.attachShadow_ = function(input,
    argumentType) {
  if (argumentType == 'n' || argumentType == 's') {
    var blockType = argumentType == 'n' ? 'math_number' : 'text';
    Blockly.Events.disable();
    try {
      var newBlock = this.workspace.newBlock(blockType);
      if (argumentType == 'n') {
        newBlock.setFieldValue('1', 'NUM');
      } else {
        newBlock.setFieldValue('', 'TEXT');
      }
      newBlock.setShadow(true);
      if (!this.isInsertionMarker()) {
        newBlock.initSvg();
        newBlock.render(false);
      }
    } finally {
      Blockly.Events.enable();
    }
    if (Blockly.Events.isEnabled()) {
      Blockly.Events.fire(new Blockly.Events.BlockCreate(newBlock));
    }
    newBlock.outputConnection.connect(input.connection);
  }
};

[rachel-fenichel]

Since we know that we're doing the right thing for shadow blocks everywhere else, it's worth looking at how that's implemented.

In xml.js, in domToBlockHeadless_:

Find any enclosed blocks or shadows in this tag.

    var childBlockNode = null;
    var childShadowNode = null;
    for (var j = 0, grandchildNode; grandchildNode = xmlChild.childNodes[j];
         j++) {
      if (grandchildNode.nodeType == 1) {
        if (grandchildNode.nodeName.toLowerCase() == 'block') {
          childBlockNode = grandchildNode;
        } else if (grandchildNode.nodeName.toLowerCase() == 'shadow') {
          childShadowNode = grandchildNode;
        }
      }
    }
    // Use the shadow block if there is no child block.
    if (!childBlockNode && childShadowNode) {
      childBlockNode = childShadowNode;
    }

If this is a value or statement input and childShadowNode was set above, set the shadow DOM on the input:

      case 'value':
      case 'statement':
        input = block.getInput(name);
        if (!input) {
          console.warn('Ignoring non-existent input ' + name + ' in block ' +
                       prototypeName);
          break;
        }
        if (childShadowNode) {
          input.connection.setShadowDom(childShadowNode);
        }
...
      case 'next':
        if (childShadowNode && block.nextConnection) {
          block.nextConnection.setShadowDom(childShadowNode);
        }

With the custom procedures this code path is working if the shadow block was not occluded.

[rachel-fenichel]

@kchadha my nice theory from this afternoon was wrong. It might still be an issue but it's not what's causing our current problems.

Trying again to explain what's happening, from walking through loading this XML:

<xml>
  <block type="procedures_call" id="call_block" x="168" y="574">
    <mutation proccode="my block %n" argumentids="[&quot;input0&quot;]" warp="null">
    </mutation>
    <value name="input0">
      <shadow type="math_number" id="call_shadow">
        <field name="NUM">1</field>
      </shadow>
      <block type="sensing_mousex" id="mousex"></block>
    </value>
  </block>
</xml>

domToBlockHeadless_ is called, because of the block tag.
It creates a new block:

  var id = xmlBlock.getAttribute('id');
  block = workspace.newBlock(prototypeName, id);

No mutation has been applied. This block has zero inputs. Inspecting block.inputList returns [].

The first interesting child node is the mutation. It's applied:

case 'mutation':
        // Custom data for an advanced block.
        if (block.domToMutation) {
          block.domToMutation(xmlChild);
          if (block.initSvg) {
            // Mutation may have added some elements that need initializing.
            block.initSvg();
          }
        }
        break;

At the break the block has three inputs (two dummies, one value). The only interesting one is the value input.

The value input has a shadow block connected to it, with a randomly generated ID. Let's call that ID unexpected!.

The unexpected! shadow block was created when applying the mutation: there was no block there, so a shadow block was created and attached.

The next interesting child is the value. It's applied:

case 'value':
      case 'statement':
        input = block.getInput(name);
        if (!input) {
          console.warn('Ignoring non-existent input ' + name + ' in block ' +
                       prototypeName);
          break;
        }
        if (childShadowNode) {
          input.connection.setShadowDom(childShadowNode);
        }
        if (childBlockNode) {
          blockChild = Blockly.Xml.domToBlockHeadless_(childBlockNode,
              workspace);
          if (blockChild.outputConnection) {
            input.connection.connect(blockChild.outputConnection);
          } else if (blockChild.previousConnection) {
            input.connection.connect(blockChild.previousConnection);
          } else {
            goog.asserts.fail(
                'Child block does not have output or previous statement.');
          }
        }
        break;

There is a child shadow node, so input.connection.setShadowDom(childShadowNode) is called. Now there is a shadow block (unexpected!) and a shadow DOM (<shadow type="math_number" id="call_shadow"> <field name="NUM">1</field> </shadow>)

Next, the child block node is considered. From the code above:

blockChild = Blockly.Xml.domToBlockHeadless_(childBlockNode,
              workspace);
          if (blockChild.outputConnection) {
            input.connection.connect(blockChild.outputConnection);
          } 

In connect, there's a check for whether the parent connection is connected. It is, to unexpected!.

So this happens:

// Other connection is already connected to something.
    // Disconnect it and reattach it or bump it as needed.
    var orphanBlock = parentConnection.targetBlock();
    var shadowDom = parentConnection.getShadowDom();
    // Temporarily set the shadow DOM to null so it does not respawn.
    parentConnection.setShadowDom(null);
    // Displaced shadow blocks dissolve rather than reattaching or bumping.
    if (orphanBlock.isShadow()) {
      // Save the shadow block so that field values are preserved.
      shadowDom = Blockly.Xml.blockToDom(orphanBlock);
      orphanBlock.dispose();
      orphanBlock = null;
    } 
... 
    parentConnection.setShadowDom(shadowDom);

That is, the previously attached shadow block is disposed of, and stored on the connection. This overrides the previous value of shadowDom_, and now it'll spawn a block with id unexpected! instead of id call_shadow.

---

I don't have a solution yet. All of the rules are being applied in order. Maybe we need more information in the mutation DOM? Because by the time the mutation is done being applied, we should be able to apply the same rules as we use for any other block--custom procedures shouldn't act different with respect to the connect/disconnect behaviour.

[rachel-fenichel]

Current proposal:
Add a "generateShadows" attribute to the mutation parsing. Provide it when setting the mutation from the custom procedure editor callbacks. This makes the current code paths happen.

By default, do not generate shadow blocks when creating inputs. XML generated by scratch-blocks (when saving the project) and by scratch-vm (for loading a project) should not set "generateShadows". That means that a shadow block will not auto-generate, and the shadow DOM from the XML will be respected.

@kchadha I've turned this idea around and I'm happy with it, though I don't know exactly where to edit the custom procedure editor code to add "generateShadows". Let me know if you see problems with this approach.

[kchadha]

@rachel-fenichel, this sounds good to me!