Investigate why calling field.getFocusableElement().focus() doesn't update the cursor correctly #499
Comments
|
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 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 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 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 |
|
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 |
|
The failing tests can be fixed in a different way, Ben has some suggestions above related to using |
|
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. |
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes google/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).
|
Fixed in google/blockly#9032. It should now be possible to use |
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes google/blockly-keyboard-experimentation#526 ### Proposed Changes This limits synchronizing in `LineCursor` from `FocusManager` to just nodes that have a corresponding block. ### Reason for Changes Limiting the synchronizing in this way ensures that navigation can't enter a bad state. The reason for why this is needed is explained in google/blockly-keyboard-experimentation#526 (comment). Longer term it would maybe be ideal to do one or both of the following: - Figure out ways of making navigation a bit more robust (perhaps on the keyboard navigation side) such that if cursor _is_ in a bad state there's some way to recover (rather than ending up permanently broken). - Remove `Marker`'s internal state in favor of always relying on `FocusManager`'s state to cover the cases where there can be automatic focus shifting. ### Test Coverage This was manually tested with the keyboard navigation plugin and verified to ensure that both google/blockly-keyboard-experimentation#526 and google/blockly-keyboard-experimentation#499 are (still) working as expected. Some basic testing was done with the core simple playground with the developer console open to ensure there weren't any expected failures. Automated testing cases would be better addressed as part of resolving #8915. ### Documentation No new documentation is needed here. ### Additional Information This behavior is expected to only affect the keyboard navigation plugin.
Check for duplicates
Description
This is causing tests that depend on programmatically focusing on a field to fail.
Reproduction steps
Stack trace
Screenshots
No response
Browsers
No response
The text was updated successfully, but these errors were encountered: