Investigate why calling field.getFocusableElement().focus() doesn't update the cursor correctly

[RoboErikG]

Check for duplicates

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

Description

This is causing tests that depend on programmatically focusing on a field to fail.

Reproduction steps

1. Open up the console and set up a field variable that points at some field in the workspace.
2. In console use setTimeout(() => {field.getFocusableElement().focus()}, 5000);
3. Click on a different block than the one that will get focused
4. After focus moves to the field from the timeout, press up on the keyboard
5. See the focus jump back to where it was after clicking (moved up one)

Stack trace

Screenshots

No response

Browsers

No response

[BenHenning]

So I looked into this, and I'm fairly certain what's happening is that focus state is definitely updating correctly, but the cursor doesn't represent this because it filters out all non-block focusables. That's obviously wrong and is from previous selection-syncing logic. We don't really see this manifest as a real problem normally because the cursor itself (or navigation logic now) will explicit set the exact next thing to go to, but this means that DOM-driven cursor changes like the one mentioned in this issue won't work.

This is a problem for all non-blocks that can be navigated to. I would say this is a non-priority to fix as it realistically should only ever effect code or tests, and code calling focus() directly probably should use FocusManager's focusNode, anyway.

For tests, it would be preferable to navigate to the field using keyboard navigation in the same way as a human (which would actually test the full flows rather than skip them like this).

That being said, it would be more correct to ensure the cursor state properly reflects the FocusManager. Doing that would require this update to LineCursor (as of RaspberryPiFoundation/blockly#9004):

  private updateCurNodeFromFocus() {
    const focused = getFocusManager().getFocusedNode();

    if (isNavigable(focused)) {
      // TODO: Is this needed? Is it needed for non-blocks?
      if (focused instanceof BlockSvg && focused.workspace !== this.workspace) {
        return;
      }
      this.setCurNode(focused);
    }
  }

(Note that this requires adding an isNavigable function to i_navigable.ts).

I'm unsure if the existing workspace check is really necessary. I think it logically is for the multiple workspaces situation, but it's tricky because there's no top-level way to extract a workspace from either an IFocusableNode or an INavigable.

[maribethb]

Hopefully also a non-issue also if we get rid of the cursor keeping track of its current node anyway, since it can just ask the focus manager what's focused

[maribethb]

The failing tests can be fixed in a different way, Ben has some suggestions above related to using focusNode instaed of focus

[BenHenning]

Sorry that's not an exact fix, that was actually just a separate thought since using DOM focus directly is a bit messier (but roughly the same behavior).

The issue is we'll need to change cursor behavior in order to sync to more than just blocks to properly fix focusing non-block nodes, otherwise we can only use keyboard navigation for navigating between things in tests.

[BenHenning]

Fixed in RaspberryPiFoundation/blockly#9032. It should now be possible to use .focus() or focusNode() to correctly focus a field for continued cursor navigation.