Hide connection previews in `revertDrag`

[rachel-fenichel]

Check for duplicates

• I have searched for similar issues before opening a new one.

Problem

While implementing constrained drag in keyboard experimentation, I found that revertDrag in BlockDragStrategy does not call connectionPreviewer.hidePreview.

So far this has been safe because drags are only reverted when dropped onto an invalid location.

With the option to end a drag with escape, it's important to fully clean up in revertDrag.

Request

Call connectionPreviwer.hidePreview in revertDrag. Should be a no-op if no preview was being shown.

Alternatives considered

No response

Additional context

No response

[cpcallen]

I am confused. BlockDragStrategy.prototype.revertDrag certainly does call this.connectionPreviewer.hidePreview (and it's not a recent change, either).

Now looking to see howMover is using revertDrag to see what @rachel-fenichel encountered that caused her to file this bug.

[cpcallen]

OK, so the proximate cause of the creation of this issue appears to be that (absent the call to hidePreview in abortMove added by @rachel-fenichel in RaspberryPiFoundation/blockly-keyboard-experimentation#352), the existing call to hidePreview in revertDrag will end up throwing a DOM error:

block_svg.ts:313 Uncaught NotFoundError: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.
    at BlockSvg$$module$build$src$core$block_svg.setParent (block_svg.ts:313:36)
    at RenderedConnection$$module$build$src$core$rendered_connection.disconnectInternal (connection.ts:295:53)
    at RenderedConnection$$module$build$src$core$rendered_connection.disconnectInternal (rendered_connection.ts:484:11)
    at RenderedConnection$$module$build$src$core$rendered_connection.disconnect (connection.ts:260:10)
    at BlockSvg$$module$build$src$core$block_svg.unplugFromStack (block.ts:504:31)
    at BlockSvg$$module$build$src$core$block_svg.unplug (block.ts:412:12)
    at InsertionMarkerPreviewer$$module$build$src$core$insertion_marker_previewer.hideInsertionMarker (insertion_marker_previewer.ts:238:14)
    at InsertionMarkerPreviewer$$module$build$src$core$insertion_marker_previewer.hidePreview (insertion_marker_previewer.ts:218:14)
    at KeyboardDragStrategy.revertDrag (block_drag_strategy.ts:480:31)
    at BlockSvg$$module$build$src$core$block_svg.revertDrag (block_svg.ts:1782:23)

This appears to be due to the blocks having been reconfigured (back to their original configuration, presumably) before the insertion marker is deleted, and in particular that the block being dragged is no longer a top-level block on the workspace, but its SVG still matches the .blocklyDragging CSS selector, causing BlockSvg.prototype.setParent(null) to throw.

Moving the call to hidePreview earlier in revertDrag prevents this from happening—and is probably a good idea in any case—but the root cause would seem to be that the dragged block is somehow being reconnected (and thus no longer a top-level block) without having had its CSS classes updated.

[cpcallen]

OK, so the root problem is a bug in BlockSvg.prototype.setParent, which makes an unwarranted assumption that the block being dragged (in particular, the block upon which .setDragging(true) has been called), is a top-level block—which it normally will be, but at the start and end of a drag might not be true at all times.

I'm going to fix this incorrect assumption and also make some changes to revertDrag to do things in a slightly more sensible order.