Commit Graph

4677 Commits

Author SHA1 Message Date
Ben Henning
4f3eadef33 fix: Update focusNode to self correct focus (#9082)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes https://github.com/google/blockly-keyboard-experimentation/issues/87

### Proposed Changes

This updates `FocusManager.focusNode()` to automatically defocus its internal state if it detects that DOM focus (per `document.activeElement`) doesn't match its own internal focus.

It also updates `FocusManager` to avoid duplicate self calls to `focusNode()`.

### Reason for Changes

This is a robustness improvement for `focusNode` that is nice to keep as a "if all else fails" mechanism, but it's currently a hacky workaround to https://github.com/google/blockly-keyboard-experimentation/issues/87. #9081 is tracking introducing a long-term fix for the desynchronizing problem, but that's likely to be potentially much harder to solve and this at least introduces a reasonable correction.

From a stability perspective, it seems likely that there are multiple classes of failures covered by this fix. Essentially the browser behavior difference in Firefox and Safari over Chrome is that the former do not fire a focus change event when a focused element is removed from the DOM (leading to `FocusManager` getting out of sync). There may be other such cases when a focus event isn't fired, so this robustness improvement at least ensures eventual consistency so long as `focusNode()` is called (and, fortunately, that's done a lot now).

While this is a nice robustness improvement, it's not a perfect replacement for a real fix. For the time between `FocusManager` getting out of sync and `focusNode` getting called, `getFocusedNode` will _not_ match the actual element holding focus. The primary class of issues known is when a DOM element is being moved, and in these cases `focusNode` _is_ called. If there are other such unknown cases where a desync can happen, **`getFocusedNode()` will remain wrong until a later `focusNode()` call**.

