feat: Use FocusManager for navigation & state management #482
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This removes nearly all custom focus/blur management and instead leverages FocusManager as the source of truth and primary system for focusing/selecting elements. There are still a bunch of issues with this branch, so more commits will be necessary before this is ready to be merged.
BenHenning
changed the title
Merged
1 task
Collaborator
Author
|
I still need to push my latest changes (currently working through some focus issues with variable creation), but now that block movement is working I'm pretty happy with the stability of this PR with regards to it being testable. It's definitely not yet ready for review, but I'm hopeful the delta won't be too large. |
This ensures block movement works again, and that the initial item selected when opening the flyout is correctly focused.
1 task
It no longer seems required.
BenHenning
commented
May 1, 2025
Collaborator
Author
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed changes.
Collaborator
Author
|
PTAL @rachel-fenichel for an initial review pass. If you can, I'd also really appreciate your check on this @microbit-matt-hillsdon as it makes substantial changes to focus management that you had carefully considered as part of #326. I think that I've matched parity here with the removed focus handlers, but I can't 100% certain for flyout (due to |
This is now in Core.
BenHenning
added a commit
to RaspberryPiFoundation/blockly
that referenced
this pull request
May 2, 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 #8940 Fixes #8954 Fixes #8955 ### Proposed Changes This updates `LineCursor` to use `FocusManager` rather than selection (principally) as the source of truth. ### Reason for Changes Ensuring that keyboard navigation works correctly with eventual screen reader support requires ensuring that ever navigated component is focused, and this is primarily what `FocusManager` has been designed to do. Since these nodes are already focused, `FocusManager` can be used as the primary source of truth for determining where the user currently has navigated, and where to go next. Previously, `LineCursor` relied on selection for this purpose, but selection is now automatically updated (for blocks) using focus-controlled `focus` and `blur` callbacks. Note that the cursor will still fall back to synchronizing with selection state, though this will be removed once the remaining work to eliminate `MarkerSvg` has concluded (which requires further consideration on the keyboard navigation side viz-a-viz styling and CSS decisions) and once mouse clicks are synchronized with focus management. Note that the changes in this PR are closely tied to RaspberryPiFoundation/blockly-keyboard-experimentation#482 as both are necessary in order for the keyboard navigation plugin to correctly work with `FocusManager`. Some other noteworthy changes: - Some special handling exists for flyouts to handle navigating across stacks (per the current cursor design). - `FocusableTreeTraverser` is needed by the keyboard navigation plugin (in RaspberryPiFoundation/blockly-keyboard-experimentation#482) so it's now being exported. - `FocusManager` had one bug that's now patched and tested in this PR: it didn't handle the case of the browser completely forcing focus loss. It would continue to maintain active focus even though no tracked elements now hold focus. One such case is the element being deleted, but there are other cases where this can happen (such as with dialog prompts). - `FocusManager` had some issues from #8909 wherein it would overeagerly call tree focus callbacks and slightly mismanage the passive node. Since tests haven't yet been added for these lifecycle callbacks, these cases weren't originally caught (per #8910). - `FocusManager` was updated to move the tracked manager into a static function so that it can be replaced in tests. This was done to facilitate changes to setup_teardown.js to ensure that a unique `FocusManager` exists _per-test_. It's possible for DOM focus state to still bleed across tests, but `FocusManager` largely guarantees eventual consistency. This change prevents a class of focus errors from being possible when running tests. - A number of cursor tests needed to be updated to ensure that a connections are properly rendered (as this is a requirement for focusable nodes, and cursor is now focusing nodes). One test for output connections was changed to use an input connection, instead, since output connections can no longer be navigated to (and aren't rendered, thus are not focusable). It's possible this will need to be changed in the future if we decide to reintroduce support for output connections in cursor, but it seems like a reasonable stopgap. Huge thanks to @rachel-fenichel for helping investigate and providing an alternative for the output connection test. **Current gaps** to be fixed after this PR is merged: - The flyout automatically closes when creating a variable with with keyboard or mouse (I think this is only for the keyboard navigation plugin). I believe this is a regression from previous behavior due to how the navigation plugin is managing state. It would know the flyout should be open and thus ensure it stays open even when things like dialog prompts try to close it with a blur event. However, the new implementation in RaspberryPiFoundation/blockly-keyboard-experimentation#482 complicates this since state is now inferred from `FocusManager`, and the flyout _losing_ focus will force it closed. There was a fix introduced in this PR to fix it for keyboard navigation, but fails for clicks because the flyout never receives focus when the create variable button is clicked. It also caused the advanced compilation tests to fail due to a subtle circular dependency from importing `WorkspaceSvg` directly rather than its type. - The flyout, while it stays open, does not automatically update past the first variable being created without closing and reopening it. I'm actually not at all sure why this particular behavior has regressed. ### Test Coverage No new non-`FocusManager` tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915. Some new `FocusManager` tests were added, but more are still needed and this is tracked as part of #8910. ### Documentation No new documentation should be needed for these changes. ### Additional Information This includes changes that have been pulled from #8875.
Use tab navigation instead of clicking for selection.
Collaborator
Author
|
The build & test failures were partly due to RaspberryPiFoundation/blockly#8941 not yet being merged, but it's merged now. There were still test updates needed which I will document shortly. |
BenHenning
commented
May 2, 2025
Collaborator
Author
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed changes.
Collaborator
Author
|
Thanks to the wonderful testing from @microbit-grace and @microbit-matt-hillsdon there are ~40 issues that have been found with this PR. However, I think we may still want to merge this as-is since, without these changes, the plugin is entirely unusable with tip-of-tree v12 (especially after RaspberryPiFoundation/blockly#8941 was merged). I think we can follow up with the needed fixes in Core and the plugin to address those problems, plus a number warrant additional discussion (especially around styling). I defer to you @rachel-fenichel on this. |
rachel-fenichel
approved these changes
May 2, 2025
Collaborator
rachel-fenichel
left a comment
rachel-fenichel
left a comment
There was a problem hiding this comment.
Let's merge this to move from "head is completely broken" to "head has broken flows that we can debug".
src/actions/enter.ts
Outdated
| Field, | ||
| FlyoutButton, | ||
| WorkspaceSvg, | ||
| getFocusManager, |
Collaborator
There was a problem hiding this comment.
Should not need to change all of the import types to imports just to add the focus manager.
Collaborator
Author
There was a problem hiding this comment.
Oops, I think I missed the type specifier and thought there were redundant import lines (which, in retrospect, I now realize would've been caught by the linter). Fixed in latest.
Accidentally moved to full imports in an earlier commit. This addresses a review comment.
BenHenning
commented
May 2, 2025
Collaborator
Author
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed latest fix.
Collaborator
Author
|
Going ahead and merging this as discussed above. Thanks for the review! |
1 task
BenHenning
added a commit
to RaspberryPiFoundation/blockly
that referenced
this pull request
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 RaspberryPiFoundation/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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #142
Fixes #481
This removes nearly all custom focus/blur management and instead leverages
FocusManageras the source of truth and primary system for focusing/selecting elements.This largely continues the work in Core starting with RaspberryPiFoundation/blockly#8938 and ending with RaspberryPiFoundation/blockly#8941 in order to bring basic parity in the keyboard navigation plugin using
FocusManager.Specifically:
focusNode/focusTreecalls viaFocusManager.Navigationnow infers state based on what's currently focused rather than tracking it separately.WorkspaceSvg'smarkFocused).FocusManager-drivenblocklyPassiveFocusCSS class exists for styling.Toolboxwill automatically synchronize its selected item state with its item that has active focus.ToolboxandFlyoutcooperate for automatically closing theFlyoutat the correct times.Toolboxnavigation was moved to using its methods directly instead of its event since selection logic now needs to be paired withfocusNode.Toolboxitem selection could auto-focus withinToolboxitself, or this could be done as part ofToolbox-specific navigation.Flyoutfocus could automatically focus its first item if nothing is currently focused.handleFocus*()functions, though there may be cases fromhandleFocusWorkspaceyet to check.The test updates were necessary because, at present, clicking does not force focus which means the tests need to rely purely on keys for navigation. I think these changes are actually an improvement since it closer aligns them with keyboard-only usage.
Current gaps that may need resolution prior to this PR being mergeable: