feat: Make connections focusable #8928
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 adds basic support for WorkspaceSvg being focusable via its blocks, though fields and connections are not yet supported.
This introduces the fundamental support needed to ensure that both toolboxes and flyouts are focusable using FocusManager.
…-workspace-focusable
This ensures that fields with editors properly signal when the editor is shown/hidden, and introduces show/hide callbacks that can be overridden to properly trigger ephemeral focus.
Note that this doesn't yet contain all of the changes needed in order to ensure that this works correctly.
…-workspace-focusable
Addresses review comment.
Addresses reviewer comment.
This is a more general purpose alternative to making fields explicitly focusable (at least making them hook up properly to ephemeral focus).
With widget and drop down divs focusable directly, all field editors should automatically inherit this benefit.
Also, remove unnecessary casting.
There's a lot of clean-up and simplification work needed yet.
Addresses a reviewer comment.
This essentially makes the underlying highlight path permanent so that only its transform/d attributes need to be updated, plus its display property.
It needed a unique ID and to be properly hooked up to node retrieval.
Addresses self-review comment.
…ke-fields-focusable Conflicts: core/workspace_svg.ts
Addresses self-review comment.
BenHenning
commented
Apr 30, 2025
Contributor
Author
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed changes not overlapping with #8923.
This addresses a self-review comment.
Contributor
Author
|
PTAL @rachel-fenichel. FYI that this does include a few changes over #8920 in order to make @RoboErikG feel free to take a pass, but I believe Rachel plans to review the entire chain of PRs up to #8941. |
BenHenning
commented
Apr 30, 2025
…x-and-flyout-focusable-roll-forward
…ke-fields-focusable
BenHenning
added a commit
that referenced
this pull request
Apr 30, 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 #8922 Fixes #8929 Fixes part of #8771 ### Proposed Changes This PR introduces support for fields to be focusable (and thus navigable with keyboard navigation when paired with downstream changes to `LineCursor` and the keyboard navigation plugin). This is a largely isolated change in how it fundamentally works: - `Field` was updated to become an `IFocusableNode`. Note that it uses a specific string-based ID schema in order to ensure that it can be properly linked back to its unique block (which helps make the search for the field in `WorkspaceSvg` a bit more efficient). This could be done with a globally unique ID, instead, but all fields would need to be searched vs. just those for the field's parent block. - The drop-down and widget divs have been updated to manage ephemeral focus with `FocusManager` when they're open for non-system dialogs (ephemeral focus isn't needed for system dialogs/prompts since those already take/restore focus in a native way that `FocusManager` will respond to--this may require future work, however, if the restoration causes unexpected behavior for users). This approach was done due to a suggestion from @maribethb as the alternative would be a more complicated breaking change (forcing `Field` subclasses to properly manage ephemeral focus). It may still be the case that certain cases will need to do so, but widget and drop-down divs seem to address the majority of possibilities. **Important**: `Input`s are not explicitly being supported here. As far as I can tell, we can't run into a case where `LineCursor` tries to set an input node, though perhaps I simply haven't come across this case. Supporting `Fields` and `Connections` (per #8928) seems to cover the main needed cases, though making `Input`s focusable may be a future requirement. ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). Note that #8929 isn't broadly addressed since making widget & drop down divs manage ephemeral focus directly addresses a large class of cases. Additional cases may arise where a plugin or Blockly integration may require additional effort to make keyboard navigation work for their field--this may be best addressed with documentation and guidance. ### Test Coverage No new 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. ### Documentation No new documentation is planned, however it may be prudent to update the field documentation in the future to explain how to utilize ephemeral focus when specifically building compatibility for keyboard navigation. ### Additional Information This includes changes that have been pulled from #8875.
BenHenning
changed the title
BenHenning
commented
Apr 30, 2025
Contributor
Author
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-review to confirm that this seems to contain the same changes as BenHenning#3.
rachel-fenichel
approved these changes
Apr 30, 2025
Contributor
Author
|
See #8938 (comment) for why this is being merged now. |
BenHenning
merged commit 0cbcc31
into
RaspberryPiFoundation:rc/v12.0.0
7 of 9 checks passed
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
The basics
The details
Resolves
Fixes #8930
Fixes part of #8771
Proposed Changes
This PR introduces support for connections to be focusable (and thus navigable with keyboard navigation when paired with downstream changes to
LineCursorand the keyboard navigation plugin). This is a largely isolated change in how it fundamentally works:RenderedConnectionhas been updated to be anIFocusableNodeusing a new unique ID maintained byConnectionand automatically enabling/disabling the connection highlight based on whether it's focused (per keyboard navigation).Reason for Changes
This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin).
Test Coverage
No new 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.
Documentation
No documentation changes should be needed here.
Additional Information
This includes changes that have been pulled from #8875.