keyboard nav monkey patches fields & toolbox

[maribethb]

Check for duplicates

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

Component

keyboard-navigation

Description

• In Blockly core, we have an IKeyboardAccessible interface, which has one method: onShortcut.
• The base Toolbox and Field classes implement this interface, but the default implementation is just return false aka don't handle any shortcuts.
• None of the fields/toolboxes in core override this default implementation.
• In blockly-samples, the keyboard-navigation plugin monkeypatches some fields and the toolbox class to add actual shortcut handlers directly to the prototype of the relevant objects.

This situation has a few problems:

• Monkeypatching is bad.
• Our Toolbox class is not required to be used. It's allowed that developers implement IToolbox instead and don't use our base class. If they do, they won't get the patched shortcut handler. The same situation applies for FieldDropdown (where a developer could have registered their own field in place of ours).
• The plugin attempts to patch Blockly.FieldColour which no longer exists. This doesn't crash at runtime because we do check to make sure it exists before trying to patch it, but it does emit build warnings. Filed remove references to Blockly.FieldColour from keyboard-navigation #2382 to track separately
• The old colour field was compatible with keyboard-nav (through above patching) but the new plugin version is not.

It's not straightforward to fix this. I think this concept was originally designed when keyboard navigation was going to be part of core. We can't simply move the shortcut handlers to core, because the handlers need to know which keyboard shortcut was pressed. The shortcuts themselves are defined from the keyboard-nav plugin. We cannot add a dependency from core to the keyboard-navigation plugin. Therefore we can't reference shortcuts that are defined in the plugin from core.

  fieldDropdownHandler(shortcut) {
    if (this.menu_) {
      switch (shortcut.name) {
        case Constants.SHORTCUT_NAMES.PREVIOUS: <-- HERE
          this.menu_.highlightPrevious();
          return true;
        case Constants.SHORTCUT_NAMES.NEXT: <-- HERE
          this.menu_.highlightNext();
          return true;
        default:
          return false;
      }
    }
    // If we haven't already handled the shortcut, let the default Field
    // handler try.
    return Blockly.Field.prototype.onShortcut.call(this, shortcut);
  }

As we see above, the onShortcut method will receive the KeyboardShortcut object, but in core we'd have no way to relate the name to the name defined in the plugin's Constants.

It might be theoretically possible to use optional peer dependencies for this problem, where we add keyboard-navigation as an optional peer dependency and check if it is installed before trying to use its Constants. Then we could move the onShortcut handlers to core. But peer dependencies in general are kind of a mess and I truly have no idea based on this discussion if optional peer dependencies are currently automatically installed by npm. It would be highly undesirable for the keyboard-navigation plugin to automatically be installed for everyone who installs blockly. And I'm loath to add any more dependencies between core and blockly-samples, even if they are optional. I'm slightly less loath to do this in samples itself, so this might be a reasonable way to solve the regression where the colour field is no longer keyboard navigable. But I think it'd be better to have a general solution/design for this instead.

Reproduction steps

Stack trace

No response

Screenshots

No response

[BeksOmega]

Is there a reason keyboard navigation needs to exist in plugins? Given the constraints here, plus the fact that we want to make it easy for folks to enable it, I think it makes more sense for it to be in core.

[maribethb]

That might be the end result, but keyboard navigation as a whole needs a new design doc before we decide that's the best solution imo.