Skip to content

Commit 5378c98

Browse files
authored
feat: Use FocusManager for navigation & state management (#482)
Fixes #142 Fixes #481 This removes nearly all custom focus/blur management and instead leverages `FocusManager` as 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: - This replaces all cases where explicit focus/opening logic is needed with `focusNode`/`focusTree` calls via `FocusManager`. - `Navigation` now infers state based on what's currently focused rather than tracking it separately. - All of the existing focus/blur response logic has been made redundant due to the inferred navigation mode and focus being tightly coupled with the components themselves. - The gesture monkey patch seems no longer necessary since, as far as I can tell, focus gets forced back to where it's supposed to be at specific lifecycle moments (which helps undo many of the problems caused by `WorkspaceSvg`'s `markFocused`). - The passive indicator is no longer necessary now that the `FocusManager`-driven `blocklyPassiveFocus` CSS class exists for styling. - Note that there's a lot of automatic state synchronizing that has been purposely pushed into Core to simplify the navigation pieces, particularly: - Blocks will auto-select/unselect themselves based on focus. - `Toolbox` will automatically synchronize its selected item state with its item that has active focus. - `Toolbox` and `Flyout` cooperate for automatically closing the `Flyout` at the correct times. - The new CSS classes are not yet final as the overall strategy for indicating active/passive focus isn't final. There's also additional work needed to ensure selection and active focus can be properly stylized as a combined state, but this is blocked on the completion of RaspberryPiFoundation/blockly#8942. - `Toolbox` navigation was moved to using its methods directly instead of its event since selection logic now needs to be paired with `focusNode`. - There are a number of changes in this PR that could probably go into Core, but these can wait for a future PR as they shouldn't be API breaking. Some of the changes include: - `Toolbox` item selection could auto-focus within `Toolbox` itself, or this could be done as part of `Toolbox`-specific navigation. - `Flyout` focus could automatically focus its first item if nothing is currently focused. - For regression testing, the following has been checked: - 'T' and 'I' do work as expected (ensuring RaspberryPiFoundation/blockly#8933 (review) is no longer an issue). - Verified #227, #200, #229, #222, and #287 have not regressed. RaspberryPiFoundation/blockly-samples#2498 shouldn't be possible anymore (even with the fix) due to ephemeral focus, and we seem to be matching parity with #326 per the `handleFocus*()` functions, though there may be cases from `handleFocusWorkspace` yet 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: - There's a regression with clicking to create a variable closing the flyout that I'm still investigating. This is detailed more in RaspberryPiFoundation/blockly#8941. - At some point between the latest changes to this branch and the ones in RaspberryPiFoundation/blockly#8941 block movement regressed slightly: finishing a movement gesture now defocuses the workspace entirely (even though this was working earlier). - There are a lot of inconsistencies between the focus and selection styling before this PR and with this PR. It's unclear how much of this requires actual resolution. - Much more testing is needed to understand gaps as this changes a significant amount of state handling for the plugin.
1 parent c90aae9 commit 5378c98

16 files changed

+211
-857
lines changed

src/actions/arrow_navigation.ts

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {ASTNode, ShortcutRegistry, utils as BlocklyUtils} from 'blockly/core';
88

99
import type {Field, Toolbox, WorkspaceSvg} from 'blockly/core';
1010

11+
import * as Blockly from 'blockly/core';
1112
import * as Constants from '../constants';
1213
import type {Navigation} from '../navigation';
1314

@@ -53,6 +54,7 @@ export class ArrowNavigation {
5354
shortcut: ShortcutRegistry.KeyboardShortcut,
5455
): boolean => {
5556
const toolbox = workspace.getToolbox() as Toolbox;
57+
const flyout = workspace.getFlyout();
5658
let isHandled = false;
5759
switch (this.navigation.getState(workspace)) {
5860
case Constants.STATE.WORKSPACE:
@@ -67,12 +69,11 @@ export class ArrowNavigation {
6769
}
6870
return isHandled;
6971
case Constants.STATE.TOOLBOX:
70-
isHandled =
71-
toolbox && typeof toolbox.onShortcut === 'function'
72-
? toolbox.onShortcut(shortcut)
73-
: false;
74-
if (!isHandled) {
75-
this.navigation.focusFlyout(workspace);
72+
// @ts-expect-error private method
73+
isHandled = toolbox && toolbox.selectChild();
74+
if (!isHandled && flyout) {
75+
Blockly.getFocusManager().focusTree(flyout.getWorkspace());
76+
this.navigation.defaultFlyoutCursorIfNeeded(workspace);
7677
}
7778
return true;
7879
default:
@@ -100,12 +101,13 @@ export class ArrowNavigation {
100101
}
101102
return isHandled;
102103
case Constants.STATE.FLYOUT:
103-
this.navigation.focusToolbox(workspace);
104+
if (toolbox) {
105+
Blockly.getFocusManager().focusTree(toolbox);
106+
}
104107
return true;
105108
case Constants.STATE.TOOLBOX:
106-
return toolbox && typeof toolbox.onShortcut === 'function'
107-
? toolbox.onShortcut(shortcut)
108-
: false;
109+
// @ts-expect-error private method
110+
return toolbox && toolbox.selectParent();
109111
default:
110112
return false;
111113
}
@@ -173,9 +175,24 @@ export class ArrowNavigation {
173175
}
174176
return isHandled;
175177
case Constants.STATE.TOOLBOX:
176-
return toolbox && typeof toolbox.onShortcut === 'function'
177-
? toolbox.onShortcut(shortcut)
178-
: false;
178+
if (toolbox) {
179+
if (!toolbox.getSelectedItem()) {
180+
const firstItem =
181+
toolbox
182+
.getToolboxItems()
183+
.find((item) => item.isSelectable()) ?? null;
184+
toolbox.setSelectedItem(firstItem);
185+
isHandled = true;
186+
} else {
187+
// @ts-expect-error private method
188+
isHandled = toolbox.selectNext();
189+
}
190+
const selectedItem = toolbox.getSelectedItem();
191+
if (selectedItem) {
192+
Blockly.getFocusManager().focusNode(selectedItem);
193+
}
194+
}
195+
return isHandled;
179196
default:
180197
return false;
181198
}
@@ -221,9 +238,15 @@ export class ArrowNavigation {
221238
}
222239
return isHandled;
223240
case Constants.STATE.TOOLBOX:
224-
return toolbox && typeof toolbox.onShortcut === 'function'
225-
? toolbox.onShortcut(shortcut)
226-
: false;
241+
if (toolbox) {
242+
// @ts-expect-error private method
243+
isHandled = toolbox.selectPrevious();
244+
const selectedItem = toolbox.getSelectedItem();
245+
if (selectedItem) {
246+
Blockly.getFocusManager().focusNode(selectedItem);
247+
}
248+
}
249+
return isHandled;
227250
default:
228251
return false;
229252
}

src/actions/clipboard.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
clipboard,
1414
ICopyData,
1515
LineCursor,
16+
getFocusManager,
1617
} from 'blockly';
1718
import * as Constants from '../constants';
1819
import type {BlockSvg, WorkspaceSvg} from 'blockly';
@@ -281,7 +282,7 @@ export class Clipboard {
281282
const copied = !!this.copyData;
282283
if (copied) {
283284
if (navigationState === Constants.STATE.FLYOUT) {
284-
this.navigation.focusWorkspace(workspace);
285+
getFocusManager().focusTree(workspace);
285286
}
286287
showCopiedHint(workspace);
287288
}
@@ -375,10 +376,10 @@ export class Clipboard {
375376
? workspace
376377
: this.copyWorkspace;
377378

378-
const targetNode = this.navigation.getStationaryNode(pasteWorkspace);
379-
// If we're pasting in the flyout it still targets the workspace. Focus first
380-
// so ensure correct selection handling.
381-
this.navigation.focusWorkspace(workspace);
379+
const targetNode = this.navigation.getFocusedASTNode(pasteWorkspace);
380+
// If we're pasting in the flyout it still targets the workspace. Focus
381+
// first as to ensure correct selection handling.
382+
getFocusManager().focusTree(workspace);
382383

383384
Events.setGroup(true);
384385
const block = clipboard.paste(this.copyData, pasteWorkspace) as BlockSvg;

src/actions/enter.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
Events,
1010
ShortcutRegistry,
1111
utils as BlocklyUtils,
12+
getFocusManager,
1213
} from 'blockly/core';
1314

1415
import type {
@@ -134,7 +135,7 @@ export class EnterAction {
134135
Events.setGroup(true);
135136
}
136137

137-
const stationaryNode = this.navigation.getStationaryNode(workspace);
138+
const stationaryNode = this.navigation.getFocusedASTNode(workspace);
138139
const newBlock = this.createNewBlock(workspace);
139140
if (!newBlock) return;
140141
const insertStartPoint = stationaryNode
@@ -146,7 +147,7 @@ export class EnterAction {
146147

147148
workspace.setResizesEnabled(true);
148149

149-
this.navigation.focusWorkspace(workspace);
150+
getFocusManager().focusTree(workspace);
150151
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
151152
workspace.getCursor()?.setCurNode(ASTNode.createBlockNode(newBlock)!);
152153
this.mover.startMove(workspace, newBlock, insertStartPoint);

src/actions/exit.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,12 @@
44
* SPDX-License-Identifier: Apache-2.0
55
*/
66

7-
import {ShortcutRegistry, utils as BlocklyUtils} from 'blockly/core';
7+
import {
8+
ShortcutRegistry,
9+
utils as BlocklyUtils,
10+
getFocusManager,
11+
Gesture,
12+
} from 'blockly/core';
813

914
import * as Constants from '../constants';
1015
import type {Navigation} from '../navigation';
@@ -29,7 +34,10 @@ export class ExitAction {
2934
switch (this.navigation.getState(workspace)) {
3035
case Constants.STATE.FLYOUT:
3136
case Constants.STATE.TOOLBOX:
32-
this.navigation.focusWorkspace(workspace);
37+
getFocusManager().focusTree(workspace);
38+
if (!Gesture.inProgress()) {
39+
workspace.hideChaff();
40+
}
3341
return true;
3442
default:
3543
return false;

src/actions/mover.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
ASTNode,
1616
Connection,
1717
dragging,
18+
getFocusManager,
1819
registry,
1920
utils,
2021
WorkspaceSvg,
@@ -132,6 +133,9 @@ export class Mover {
132133
// Begin drag.
133134
dragger.onDragStart(info.fakePointerEvent('pointerdown'));
134135
info.updateTotalDelta();
136+
// In case the block is detached, ensure that it still retains focus
137+
// (otherwise dragging will break).
138+
getFocusManager().focusNode(block);
135139
return true;
136140
}
137141

@@ -159,6 +163,8 @@ export class Mover {
159163
this.moves.delete(workspace);
160164
// Delay scroll until after block has finished moving.
161165
setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0);
166+
// If the block gets reattached, ensure it retains focus.
167+
getFocusManager().focusNode(info.block);
162168
return true;
163169
}
164170

@@ -205,6 +211,8 @@ export class Mover {
205211
this.moves.delete(workspace);
206212
// Delay scroll until after block has finished moving.
207213
setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0);
214+
// If the block gets reattached, ensure it retains focus.
215+
getFocusManager().focusNode(info.block);
208216
return true;
209217
}
210218

src/gesture_monkey_patch.js

Lines changed: 0 additions & 32 deletions
This file was deleted.

0 commit comments

Comments
 (0)