Note one other change: originally implemented but removed in earlier PRs for `FocusManager`, this change also includes ensuring `focusNode()` isn't called multiple times for a single request to focus a node. Current logic results in a call to `focusNode()` calling a node's `focus()` which then processes a second call to `focusNode()` (which is fully executed because `FocusManager.focusedNode` isn't updated until after the call to `focus()`). This doesn't actually correct any state, but it's more efficient and provides some resilience against potential logic issues from calling node/tree callbacks multiple times (which was observed up to 3 times in some cases).

### Test Coverage

This has been tested via the keyboard navigation experimental plugin's test environment (with Firefox), plus new unit tests. Note the new test for directly verifying desyncing logic is contrived, but it should be perfectly testing the exact scenario that's being observed on Firefox/Safari. A separate test was added for the existing behavior of focusing a different node still correcting `FocusManager` state even if it was desynced (the bug only happens for the same node being refocused).

New tests were also added for the various lifecycle callbacks (to ensure they aren't called multiple times).

All of the new tests were verified to fail without the two fixes in place (they were verified in isolation), minus the test for focusing a second node when desynced (since that should pass regardless of the new fixes).

Some basic simple playground testing was done, as well, just to verify nothing obvious was broken around selection, gestures, and copy/paste.

### Documentation

No new documentation should be needed here.

### Additional Information

This wasn't explicitly tested in Safari since I only have access to Chrome and Firefox, but I will ask someone else on the team to verify this for me after merging if it isn't checked sooner.
2025-05-22 09:40:32 -07:00
RoboErikG
e4d7245e86 fix: Visit all nodes in getNextSibling and getPreviousSibling (#9080)
* WIP on line by line navigation

Doesn't work, likely due to isValid check.

* Add all inputs to the list of siblings

* Fix formatting

* Add tests

* Remove dupe keys
2025-05-21 16:42:22 -07:00
Maribeth Moffatt
4f01c9937a fix: focus after drag and deleting comments (#9074)
* fix: focus after drag and deleting comments

* chore: fix import
2025-05-20 11:58:05 -07:00
Aaron Dodson
53d7876539 feat: Add keyboard navigation support for icons. (#9072)
* feat: Add keyboard navigation support for icons.

* chore: Satisfy the linter.
2025-05-20 08:52:18 -07:00
Maribeth Moffatt
135da402ef fix: focus something after deleting a block (#9073) 2025-05-19 17:18:38 -07:00
Ben Henning
91632a4861 fix: Limit LineCursor<-focus syncing. (#9062)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes https://github.com/google/blockly-keyboard-experimentation/issues/526

### Proposed Changes

This limits synchronizing in `LineCursor` from `FocusManager` to just nodes that have a corresponding block.

### Reason for Changes

Limiting the synchronizing in this way ensures that navigation can't enter a bad state. The reason for why this is needed is explained in https://github.com/google/blockly-keyboard-experimentation/issues/526#issuecomment-2885117998.

Longer term it would maybe be ideal to do one or both of the following:
- Figure out ways of making navigation a bit more robust (perhaps on the keyboard navigation side) such that if cursor _is_ in a bad state there's some way to recover (rather than ending up permanently broken).
- Remove `Marker`'s internal state in favor of always relying on `FocusManager`'s state to cover the cases where there can be automatic focus shifting.

### Test Coverage

This was manually tested with the keyboard navigation plugin and verified to ensure that both https://github.com/google/blockly-keyboard-experimentation/issues/526 and https://github.com/google/blockly-keyboard-experimentation/issues/499 are (still) working as expected. Some basic testing was done with the core simple playground with the developer console open to ensure there weren't any expected failures.

Automated testing cases would be better addressed as part of resolving #8915.

### Documentation

No new documentation is needed here.

### Additional Information

This behavior is expected to only affect the keyboard navigation plugin.
2025-05-19 10:16:38 -07:00
RoboErikG
3010ceee2c fix: Skip hidden fields when navigating (#9070) 2025-05-19 09:47:16 -07:00
RoboErikG
7d0414c5dd fix: When moving to a field, scroll the field's block into view (#9071)
* fix: When moving to a field, scroll the field's block into view

* fix formatting
2025-05-19 09:35:59 -07:00
Ben Henning
3a53af903c fix: Ensure FieldImage is clickable when appropriate (#9063)
* fix: Ensure FieldImage is clickable when valid.

* chore: Add new tests for FieldImage.isClickable().
2025-05-16 10:58:56 -07:00
Aaron Dodson
ad0563daf7 fix: Make clickable but non-editable fields navigable. (#9054) 2025-05-15 09:15:07 -07:00
Aaron Dodson
215fad8676 fix: Remove un-typesafe cast. (#9052) 2025-05-14 15:45:02 -07:00
Ben Henning
083329aaa5 feat: Add support for conditional ephemeral focus. (#9051)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Needed for fixing https://github.com/google/blockly-samples/issues/2514 and https://github.com/google/blockly-samples/issues/2515.

### Proposed Changes

Update `FieldInput` along with drop-down and widget divs to support disabling the automatic ephemeral focus functionality.

### Reason for Changes

As mentioned in https://github.com/google/blockly-samples/issues/2514#issuecomment-2881539117 both https://github.com/google/blockly-samples/issues/2514 and https://github.com/google/blockly-samples/issues/2515 were caused by the custom fields leading to both drop-down and widget divs simultaneously taking ephemeral focus (and that's not currently allowed by `FocusManager`). This change updates both widget and drop-down divs with _optional_ parameters to conditionally disable automatic ephemeral focus so that `FieldInput` can, in turn, be customized with disabling automatic ephemeral focus for its inline editor. Being able to disable ephemeral focus for `FieldInput` allows the custom fields' own drop-down divs to take and manage ephemeral focus, instead, avoiding the duplicate scenario that led to the runtime failure.

Note that the drop-down div change in this PR is completely optional, but it's added for consistency and to avoid future scenarios of breakage when trying to use both divs together (as a fix is required in Core without monkeypatching).

It's worth noting that there could be a possibility for a more 'proper' fix in `FocusManager` by allowing multiple calls to `takeEphemeralFocus`, but it's unclear exactly how to solve this consistently (which is why it results in a hard failure today). The main issue is that the current focused node can change from underneath the manager (due to DOM focus changes), and the current focused element may also change. It's not clear if the first, last, or some other call to `takeEphemeralFocus` should take precedent or which node to return focus once ephemeral focus ends (in cases with multiple subsequent calls).

### Test Coverage

No new tests were added, though common field cases were tested manually in core's simple playground and in the plugin-specific playgrounds (per the original regressions). The keyboard navigation plugin test environment was also verified to ensure that this didn't alter any existing behavior (it should be a no-op except for the two custom field plugins).

Automated tests would be nice to add for all three classes, perhaps as part of #8915.

### Documentation

The code documentation changes here should be sufficient.

### Additional Information

These changes are being done directly to ease solving the above samples issues. See https://github.com/google/blockly-samples/pull/2521 for the follow-up fixes to samples.
2025-05-14 15:35:07 -07:00
Aaron Dodson
4a2b743f10 fix: Fix bug when referencing HTMLElement in non-browser environments. (#9050) 2025-05-14 14:50:23 -07:00
Maribeth Moffatt
205ef6c7d7 fix!: deepMerge for arrays, shortcut keycodes returned as array (#9047) 2025-05-14 12:23:12 -07:00
Maribeth Moffatt
523dca92bd fix: fieldDropdown.getText works in node (#9048)
* fix: dropdown getText works in node

* chore: comment
2025-05-14 12:22:09 -07:00
Aaron Dodson
7a7fad45c1 fix: Reenable support for tabbing between fields. (#9049)
* fix: Reenable support for tabbing between fields.

* refactor: Reduce code duplication.
2025-05-14 11:23:12 -07:00
Ben Henning
e1179808fd fix: Ensure cursor syncs with more than just focused blocks (#9032)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes https://github.com/google/blockly-keyboard-experimentation/issues/499

### Proposed Changes

This ensures that non-blocks which hold active focus correctly update `LineCursor`'s internal state.

### Reason for Changes

This is outright a correction in how `LineCursor` has worked up until now, and is now possible after several recent changes (most notably #9004). #9004 updated selection to be more explicitly generic (and based on `IFocusableNode`) which means `LineCursor` should also properly support more than just blocks when synchronizing with focus (in place of selection), particularly since lots of non-block things can be focusable.

What's interesting is that this change isn't strictly necessary, even if it is a reasonable correction and improvement in the robustness of `LineCursor`. Essentially everywhere navigation is handled results in a call to `setCurNode` which correctly sets the cursor's internal state (with no specific correction from focus since only blocks were checked and we already ensure that selecting a block correctly focuses it).

### Test Coverage

It would be nice to add test coverage specifically for the cursor cases, but it seems largely unnecessary since:
1. The main failure cases are test-specific (as mentioned above).
2. These flows are better left tested as part of broader accessibility testing (per #8915).

This has been tested with a cursory playthrough of some basic scenarios (block movement, insertion, deletion, copy & paste, context menus, and interacting with fields).

### Documentation

No new documentation should be needed here.

### Additional Information

This is expected to only affect keyboard navigation plugin behaviors, particularly plugin tests.

It may be worth updating `LineCursor` to completely reflect current focus state rather than holding an internal variable. This, in turn, may end up simplifying solving issues like #8793 (but not necessarily).
2025-05-14 10:50:00 -07:00
Ben Henning
2b9d06ac99 fix: Use a unique focus ID for BlockSvg. (#9045)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes #9043
Fixes https://github.com/google/blockly-samples/issues/2512

### Proposed Changes

This replaces using BlockSvg's own ID for focus management since that's not guaranteed to be unique across all workspaces on the page.

### Reason for Changes

Both https://github.com/google/blockly-samples/issues/2512 covers the user-facing issue in more detail, but from a technical perspective it's possible for blocks to share IDs across workspaces. One easy demonstration of this is the flyout: the first block created from the flyout to the main workspace will share an ID. The workspace minimap plugin just makes the underlying problem more obvious.

The reason this introduces a breakage is due to the inherent ordering that `FocusManager` uses when trying to find a matching tree for a given DOM element that has received focus. These trees are iterated in the order of their registration, so it's quite possible for some cases (like main workspace vs. flyout) to resolve such that the behavior looks correct to users, vs. others (such as the workspace minimap) not behaving as expected.

Guaranteeing ID uniqueness across all workspaces fixes the problem entirely.

### Test Coverage

This has been manually tested in core Blockly's simple test playground and in Blockly samples' workspace minimap plugin test environment (linked against this change). See the new behavior for the minimap plugin:

[Screen recording 2025-05-13 4.31.31 PM.webm](https://github.com/user-attachments/assets/d2ec3621-6e86-4932-ae85-333b0e7015e1)

Note that this is a regression to v11 behavior in that the blocks in the minimap now show as selected.

This has been verified as working with the latest version of the keyboard navigation plugin (tip-of-tree). Keyboard-based block operations and movement seem to work as expected.

For automated testing this is expected to largely be covered by future tests added as part of resolving #8915.

### Documentation

No public documentation changes should be needed, though `IFocusableNode`'s documentation has been refined to be clearer on the uniqueness property for focusable element IDs.

### Additional Information

There's a separate open design question here about whether `BlockSvg`'s descendants should use the new focus ID vs. the block ID. Here is what I consider to be the trade-off analysis in this decision:

|                        | Pros                                            | Cons                                                                         |
|------------------------|-------------------------------------------------|------------------------------------------------------------------------------|
| Use `BlockSvg.id`      | Can use fast `WorkspaceSvg.getBlockById`.       | `WorkspaceSvg.lookUpFocusableNode` now uses 2 different IDs.                 |
| Use `BlockSvg.focusId` | Consistency in IDs use for block-related focus. | Requires more expensive block look-up in `WorkspaceSvg.lookUpFocusableNode`. |
2025-05-14 10:46:22 -07:00
Aaron Dodson
ae22165cbe refactor: Remove INavigable in favor of IFocusableNode. (#9037)
* refactor: Remove INavigable in favor of IFocusableNode.

* chore: Fix JSDoc.

* chore: Address review feedback.
2025-05-13 15:04:49 -07:00
Ben Henning
e34a9690ed fix: Ensure selection stays when dragging blocks (#9034)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes #9027

### Proposed Changes

Ensure that a block being dragged is properly focused mid-drag.

### Reason for Changes

Focus seems to be lost due to the block being moved to the drag layer, so re-focusing the block ensures that it remains both actively focused and selected while dragging.

The regression was likely caused when block selection was moved to be fully synced based on active focus.

### Test Coverage

This has been manually verified in Core's simple playground. At the time of the PR being opened, this couldn't be tested in the test environment for the experimental keyboard navigation plugin since there's a navigation connection issue there that needs to be resolved to test movement.

It would be helpful to add a new test case for the underlying problem (i.e. ensuring that the block holds focus mid-drag) as part of resolving #8915.

### Documentation

No new documentation should need to be added.

### Additional Information

This was found during the development of https://github.com/google/blockly-keyboard-experimentation/pull/511.
2025-05-13 14:37:58 -07:00
Maribeth Moffatt
556ee39f6f fix!: remove deprecated setEnabled and backwards event filtering (#9039) 2025-05-13 14:30:28 -07:00
Aaron Dodson
14e1ef6dc6 fix: Fix regressions in Field. (#9011) 2025-05-13 14:26:00 -07:00
Aaron Dodson
ece662a45f Fix: don't visit connections with the cursor. (#9030) 2025-05-13 11:03:01 -07:00
Ben Henning
e7af75e051 fix: Improve robustness of IFocusableNode uses (#9031)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes https://github.com/google/blockly-keyboard-experimentation/issues/515

### Proposed Changes

This adds `canBeFocused()` checks to all the places that could currently cause problems if a node were to return `false`.

### Reason for Changes

This can't introduce a problem in current Core and, in fact, most of these classes can never return `false` even through subclasses. However, this adds better robustness and fixes the underlying issue by ensuring that `getFocusableElement()` isn't called for a node that has indicated it cannot be focused.

### Test Coverage

I've manually tested this through the keyboard navigation plugin. However, there are clearly additional tests that would be nice to add both for the traverser and for `WorkspaceSvg`, both likely as part of resolving #8915.

### Documentation

No new documentation changes should be needed here.

### Additional Information

This is fixing theoretical issues in Core, but a real issue tracked by the keyboard navigation plugin repository.
2025-05-12 23:36:23 +00:00
Aaron Dodson
a1be83bad8 refactor: Make INavigable extend IFocusableNode. (#9033) 2025-05-12 15:46:27 -07:00
RoboErikG
77bfa5b572 fix: Don't fire events for changes to potential variables (#9025) 2025-05-12 10:29:53 -07:00
Maribeth Moffatt
9cf9170815 chore: remove now-unneeded cast (#9016) 2025-05-09 16:20:36 -07:00
Ben Henning
4074cee31b feat!: Make everything ISelectable focusable (#9004)
* feat!: Make bubbles, comments, and icons focusable

* feat!: Make ISelectable and ICopyable focusable.

* feat: Consolidate selection calls.

Now everything is based on focus with selection only being used as a
proxy.

* feat: Invert responsibility for setSelected().

Now setSelected() is only for quasi-external use.

* feat: Push up shadow check to getters.

Needed new common-level helper.

* chore: Lint fixes.

* feat!: Allow IFocusableNode to disable focus.

* chore: post-merge lint fixes

* fix: Fix tests + text bubble focusing.

This fixed then regressed a circular dependency causing the node and
advanced compilation steps to fail. This investigation is ongoing.

* fix: Clean up & fix imports.

This ensures the node and advanced compilation test steps now pass.

* fix: Lint fixes + revert commented out logic.

* chore: Remove unnecessary cast.

Addresses reviewer comment.

* fix: Some issues and a bunch of clean-ups.

This addresses a bunch of review comments, and fixes selecting workspace
comments.

* chore: Lint fix.

* fix: Remove unnecessary shadow consideration.

* chore: Revert import.

* chore: Some doc updates & added a warn statement.
2025-05-09 08:16:14 -07:00
Rachel Fenichel
92cad53cfe fix!: delete IASTNodeLocation and friends (#9015) 2025-05-08 12:47:39 -07:00
Rachel Fenichel
561129ac61 fix!: delete ASTNode and references (#9014) 2025-05-08 11:47:42 -07:00
Rachel Fenichel
b6b229eb44 fix!: delete marker move event and tests (#9013) 2025-05-08 10:52:43 -07:00
Rachel Fenichel
bb76d6e12c fix!: remove MarkerSvg and uses (#8991)
* fix: delete MarkerSvg (marker drawer)

* fix: delete marker and cursor SVG elements

* chore: format

* chore: lint
2025-05-07 09:28:51 -07:00
Aaron Dodson
acdad98653 refactor!: Use navigation rulesets instead of ASTNode to control keyboard navigation. (#8992)
* feat: Add interfaces for keyboard navigation.

* feat: Add the Navigator.

* feat: Make core types conform to INavigable.

* feat: Require FlyoutItems elements to be INavigable.

* feat: Add navigation policies for built-in types.

* refactor: Convert Marker and LineCursor to operate on INavigables instead of ASTNodes.

* chore: Delete dead code in ASTNode.

* fix: Fix the tests.

* chore: Assuage the linter.

* fix: Fix advanced build/tests.

* chore: Restore ASTNode tests.

* refactor: Move isNavigable() validation into Navigator.

* refactor: Exercise navigation instead of ASTNode.

* chore: Rename astnode_test.js to navigation_test.js.

* chore: Enable the navigation tests.

* fix: Fix bug when retrieving the first child of an empty workspace.
2025-05-07 08:47:52 -07:00
Ben Henning
a3b3ea72f2 fix: Improve missing node resiliency (#8997)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes #8994

### Proposed Changes

This removes an error that was previously thrown by `FocusManager` when attempting to focus an invalid node (such as one that's been removed from its parent).

### Reason for Changes

https://github.com/google/blockly/issues/8994#issuecomment-2855447539 goes into more detail. While this error did cover legitimately wrong cases to try and focus things (and helped to catch some real problems), fixing this 'properly' may become a leaky boat problem where we have to track down every possible asynchronous scenario that could produce such a case. One class of this is ephemeral focus which had robustness improvements itself in #8981 that, by effect, caused this issue in the first place. Holistically fixing this with enforced API contracts alone isn't simple due to the nature of how these components interact.

This change ensures that there's a sane default to fall back on if an invalid node is passed in. Note that `FocusManager` was designed specifically to disallow defocusing a node (since fallbacks can get messy and introduce unpredictable user experiences), and this sort of allows that now. However, this seems like a reasonable approach as it defaults to the behavior when focusing a tree explicitly which allows the tree to fallback to a more suitable default (such as the first item to select in the toolbox for that particular tree). In many cases this will default back to the tree's root node (such as the workspace root group) since sometimes the removed node is still the "last focused node" of the tree (and is considered valid for the purpose of determining a fallback; tree implementations could further specialize by checking whether that node is still valid).

### Test Coverage

Some new tests were added to cover this case, but more may be useful to add as part of #8910.

### Documentation

No documentation needs to be added or updated as part of this (beyond code documentation changes).

### Additional Information

This original issue was found by @RoboErikG when testing #8995. I also verified this against the keyboard navigation plugin repository.
2025-05-06 12:57:19 -07:00
Maribeth Moffatt
86c831a3fe fix: use copyable interface for cut action, add tests (#8993)
* fix: use copyable for cut action

* chore: rename keydown test

* chore: add tests for shortcut items not acting on focused connections
2025-05-06 11:37:28 -07:00
RoboErikG
eb5181e3ef fix: Add private to variableChangeCallback (#8995) 2025-05-06 11:13:25 -07:00
RoboErikG
04a31f950f fix: Wrap toolbox refreshes in a timeout when modifying variables (#8980) 2025-05-06 10:58:05 -07:00
Maribeth Moffatt
233604a74a fix: focus for autohideable flyouts (#8990) 2025-05-05 12:30:33 -07:00
Ben Henning
bbd97eab67 fix: Synchronize gestures and focus (#8981)
## 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 https://github.com/google/blockly-keyboard-experimentation/pull/482#issuecomment-2846341307.

### 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.
2025-05-05 10:29:20 -07:00
Ben Henning
c18c7ffef1 fix: Fix Flyout auto-closing when creating a var. (#8982)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes #8976

### Proposed Changes

Only auto-close the flyout if focus is being lost to a known tree.

### Reason for Changes

I noticed from testing that the system does attempt to restore focus back to the flyout after creating a variable but the auto-closing logic was kicking in due to focus being lost with the variable creation prompt open. Even though an attempt was made to restore focus, this doesn't automatically reopen the flyout (since it is primarily governed by the toolbox selection state).

One alternative might be to try and save the previously selected toolbox category and restore it, but that's tricky. This seems simpler, and also seems to largely maintain parity with pre-focus manager Blockly. Clicking outside of the toolbox with the flyout open only closes it if the click is within the toolbox itself or within the workspace.

### Test Coverage

No new tests were added. However, it may be worth considering this specific case for future tests added with #8915.

### Documentation

No new documentation seems necessary here.

### Additional Information

None.
2025-05-05 09:27:12 -07:00
Christopher Allen
6b695414d3 chore: Merge branch 'develop' into rc/v12.0.0 2025-05-03 02:00:27 +01:00
Christopher Allen
bc0e1c3ca3 feat(WorkspaceSvg): Ignore gestures when keyboard move in progress (#8963)
* feat(WorkspaceSvg): Ignore gestures during keyboard moves

  Modify WorkspaceSvg.prototype.getGesture to return null when
  there is a keyboard-initiated move in progress.

* chore(WorkspaceSvg): Add TODOs to remove .keyboardMoveInProgress
2025-05-03 00:21:41 +01:00
RoboErikG
a4e6166ca8 fix!: Remove alt+key commands (#8961)
* fix: Remove the alt+key commands

* Remove tests for alt+key combos
2025-05-02 12:23:07 -07:00
Maribeth Moffatt
dd3133baac feat: add scope to keyboard shortcuts and use it (#8917)
* feat: add scope to keyboard shortcuts

* feat: add scope to keyboard shortcuts and use it
2025-05-02 12:22:07 -07:00
Robert Knight
1c79e1ed77 fix: Address remaining invisible input positions (#8948) 2025-05-02 10:17:11 -07:00
Christopher Allen
3d1d80d661 refactor!: Finish refactor of WorkspaceSvg VariableMap methods (#8946)
* docs: Make JSDoc @deprecated usage more consistent

* refactor(VariableMap)!: Refresh toolbox when map modified

  Delete the following methods from WorkspaceSvg:

  - renameVariableById
  - deleteVariableById
  - createVariable

  Modify the following methods on VariableMap to call
  this.workspace.refreshToolboxSelection() if this.workspace
  is a WorkspaceSvg, replicating the behaviour of the
  aforementioned deleted methods and additionally ensuring
  that that method is called following any change to the
  variable map:

  - renameVariable
  - changeVariableType
  - renameVariableAndUses
  - createVariable
  - addVariable
  - deleteVariable

  BREAKING CHANGE:

  This change ensures that the toolbox will be refreshed regardless
  of what route the VaribleMap was updated, rather than only being
  refreshed when it is updated via calls to methods on WorkspaceSvg.

  Overall this is much more likely to fix a bug (where the toolbox
  wasn't being refreshed when it should have been) than cause one
  (by refreshing the toolbox when it shouldn't be), but this is
  still a behaviour change which could _conceivably_ result an
  unexpected regression.

* refactor(VariableMap): Remove calls to deprecated getVariableUsesById

  Also refactor to use named imports core/variables.ts methods.

* refactor(Workspace): Use named imports for core/variables.ts methods

* refactor(FieldVariable): Remove call to deprecated getVariablesOfType

* refactor(variables): Remove calls to deprecated methods

* refactor(variables_dynamic): Remove call to deprecated getAllVariables

* refactor(xml): Remove calls to deprecated createVariable

* refactor(Events.VarCreate): Remove calls to deprecated methods

* refactor(Events.VarDelete): Remove calls to deprecated methods

* refactor(Events.VarRename): Remove calls to deprecated methods
2025-05-02 17:47:11 +01:00
Christopher Allen
7b4f2239d7 feat(WorkspaceSvg): Add support for tracking keyboard moves (#8959) 2025-05-02 17:40:45 +01:00
Ben Henning
fdeaa7692b feat: Update line cursor to use focus manager (#8941)
## 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 https://github.com/google/blockly-keyboard-experimentation/pull/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 https://github.com/google/blockly-keyboard-experimentation/pull/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 https://github.com/google/blockly-keyboard-experimentation/pull/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.
2025-05-01 22:18:22 -07:00
Rachel Fenichel
45c142636c fix: remove black outline on focused items (#8951) 2025-05-01 11:18:27 -07:00
Ben Henning
0cbcc3144a feat: Make connections focusable (#8928)
## 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.
2025-04-30 16:39:03 -07:00