Error caused when you keyboard navigate to a block, cut (CMD+X), paste (CMD+V), then undo (CMD+Z) via keyboard shortcuts. #8971
Labels
issue: bug
Describes why the code or behaviour is wrong
issue: triage
Issues awaiting triage by a Blockly team member
Comments
1 task
BenHenning
added a commit
that referenced
this issue
May 5, 2025
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8952 Fixes #8950 Fixes #8971 Fixes a bunch of other keyboard + mouse synchronization issues found during the testing discussed in google/blockly-keyboard-experimentation#482 (comment). ### Proposed Changes This introduces a number of changes to: - Ensure that gestures which change selection state also change focus. - Ensure that ephemeral focus is more robust against certain classes of failures. ### Reason for Changes There are some ephemeral focus issues that can come up with certain actions (like clicking or dragging) don't properly change focus. Beyond that, some users will likely mix clicking and keyboard navigation, so it's essential that focus and selection state stay in sync when switching between these two types of navigation modalities. Other changes: - Drop-down div was actually incorrectly releasing ephemeral focus before animated closes finish which could reset focus afterwards, breaking focus state. - Both drop-down and widget divs have been updated to only return focus _after_ marking the workspace as focused since the last focused node should always be the thing returned. - In a number of gesture cases selection has been removed. This is due to `BlockSvg` self-managing its selection state based on focus, so focusing is sufficient. I've also centralized some of the focusing calls (such as putting one in `bringBlockToFront` to ensure focusing happens after potential DOM changes). - Similarly, `BlockSvg`'s own `bringToFront` has been updated to automatically restore focus after the operation completes. Since `bringToFront` can always result in focus loss, this provides robustness to ensure focus is restored. - Block pasting now ensures that focus is properly set, however a delay is needed due to additional rendering changes that need to complete (I didn't dig deeply into the _why_ of this). - Dragging has been updated to always focus the moved block if it's not in the process of being deleted. - There was some selection resetting logic removed from gesture's `doWorkspaceClick` function. As far as I can tell, this won't be needed anymore since blocks self-regulate their selection state now. - `FocusManager` has a new extra check for ephemeral focus to catch a specific class of failure where the browser takes away focus immediately after it's returned. This can happen if there are delay timing situations (like animations) which result in a focused node being deleted (which then results in the document body receiving focus). The robustness check is possibly not needed, but it help discover the animation issue with drop-down div and shows some promise for helping to fix the variables-closing-flyout problem. Some caveats: - Some undo/redo steps can still result in focus being lost. This may introduce some regressions for selection state, and definitely introduces some annoyances with keyboard navigation. More work will be needed to understand how to better redirect focus (and to what) in cases when blocks disappear. - There are many other places where focus is being forced or selection state being overwritten that could, in theory, cause issues with focus state. These may need to be fixed in the future. - There's a lot of redundancy with `focusNode` being called more than it needs to be. `FocusManager` does avoid calling `focus()` more than once for the same node, but it's possible for focus state to switch between multiple nodes or elements even for a single gesture (for example, due to bringing the block to the front causing a DOM refresh). While the eventual consistency nature of the manager means this isn't a real problem, it may cause problems with screen reader output in the future and warrant another pass at reducing `focusNode` calls (particularly for gestures and the click event pipeline). ### Test Coverage This PR is largely relying on existing tests for regression verification, though no new tests have been added for the specific regression cases. #8910 is tracking improving `FocusManager` tests which could include some cases for the new ephemeral focus improvements. #8915 is tracking general accessibility testing which could include adding tests for these specific regression cases. ### Documentation No new documentation is expected to be needed here. ### Additional Information These changes originate from both #8875 and from a branch @rachel-fenichel created to investigate some of the failures this PR addresses. These changes have also been verified against both Core's playground and the keyboard navigation plugin's test environment.
|
Should be addressed with #8981. |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Check for duplicates
Description
Error caused when you keyboard navigate to a block, cut (CMD+X), paste (CMD+V), then undo (CMD+Z) via keyboard shortcuts:
Ben: unsure about this one. The stack trace is suggesting that undo is repeating the deletion which is causing the cursor to attempt to focus an old block that's no longer in the workspace. That's my interpretation of what would cause this failure, but it may be something else. The '???' is particularly interesting since that's the node being passed to focusNode. This is a particularly excellent find.
Reproduction steps
Stack trace
Screenshots
No response
Browsers
No response
The text was updated successfully, but these errors were encountered: