fix: Ensure cursor syncs with more than just focused blocks #9032
Conversation
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed changes.
|
@gonfunko I'm curious whether you have any thoughts on this PR, as well, since it's largely touching on navigable things (which may also change with your plans to maybe remove |
|
Will look in depth later, but just posted #9033 which almost merges the two, leaving INavigable solely for |
|
INavigable is gone in favor of IFocusableNode in #9037 |
|
Will update this once #9037 is merged since it will need to be essentially completely redone (but should be very straightforward to do). |
…t-focused-blocks Conflicts: core/interfaces/i_navigable.ts core/keyboard_nav/line_cursor.ts
This is also a bit simpler than the previous approach now that INavigable has been removed.
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed the newer version of this that doesn't use INavigable.
|
FYI I will test this once RaspberryPiFoundation/blockly-keyboard-experimentation#520 is merged since I can't verify this against the current keyboard navigation plugin. However, I don't expect there to be any changes in behavior after #9037 being merged. |
|
Verified against the latest tip-of-tree keyboard navigation plugin and this seems to be working well. Going ahead and merging. |
BenHenning
deleted the
ensure-cursor-syncs-with-more-than-just-focused-blocks
branch
3 participants
The basics
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
LineCursorhas 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 onIFocusableNode) which meansLineCursorshould 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 tosetCurNodewhich 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:
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
LineCursorto 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).