Skip to content

Commit e117980

Browse files
authored
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 RaspberryPiFoundation/blockly-keyboard-experimentation#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).
1 parent 2b9d06a commit e117980

File tree

1 file changed

+9
-15
lines changed

1 file changed

+9
-15
lines changed

core/keyboard_nav/line_cursor.ts

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,15 @@ export class LineCursor extends Marker {
374374
* @returns The current field, connection, or block the cursor is on.
375375
*/
376376
override getCurNode(): IFocusableNode | null {
377-
this.updateCurNodeFromFocus();
377+
// Ensure the current node matches what's currently focused.
378+
const focused = getFocusManager().getFocusedNode();
379+
const block = this.getSourceBlockFromNode(focused);
380+
if (!block || block.workspace === this.workspace) {
381+
// If the current focused node corresponds to a block then ensure that it
382+
// belongs to the correct workspace for this cursor.
383+
this.setCurNode(focused);
384+
}
385+
378386
return super.getCurNode();
379387
}
380388

@@ -401,20 +409,6 @@ export class LineCursor extends Marker {
401409
}
402410
}
403411

404-
/**
405-
* Updates the current node to match what's currently focused.
406-
*/
407-
private updateCurNodeFromFocus() {
408-
const focused = getFocusManager().getFocusedNode();
409-
410-
if (focused instanceof BlockSvg) {
411-
const block: BlockSvg | null = focused;
412-
if (block && block.workspace === this.workspace) {
413-
this.setCurNode(block);
414-
}
415-
}
416-
}
417-
418412
/**
419413
* Get the first navigable node on the workspace, or null if none exist.
420414
*

0 commit comments

Comments
 (0)