WorkspaceToDOM Includes Insertion Markers

[amber-cd]

Problem statement

If workspaceToDom is called while an insertion marker is visible, the insertion marker is included and can become a 'real' block.

Expected Behavior

workspaceToDom should ignore insertion markers, as they are UI only.

Actual Behavior

If workspaceToDom is called while an insertion marker is visible, the insertion marker is included and can become a 'real' block.

Steps to Reproduce

This is probably a bit of a tough one to run into 'in the wild'... we ran into it due to a timing issue with Blockly as nested in a React component where we run workspaceToDom in order to save the XML to the store, then repopulate the workspace from that 'changed' XML before the insertion marker can be deleted. However, to verify the issue, I believe you can just do:

1. Add a listener to the workspace to run workspaceToDom when it changes.
2. Begin dragging the child out of a top-level block
3. While the insertion marker is still visible, check the output of workspaceToDom.

(This may require a bit of playing around... we specifically encountered it by dragging the block and holding it for a little while.)

Operating System and Browser

• Desktop Chrome

I expect it applies to other browsers, but haven't tested it.

Additional Information

I realize this is a pretty obscure scenario that only really occurs in scenarios like ours wherein we use React to try to make Blockly behave like a controlled component; however, I do believe this still constitutes a legitimate bug.

I nearly just made a PR to fix this, but I wanted advice on whether it would be accepted/whether this was an appropriate solution. For the record, my fix is just a simple fix to workspaceToDom to skip insertion markers:

Blockly.Xml.workspaceToDom = function(workspace, opt_noId) {
  var xml = Blockly.Xml.utils.createElement('xml');
  var variablesElement = Blockly.Xml.variablesToDom(
      Blockly.Variables.allUsedVarModels(workspace));
  if (variablesElement.hasChildNodes()) {
    xml.appendChild(variablesElement);
  }
  var comments = workspace.getTopComments(true);
  for (var i = 0, comment; comment = comments[i]; i++) {
    xml.appendChild(comment.toXmlWithXY(opt_noId));
  }
  var blocks = workspace.getTopBlocks(true);
  for (var i = 0, block; block = blocks[i]; i++) {
    if (!block.isInsertionMarker_) {
      xml.appendChild(Blockly.Xml.blockToDomWithXY(block, opt_noId));
    }
  }
  return xml;
};

We may wind up just implementing this in our own fork, but I wanted to run it by you guys first to see if this was something you wanted a fix for and/or if my fix is appropriate or if there would be a more appropriate one.

[rachel-fenichel]

This is slightly related to #2249.

I played around with your fix and found that it doesn't work if the insertion marker is connected to another block.

In this case the entire block stack will not be serialized:

In this case the stack will be serialized, but it won't include the last block:

To fix this we would need logic to skip insertion markers and continue serializing blocks past them, rather than stopping when we hit one. This sounds like it could get gnarly.

One possible workaround is to check whether the user is mid-drag when you're trying to serialize (by checking workspace.isDragging() and deferring or skipping that serialization.

To repro for future testing, in the playground I:

1. opened developer tools
2. put a single block on the workspace
3. started dragging the block
4. hit F8 to pause (or set up a breakpoint)
5. ran toXml() in the console
6. hit run
7. check the text in the textarea, or clear the workspace and hit "Import from XML"

[amber-cd]

That workspace.isDragging() check seems to have fixed it nicely for now (it's probably generally good practice for us to skip generating the workspace code while dragging anyway, since it saves on unnecessary processing). I'd say it's probably still good practice to filter out the insertion markers in the appropriate places (good catch re: the circumstances where it stops serialization, btw, I didn't even think about that) but this shouldn't prevent us from merging the most recent master into our branch. We should be good to go just as soon as the "style" attribute issue I discussed with Abby is resolved. Thanks for the quick response!

[gengshenghong]

I've tried with workspace.isDragging(), it "fixed" some incorrect state for code generation, but the behaviour is not same with old version of Blockly. The code is not changed at all during dragging, it is not same with "workspaceToDom" in my opinion, as we do want to see the possible code generated for the "dragging block" usually. I wonder whether it is expected?

Above fix in #2667 can keep the behaviour totally same with previous versions, for real time code generation.

Thanks,
Shenghong

[rachel-fenichel]

As discussed above, ignoring the insertion marker is not enough. What about the case where it's a loop block and is wrapping other blocks? Ignoring it ignores a lot of children as well.

We are not currently supporting generating code mid-drag. I may add something to the generators to bail early if there is a drag in progress.

[BeksOmega]

I think this can be handled in almost the same way that code generation is handled. The only problem is deciding what blockToDom should return if it encounters an insertion marker with no children.

Some options:

1. Return null. Cons:
   • Now we need lots of "is null" checks.
   • Hard to get typing to like it.
2. Return an empty block element: <block/>
   • I don't think this will work because currently an error is thrown when this happens.
     Uncaught TypeError: Block type unspecified: <block/>
   • I think this is meant to help developers, so it should probably be kept.
3. Return some sort of "insertion marker type block":
   <block type="$$insertion$$"/>
4. Return some sort of insertion marker node:
   <insertion/>

Do any of those options sound workable? Or maybe there's a different way of handling serialization that would avoid this problem. (If there is I would love that)

[rachel-fenichel]

@NeilFraser What's your opinion on this one?

The fifth option is to throw an error.

I'm leaning toward option 1. My reasons:

• It's an API change to allow returning null, but we already were serializing something broken, and making it more explicit is better.
• Throwing an error would also require checks and catches, the same as null. The benefit there is that it gets even more explicit.
• Empty elements, insertion marker nodes, insertion marker type blocks, etc all require extra checks or else push the problem down the line, to the next time that it's loaded. I'd rather have the error be closer to the source, in both time and code.

[NeilFraser]

Suggest having blockToDom return an empty DocumentFragment. The caller can append that to the DOM and it will quietly disappear without a trace.

[samelhusseini]

Fixed.