Skip to content

Commit 1e37d21

Browse files
BenHenningBenHenning
authored
fix: Ensure focus changes when tabbing fields (#9173)
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes RaspberryPiFoundation/blockly-keyboard-experimentation#578 Fixes part of #8915 ### Proposed Changes Ensures fields update focus to the next field when tabbing between field editors. The old behavior can be observed in [#578](RaspberryPiFoundation/blockly-keyboard-experimentation#578) and the new behavior can be observed here: [Screen recording 2025-06-25 1.39.28 PM.webm](https://github.com/user-attachments/assets/e00fcb55-5c20-4d5c-81a8-be9049cc0d7e) ### Reason for Changes Having focus reset back to the original field editor that was opened is an unexpected experience for users. This approach is better. Note that there are some separate changes added here, as well: - Connections and fields now check if their block IDs contain their indicator prefixes since this will all-but-guarantee focus breaks for those nodes. This is an excellent example of why #9171 is needed. - Some minor naming updates for `FieldInput`: it was incorrectly implying that key events are sent for changes to the `input` element used by the field editor, but they are actually `InputEvent`s per https://developer.mozilla.org/en-US/docs/Web/API/Element/input_event. ### Test Coverage New tests were added for field editing in general (since this seems to be missing), including tabbing support to ensure the fixes originally introduced in #9049. One new test has been added specifically for verifying that focus updates with tabbing. This has been verified to fail with the fix removed (as have all tabbing tests with the tabbing code from #9049 removed). Some specific notes for the test changes: - There's a slight test dependency inversion happening here. `FieldInput` is being tested with a specific `FieldNumber` class via real block loading. This isn't ideal, but it seems fine given the circumstances (otherwise a lot of extra setup would be necessary for the tests). - The workspace actually needs to be made visible during tests in order for focus to work correctly (though it's reset at the end of each test, but this may cause some flickering while the tests are running). - It's the case that a bunch of tests were actually setting up blocks incorrectly (i.e. not defining a must-have `id` property which caused some issues with the new field and connection ID validation checks). These tests have been corrected, but it's worth noting that the blocks are likely still technically wrong since they are not conforming to their TypeScript contracts. - Similar to the previous point, one test was incorrectly setting the first ID to be returned by the ID generator as `undefined` since (presumably due to a copy-and-paste error when the test was introduced) it was referencing a `TEST_BLOCK_ID` property that hadn't been defined for that test suite. This has been corrected as, without it, there are failures due to the new validation checks. - For the connection database checks, a new ID is generated instead of fixing the block ID to ensure that it's always unique even if called multiple times (otherwise a block ID would need to be piped through from the calling tests, or an invalid situation would need to be introduced where multiple blocks shared an ID; the former seemed unnecessary and the latter seemed nonideal). - There are distinct Geras/Zelos tests to validate the case where a full-block field should have its parent block, rather than the field itself, focused on tabbing. See this conversation for more context: #9173 (comment). ### Documentation No documentation changes should be needed here. ### Additional Information Nothing to add.
1 parent 5acd072 commit 1e37d21

7 files changed

Lines changed: 364 additions & 18 deletions

File tree

‎core/connection.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,12 @@ export class Connection {
8383
public type: number,
8484
) {
8585
this.sourceBlock_ = source;
86+
if (source.id.includes('_connection')) {
87+
throw new Error(
88+
`Connection ID indicator is contained in block ID. This will cause ` +
89+
`problems with focus: ${source.id}.`,
90+
);
91+
}
8692
this.id = `${source.id}_connection_${idGenerator.getNextUniqueId()}`;
8793
}
8894

‎core/field.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,12 @@ export abstract class Field<T = any>
265265
throw Error('Field already bound to a block');
266266
}
267267
this.sourceBlock_ = block;
268+
if (block.id.includes('_field')) {
269+
throw new Error(
270+
`Field ID indicator is contained in block ID. This may cause ` +
271+
`problems with focus: ${block.id}.`,
272+
);
273+
}
268274
this.id_ = `${block.id}_field_${idGenerator.getNextUniqueId()}`;
269275
}
270276

‎core/field_input.ts‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
FieldValidator,
2828
UnattachedFieldError,
2929
} from './field.js';
30+
import {getFocusManager} from './focus_manager.js';
3031
import type {IFocusableNode} from './interfaces/i_focusable_node.js';
3132
import {Msg} from './msg.js';
3233
import * as renderManagement from './render_management.js';
@@ -83,8 +84,8 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
8384
/** Key down event data. */
8485
private onKeyDownWrapper: browserEvents.Data | null = null;
8586

86-
/** Key input event data. */
87-
private onKeyInputWrapper: browserEvents.Data | null = null;
87+
/** Input element input event data. */
88+
private onInputWrapper: browserEvents.Data | null = null;
8889

