Keyboard navigation not functional after reloading blocks

[microbit-matt-hillsdon]

If you modify the test app like this to handle the scenario selector client side, then you hit a problem.

1. Load the modified page (deployed here)
2. Use keyboard navigation to move the cursor onto a block
3. Use the scenario selector to load blocks
4. Try to use keyboard navigation again and note it doesn't highlight anything

This corresponds to a real scenario in MakeCode. When MakeCode switches from JavaScript to Blocks mode it reloads (via XML) blocks into Blockly. They may or may not have changed at this point due to edits in JavaScript mode. From that point on the keyboard navigation is broken.

You can recover navigation by navigating to the toolbox with "T" and adding a new block.

Is this something the plugin can be modified to cope with?

[rachel-fenichel]

Debugging notes:
The failure here is that the keyboard navigation code is using the same cursor it had before, and the cursor is pointing to a no-longer-extant-node. In my test case, the stack trace was

Cannot read properties of null (reading 'fieldRow')
TypeError: Cannot read properties of null (reading 'fieldRow')
    at ASTNode$$module$build$src$core$keyboard_nav$ast_node.findNextForField (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1026:163)
    at ASTNode$$module$build$src$core$keyboard_nav$ast_node.next (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1035:83)
    at LineCursor.getNextNode (webpack-internal:///./src/line_cursor.ts:297:43)
    at LineCursor.in (webpack-internal:///./src/line_cursor.ts:58:30)
    at Object.callback (webpack-internal:///./src/navigation_controller.ts:177:104)
    at ShortcutRegistry$$module$build$src$core$shortcut_registry.onKeyDown (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1280:193)
    at onKeyDown$$module$build$src$core$inject (webpack-internal:///./node_modules/blockly/blockly_compressed.js:423:269)
    at HTMLDocument.f (webpack-internal:///./node_modules/blockly/blockly_compressed.js:86:159)

[rachel-fenichel]

workspaceSvg in core has

  /**
   * Render all blocks in workspace.
   */
  render() {
    // Generate list of all blocks.
    const blocks = this.getAllBlocks(false);
    // Render each block.
    for (let i = blocks.length - 1; i >= 0; i--) {
      blocks[i].queueRender();
    }

    this.getTopBlocks()
      .flatMap((block) => block.getDescendants(false))
      .filter((block) => block.isInsertionMarker())
      .forEach((block) => block.queueRender());

    renderManagement
      .finishQueuedRenders()
      .then(() => void this.markerManager.updateMarkers());
  }

Which means that it's actually calling markerManager.updateMarkers() at a reasonable time. Unfortunately, updateMarkers doesn't do anything to check validity before redrawing:

  /**
   * Redraw the attached cursor SVG if needed.
   *
   * @internal
   */
  updateMarkers() {
    if (this.workspace.keyboardAccessibilityMode && this.cursorSvg) {
      this.workspace.getCursor()!.draw();
    }
  }

[cpcallen]

I think this will probably either resolve itself or at least get fixed as part of the work we're planning to do in core to have Blockly understand and use focus (vs. selection), but for the moment we need to do something to work around this issue.

Probably the most general fix for this (in the short term) is to add a check LineCursor.prototype.getCurNode to check that the current node is valid (and choose a new one if it's not)—along the lines of the hack added in #178 to make click-to-focus work(ish).