Calling a.setParent(b) without connecting blocks a and b can cause major rendering issues and errors

[jschanker]

Describe the bug

Calling a.setParent(b) on blocks a and b without connecting them can cause rendering issues where the parent and unconnected child block can move together with varying degrees of space in between them. If a had no parent originally, then attempting to delete b results in an unmovable b and error messages on any event.

To Reproduce (2 possibilities)

Steps to reproduce the behavior (two different types):

1. Go to the Blockly Playground.
2. Import the following XML:

<xml xmlns="https://developers.google.com/blockly/xml">
  <block type="text_print" id="s+b@l4y{Nw=CeqI3/qzW" x="38" y="38">
    <value name="TEXT">
      <shadow type="text" id="^QF~!eeGDNIEJE8Ye7ji">
        <field name="TEXT">abc</field>
      </shadow>
      <block type="text_join" id="M$%c|%@HCw^J:hg7*J%t">
        <mutation items="2"></mutation>
        <value name="ADD0">
          <block type="text" id="w,TRg]2*bD`k[5+`I$VE">
            <field name="TEXT"></field>
          </block>
        </value>
      </block>
    </value>
  </block>
</xml>

3a. Enter the following in the Developer console:
workspace.getAllBlocks().find(block => block.type === "text").setParent(workspace.getAllBlocks().find(block => block.type === "text_print"))
4a. There will be a "Still connected to parent block error." Move the text_join block around the workspace and watch the text block jump all around.

OR...

3b. Disconnect the text block and then enter the code from 3a in the console.
4b. No error this time, but if you move the print block, you will notice that the disconnected text block moves together with the print block. Attempt to delete the print block and the errors ensue.

Expected behavior

Should setParent be a package method? I'm not sure why it should be called for any other purpose than connecting/disconnecting blocks. In this case (outside of Blockly.Block and Blockly.BlockSvg), it should not be needed outside of the Connection class. The only other places I could find it being called were in testing to set the parent to null here:

blockly/tests/mocha/event_test.js

Line 975 in 64188ae

block.setParent(null);

and (indirectly) here:

blockly/tests/mocha/event_test.js

Line 977 in 64188ae

addMoveEventParent(events, block, null);

[BeksOmega]

Should setParent be a package method?

setParent should definitely be package! And taking a quick look, setShadow should be as well. There could be other functions that need to be changed too, but I just did a quick skim so I'm not sure.

[jschanker]

I browsed the history of the block.js file because I had the following questions:

1. Why is the block removed from its old parent's child list when setParent is called in a disallowed case? I.e., why can errors be thrown after Blockly.utils.arrayRemove is called? This is what permits the error reproduced in the first way.

2. Why is there a mention about disconnecting from superior blocks in the comments if all that happens is that the parent is changed to something else?

The answer to question 2 and perhaps 1 can be traced back to the commit here where disconnection calls were replaced with thrown errors during refactoring: 2a1ffa1#diff-9b5800799b33a83e7ff338b52ff14964b5ab04f1a6e1834523b121f2ea070aeaR419.

It seems to me then that for future stability, this method should be changed even though it's being declared as package. Specifically, I think that a precondition for calling a.setParent(b) for non-null b should be that the output/previous connection of a is connected to an input/next connection of b. If b is null, then a should not be connected via its previous/output connections. The pull request #4999 enforces this.

[jschanker]

Resolved by #4995 and #4999.