8990
/**
9091
* Whether the field should consider the whole parent block to be its click
@@ -558,7 +559,7 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
558559
this.onHtmlInputKeyDown_,
559560
);
560561
// Resize after every input change.
561-
this.onKeyInputWrapper = browserEvents.conditionalBind(
562+
this.onInputWrapper = browserEvents.conditionalBind(
562563
htmlInput,
563564
'input',
564565
this,
@@ -572,9 +573,9 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
572573
browserEvents.unbind(this.onKeyDownWrapper);
573574
this.onKeyDownWrapper = null;
574575
}
575-
if (this.onKeyInputWrapper) {
576-
browserEvents.unbind(this.onKeyInputWrapper);
577-
this.onKeyInputWrapper = null;
576+
if (this.onInputWrapper) {
577+
browserEvents.unbind(this.onInputWrapper);
578+
this.onInputWrapper = null;
578579
}
579580
}
580581

@@ -614,6 +615,14 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
614615
if (target instanceof FieldInput) {
615616
WidgetDiv.hideIfOwner(this);
616617
dropDownDiv.hideWithoutAnimation();
618+
const targetSourceBlock = target.getSourceBlock();
619+
if (
620+
target.isFullBlockField() &&
621+
targetSourceBlock &&
622+
targetSourceBlock instanceof BlockSvg
623+
) {
624+
getFocusManager().focusNode(targetSourceBlock);
625+
} else getFocusManager().focusNode(target);
617626
target.showEditor();
618627
}
619628
}
@@ -622,7 +631,7 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
622631
/**
623632
* Handle a change to the editor.
624633
*
625-
* @param _e Keyboard event.
634+
* @param _e InputEvent.
626635
*/
627636
private onHtmlInputChange(_e: Event) {
628637
// Intermediate value changes from user input are not confirmed until the

‎tests/mocha/connection_checker_test.js‎

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,10 @@ suite('Connection checker', function () {
2929
}
3030

3131
test('Target Null', function () {
32-
const connection = new Blockly.Connection({}, ConnectionType.INPUT_VALUE);
32+
const connection = new Blockly.Connection(
33+
{id: 'test'},
34+
ConnectionType.INPUT_VALUE,
35+
);
3336
assertReasonHelper(
3437
this.checker,
3538
connection,
@@ -38,7 +41,7 @@ suite('Connection checker', function () {
3841
);
3942
});
4043
test('Target Self', function () {
41-
const block = {workspace: 1};
44+
const block = {id: 'test', workspace: 1};
4245
const connection1 = new Blockly.Connection(
4346
block,
4447
ConnectionType.INPUT_VALUE,
@@ -57,11 +60,11 @@ suite('Connection checker', function () {
5760
});
5861
test('Different Workspaces', function () {
5962
const connection1 = new Blockly.Connection(
60-
{workspace: 1},
63+
{id: 'test1', workspace: 1},
6164
ConnectionType.INPUT_VALUE,
6265
);
6366
const connection2 = new Blockly.Connection(
64-
{workspace: 2},
67+
{id: 'test2', workspace: 2},
6568
ConnectionType.OUTPUT_VALUE,
6669
);
6770

@@ -76,10 +79,10 @@ suite('Connection checker', function () {
7679
setup(function () {
7780
// We have to declare each separately so that the connections belong
7881
// on different blocks.
79-
const prevBlock = {isShadow: function () {}};
80-
const nextBlock = {isShadow: function () {}};
81-
const outBlock = {isShadow: function () {}};
82-
const inBlock = {isShadow: function () {}};
82+
const prevBlock = {id: 'test1', isShadow: function () {}};
83+
const nextBlock = {id: 'test2', isShadow: function () {}};
84+
const outBlock = {id: 'test3', isShadow: function () {}};
85+
const inBlock = {id: 'test4', isShadow: function () {}};
8386
this.previous = new Blockly.Connection(
8487
prevBlock,
8588
ConnectionType.PREVIOUS_STATEMENT,
@@ -197,11 +200,13 @@ suite('Connection checker', function () {
197200
suite('Shadows', function () {
198201
test('Previous Shadow', function () {
199202
const prevBlock = {
203+
id: 'test1',
200204
isShadow: function () {
201205
return true;
202206
},
203207
};
204208
const nextBlock = {
209+
id: 'test2',
205210
isShadow: function () {
206211
return false;
207212
},
@@ -224,11 +229,13 @@ suite('Connection checker', function () {
224229
});
225230
test('Next Shadow', function () {
226231
const prevBlock = {
232+
id: 'test1',
227233
isShadow: function () {
228234
return false;
229235
},
230236
};
231237
const nextBlock = {
238+
id: 'test2',
232239
isShadow: function () {
233240
return true;
234241
},
@@ -251,11 +258,13 @@ suite('Connection checker', function () {
251258
});
252259
test('Prev and Next Shadow', function () {
253260
const prevBlock = {
261+
id: 'test1',
254262
isShadow: function () {
255263
return true;
256264
},
257265
};
258266
const nextBlock = {
267+
id: 'test2',
259268
isShadow: function () {
260269
return true;
261270
},
@@ -278,11 +287,13 @@ suite('Connection checker', function () {
278287
});
279288
test('Output Shadow', function () {
280289
const outBlock = {
290+
id: 'test1',
281291
isShadow: function () {
282292
return true;
283293
},
284294
};
285295
const inBlock = {
296+
id: 'test2',
286297
isShadow: function () {
287298
return false;
288299
},
@@ -305,11 +316,13 @@ suite('Connection checker', function () {
305316
});
306317
test('Input Shadow', function () {
307318
const outBlock = {
319+
id: 'test1',
308320
isShadow: function () {
309321
return false;
310322
},
311323
};
312324
const inBlock = {
325+
id: 'test2',
313326
isShadow: function () {
314327
return true;
315328
},
@@ -332,11 +345,13 @@ suite('Connection checker', function () {
332345
});
333346
test('Output and Input Shadow', function () {
334347
const outBlock = {
348+
id: 'test1',
335349
isShadow: function () {
336350
return true;
337351
},
338352
};
339353
const inBlock = {
354+
id: 'test2',
340355
isShadow: function () {
341356
return true;
342357
},
@@ -373,9 +388,11 @@ suite('Connection checker', function () {
373388
};
374389
test('Output connected, adding previous', function () {
375390
const outBlock = {
391+
id: 'test1',
376392
isShadow: function () {},
377393
};
378394
const inBlock = {
395+
id: 'test2',
379396
isShadow: function () {},
380397
};
381398
const outCon = new Blockly.Connection(
@@ -394,6 +411,7 @@ suite('Connection checker', function () {
394411
ConnectionType.PREVIOUS_STATEMENT,
395412
);
396413
const nextBlock = {
414+
id: 'test3',
397415
isShadow: function () {},
398416
};
399417
const nextCon = new Blockly.Connection(
@@ -410,9 +428,11 @@ suite('Connection checker', function () {
410428
});
411429
test('Previous connected, adding output', function () {
412430
const prevBlock = {
431+
id: 'test1',
413432
isShadow: function () {},
414433
};
415434
const nextBlock = {
435+
id: 'test2',
416436
isShadow: function () {},
417437
};
418438
const prevCon = new Blockly.Connection(
@@ -431,6 +451,7 @@ suite('Connection checker', function () {
431451
ConnectionType.OUTPUT_VALUE,
432452
);
433453
const inBlock = {
454+
id: 'test3',
434455
isShadow: function () {},
435456
};
436457
const inCon = new Blockly.Connection(
@@ -449,8 +470,14 @@ suite('Connection checker', function () {
449470
});
450471
suite('Check Types', function () {
451472
setup(function () {
452-
this.con1 = new Blockly.Connection({}, ConnectionType.PREVIOUS_STATEMENT);
453-
this.con2 = new Blockly.Connection({}, ConnectionType.NEXT_STATEMENT);
473+
this.con1 = new Blockly.Connection(
474+
{id: 'test1'},
475+
ConnectionType.PREVIOUS_STATEMENT,
476+
);
477+
this.con2 = new Blockly.Connection(
478+
{id: 'test2'},
479+
ConnectionType.NEXT_STATEMENT,
480+
);
454481
});
455482
function assertCheckTypes(checker, one, two) {
456483
assert.isTrue(checker.doTypeChecks(one, two));

‎tests/mocha/connection_db_test.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66

77
import {ConnectionType} from '../../build/src/core/connection_type.js';
8+
import * as idGenerator from '../../build/src/core/utils/idgenerator.js';
89
import {assert} from '../../node_modules/chai/chai.js';
910
import {
1011
sharedTestSetup,
@@ -31,7 +32,7 @@ suite('Connection Database', function () {
3132
};
3233
workspace.connectionDBList[type] = opt_database || this.database;
3334
const connection = new Blockly.RenderedConnection(
34-
{workspace: workspace},
35+
{id: idGenerator.getNextUniqueId(), workspace: workspace},
3536
type,
3637
);
3738
connection.x = x;

‎tests/mocha/event_test.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,7 @@ suite('Events', function () {
355355

356356
suite('With variable getter blocks', function () {
357357
setup(function () {
358+
this.TEST_BLOCK_ID = 'test_block_id';
358359
this.genUidStub = createGenUidStubWithReturns([
359360
this.TEST_BLOCK_ID,
360361
'test_var_id',

0 commit comments

Comments
 (0)