feat!: Introduce new focus tree/node functions. #8909
Merged
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 introduces new callback methods for IFocusableTree and IFocusableNode for providing a basis of synchronizing domain state with focus changes. It also introduces support for implementations of IFocusableTree to better manage initial state cases, especially when a tree is focused using tab navigation. FocusManager has also been updated to ensure functional parity between tab-navigating to a tree and using focusTree() on that tree. This means that tab navigating to a tree will actually restore focus back to that tree's previous focused node rather than the root (unless the root is navigated to from within the tree itself). This is meant to provide better consistency between tab and non-tab keyboard navigation. Note that these changes originally came from google#8875 and are required for later PRs that will introduce IFocusableNode and IFocusableTree implementations.
BenHenning
commented
Apr 21, 2025
These were needed in previous versions of plugin changes, but aren't anymore.
BenHenning
commented
Apr 21, 2025
|
PTAL @rachel-fenichel. |
This reverts commit 404c20e.
BenHenning
commented
Apr 21, 2025
1 task
BenHenning
commented
Apr 22, 2025
1 task
rachel-fenichel
approved these changes
Apr 22, 2025
| } | ||
| }; | ||
| } | ||
|
|
||
| private ensureManagerIsUnlocked(): void { |
There was a problem hiding this comment.
Add function comments on private functions (here and below) for future readers.
There was a problem hiding this comment.
I usually never add private methods documentation (since usually the code + line comments are sufficient), but happy to add them!
BenHenning
commented
Apr 23, 2025
| } | ||
| }; | ||
| } | ||
|
|
||
| private ensureManagerIsUnlocked(): void { |
There was a problem hiding this comment.
I usually never add private methods documentation (since usually the code + line comments are sufficient), but happy to add them!
|
Going ahead and merging since I suspect the private method doc changes will be uncontroversial. |
BenHenning
added a commit
to BenHenning/blockly
that referenced
this pull request
Apr 30, 2025
This exposes FocusableTreeTraverser for the plugin's use. It also fixes a few callback issues with FocusManager that were missed in google#8909.
1 task
BenHenning
added a commit
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 google/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 google/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 google/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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The basics
The details
Resolves
Fixes #8770
Fixes part of #8771
Proposed Changes
This introduces new callback methods for IFocusableTree and IFocusableNode for providing a basis of synchronizing domain state with focus changes. It also introduces support for implementations of IFocusableTree to better manage initial state cases, especially when a tree is focused using tab navigation.
FocusManager has also been updated to ensure functional parity between tab-navigating to a tree and using focusTree() on that tree. This means that tab navigating to a tree will actually restore focus back to that tree's previous focused node rather than the root (unless the root is navigated to from within the tree itself). This is meant to provide better consistency between tab and non-tab keyboard navigation.
The
isFocusableNode/isFocusableTreemethods are type coercion functions needed eventually byLineCursor(at least, checking whether something is a node is needed).Note that this PR includes a fix for
FocusManager.unregisterTree()--it wasn't actually correctly removing the specified tree but, rather, always removing the first registered tree. This was likely introduced after a few back-and-forth implementations with using a wrapper registration class instead of the tree directly, and this spot was missed. A new test was added to specifically catch this issue (and was verified to fail without the fix in place).Reason for Changes
#8875 demonstrates future changes that will depend on the changes introduced in this PR. Much of this functionality is needed to ensure an easier time updating Workspace, Toolbox, and Flyout to be focusable trees along with their constituent nodes.
Test Coverage
These changes have been manually tested as part of the development work in #8875. Since the new functionality is largely callback related, it isn't being tested in this PR. However, given how critical FocusManager is, it's important to ensure eventual coverage.
#8910 is tracking updating the tests as these can be added after v12 is cut, and it's not expected that these new tests will find many problems with the implementation changes.
Documentation
No documentation changes should be needed here beyond the ones already introduced with JS docs.
Additional Information
Note that these changes originally came from #8875 and are required for later PRs that will introduce IFocusableNode and IFocusableTree implementations.