Don't break on a null mutation

[rachel-fenichel]

Resolves

No bug number, but this fixes the case when the dragging block has a null mutation. Previously it would get stuck in a dragging state. Found while working on Blockly, where null mutations happen more frequently.

Proposed Changes

Don't call domToMutation if the mutation is null.

[kchadha]

@rachel-fenichel Could you explain the repro for the bug this is fixing a little more?

[rachel-fenichel]

Sure--I'm adding insertion markers into Blockly, so it's a case with the if/else block there. Example blocks with XML and screenshots below.

The if/else block uses a mutation to add else and else if clauses. The basic version of the block has no mutation tag, rather than having a mutation that is explicitly empty. That means that the result of sourceBlock.mutationToDom is null, and result.domToMutation(null) crashes. This happens when creating the insertion marker for a basic if block at the start of a drag.

Keeping this code the same in blockly and scratch blocks will be better for merges--ideally there will be no conflicts in this file.

Here's an if block with no else:

  <block type="controls_if"></block>

Here's an if block with one else if:

  <block type="controls_if">
    <mutation elseif="1"></mutation>
  </block>

Here's an if block with one else and two else ifs:

  <block type="controls_if">
    <mutation elseif="2" else="1"></mutation>
  </block>

[kchadha]

@rachel-fenichel, I see. That makes sense. Thanks for providing the context.

I don't have a sense for how InsertionMarkerManager is used within scratch-blocks... but that's probably a separate issue.

I imagine the xToDom(domToX(stuff)) (or vice-versa domToX(xToDom(stuff)) convention occurs pretty often... or at least it seems natural to think that one would be the inverse function of the other e.g. that domToX should be able to act on the output of xToDom, whatever that output may be. Given that, does it make sense for domToMutation handle the null case by doing an early exit if the input is null?

Given that you're pulling in a change from blockly so that there aren't any future merge conflicts, I don't want to impede this PR, but maybe an issue should be filed to fix this issue and possibly also write unit tests that ensure domToMutation can be called on the output of mutationToDom etc.

[kchadha]

LG

[kchadha]

Approved this PR based on discussions offline. Since domToMutation and mutationToDom are often developer-provided functions, it's definitely good to make these checks.