fix: Ensure cursor syncs with more than just focused blocks (#9032)

## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes https://github.com/google/blockly-keyboard-experimentation/issues/499

### Proposed Changes

This ensures that non-blocks which hold active focus correctly update `LineCursor`'s internal state.

### Reason for Changes

This is outright a correction in how `LineCursor` has worked up until now, and is now possible after several recent changes (most notably #9004). #9004 updated selection to be more explicitly generic (and based on `IFocusableNode`) which means `LineCursor` should also properly support more than just blocks when synchronizing with focus (in place of selection), particularly since lots of non-block things can be focusable.

What's interesting is that this change isn't strictly necessary, even if it is a reasonable correction and improvement in the robustness of `LineCursor`. Essentially everywhere navigation is handled results in a call to `setCurNode` which correctly sets the cursor's internal state (with no specific correction from focus since only blocks were checked and we already ensure that selecting a block correctly focuses it).

### Test Coverage

It would be nice to add test coverage specifically for the cursor cases, but it seems largely unnecessary since:
1. The main failure cases are test-specific (as mentioned above).
2. These flows are better left tested as part of broader accessibility testing (per #8915).

This has been tested with a cursory playthrough of some basic scenarios (block movement, insertion, deletion, copy & paste, context menus, and interacting with fields).

### Documentation

No new documentation should be needed here.

### Additional Information

This is expected to only affect keyboard navigation plugin behaviors, particularly plugin tests.

It may be worth updating `LineCursor` to completely reflect current focus state rather than holding an internal variable. This, in turn, may end up simplifying solving issues like #8793 (but not necessarily).
This commit is contained in:
Ben Henning
2025-05-14 10:50:00 -07:00
committed by GitHub
parent 2b9d06ac99
commit e1179808fd

View File

@@ -374,7 +374,15 @@ export class LineCursor extends Marker {
* @returns The current field, connection, or block the cursor is on.
*/
override getCurNode(): IFocusableNode | null {
this.updateCurNodeFromFocus();
// Ensure the current node matches what's currently focused.
const focused = getFocusManager().getFocusedNode();
const block = this.getSourceBlockFromNode(focused);
if (!block || block.workspace === this.workspace) {
// If the current focused node corresponds to a block then ensure that it
// belongs to the correct workspace for this cursor.
this.setCurNode(focused);
}
return super.getCurNode();
}
@@ -401,20 +409,6 @@ export class LineCursor extends Marker {
}
}
/**
* Updates the current node to match what's currently focused.
*/
private updateCurNodeFromFocus() {
const focused = getFocusManager().getFocusedNode();
if (focused instanceof BlockSvg) {
const block: BlockSvg | null = focused;
if (block && block.workspace === this.workspace) {
this.setCurNode(block);
}
}
}
/**
* Get the first navigable node on the workspace, or null if none exist.
*