Skip to content

Conversation

@BenHenning
Copy link
Contributor

@BenHenning BenHenning commented Apr 22, 2025

The basics

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: Inputs 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 Inputs 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.

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.
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.
@BenHenning BenHenning changed the base branch from develop to rc/v12.0.0 April 22, 2025 21:34
@github-actions github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: feature Adds a feature labels Apr 22, 2025
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.
@BenHenning BenHenning changed the title feat!: Make fields focusable feat: Make fields focusable Apr 24, 2025
@BenHenning BenHenning changed the title feat: Make fields focusable feat: Make fields focusable [Blocked on: #8920] Apr 24, 2025
The CSS work here is far more extensive than should go in this or any of
the structural branches. Instead, proper rendering will be done via the
plugin and eventually merged into Core once finalized.
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
Copy link
Contributor Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-reviewed changes not included from #8939.

Addresses self-review comment.
@BenHenning BenHenning marked this pull request as ready for review April 30, 2025 02:02
@BenHenning BenHenning requested a review from a team as a code owner April 30, 2025 02:02
@BenHenning
Copy link
Contributor Author

PTAL @rachel-fenichel. Feel free to use BenHenning#5 to see the exact diff with #8939 which will hopefully make the review easier.

@rachel-fenichel
Copy link
Collaborator

LGTM--the exact diff is not that large and matches the designed behaviour.

@BenHenning BenHenning changed the title feat: Make fields focusable [Blocked on: #8939] feat: Make fields focusable Apr 30, 2025
Copy link
Contributor Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review confirming this has the same changes as BenHenning#5.

@BenHenning
Copy link
Contributor Author

See #8938 (comment) for why this is being merged now.

@BenHenning BenHenning merged commit f68081b into RaspberryPiFoundation:rc/v12.0.0 Apr 30, 2025
7 of 9 checks passed
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 #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 `LineCursor` and the keyboard navigation plugin). This is a largely isolated change in how it fundamentally works:
- `RenderedConnection` has been updated to be an `IFocusableNode` using a new unique ID maintained by `Connection` and automatically enabling/disabling the connection highlight based on whether it's focused (per keyboard navigation).
- The way that rendering works here has changed: rather than recreating the connection's highlight SVG each time, it's only created once and updated thereafter to ensure that it correctly fits block resizes or movements. Visibility of the highlight is controlled entirely through display visibility and can now be done synchronously (which was a requirement for focusability as only displayed elements can be focused).
- This employs the same type of ID schema strategy as fields in #8923.

### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: feature Adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Arbitrary modals (such as the confirmation dialog when deleting a variable that is used in multiple places). Fields (may require further break-down).

2 participants