Skip to content

Commit 38e30fa

Browse files
authored
feat: Expand single field block labeling (experimental) (#9484)
## The basics - [ ] ~I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)~ (I can't really due to the nature of this change--I have to rely on tests and external testing) ## The details ### Resolves Fixes part of #9456 (remainder will be in a change in the keyboard navigation plugin) ### Proposed Changes Introduce and use new `Block` function for retrieving a configurably constrained singleton field for a given block. The constraints allow for some level of configuring (such as whether to isolate to only full or editable blocks). The existing simple reporter function has been retrofitted to use this new function, instead. ### Reason for Changes This expanded support fixes the underlying use case. Separately, this change reveals two noteworthy details: 1. There's inconsistency in the codebase as to when the singleton field needs to be editable, a full-block field, both, and neither. It would be ideal to make this consistent. Interestingly, the documentation for `isSimpleReporter` seems to have been wrong since it wasn't actually fulfilling its contract of returning an editable field (this has been retained for callsites except where the check was already happening). 2. There's a possible recursion case now possible between `getSingletonFullBlockField` and `isFullBlockField` due to `FieldInput`'s `isFullBlockField` depending on `isSimpleReporter`. Ideally this would be changed in the future to avoid that potential recursion risk (possibly as part of #9307). ### Test Coverage No new automated tests are needed for this experimental work. Manual testing mainly comprised of cursory navigation and readout checks for single-field blocks to make sure nothing breaks. More thorough testing is difficult in core since the specific situation of multiple fields don't have a corresponding block to use in the playground to verify. Automated tests are also being heavily relied on for correctness since all of the nuance behind the simple reporter cases would require a deeper testing pass. ### Documentation No new documentation needed for this experimental work. ### Additional Information None.
1 parent 0aa176e commit 38e30fa

5 files changed

Lines changed: 42 additions & 35 deletions

File tree

core/block.ts

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -962,16 +962,43 @@ export class Block {
962962
}
963963

964964
/**
965-
* @returns True if this block is a value block with a single editable field.
965+
* @returns True if this block is a value block with a full block field.
966+
* @param mustBeFullBlock Whether the evaluated field must be 'full-block'.
967+
* @param mustBeEditable Whether the evaluated field must be editable.
966968
* @internal
967969
*/
968-
isSimpleReporter(): boolean {
969-
if (!this.outputConnection) return false;
970+
isSimpleReporter(
971+
mustBeFullBlock: boolean = false,
972+
mustBeEditable: boolean = false,
973+
): boolean {
974+
return (
975+
this.getSingletonFullBlockField(mustBeFullBlock, mustBeEditable) !== null
976+
);
977+
}
970978

971-
for (const input of this.inputList) {
972-
if (input.connection || input.fieldRow.length > 1) return false;
973-
}
974-
return true;
979+
/**
980+
* Determines and returns the only field of this block, or null if there isn't
981+
* one and this block can't be considered a simple reporter. Null will also be
982+
* returned if the singleton block doesn't match additional criteria, if set,
983+
* such as being full-block or editable.
984+
*
985+
* @param mustBeFullBlock Whether the returned field must be 'full-block'.
986+
* @param mustBeEditable Whether the returned field must be editable.
987+
* @returns The only full-block, maybe editable field of this block, or null.
988+
* @internal
989+
*/
990+
getSingletonFullBlockField(
991+
mustBeFullBlock: boolean,
992+
mustBeEditable: boolean,
993+
): Field<any> | null {
994+
if (!this.outputConnection) return null;
995+
for (const input of this.inputList) if (input.connection) return null;
996+
const matchingFields = Array.from(this.getFields()).filter((field) => {
997+
if (mustBeFullBlock && !field.isFullBlockField()) return false;
998+
if (mustBeEditable && !field.isCurrentlyEditable()) return false;
999+
return true;
1000+
});
1001+
return matchingFields.length === 1 ? matchingFields[0] : null;
9751002
}
9761003

9771004
/**

core/block_svg.ts

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -233,10 +233,7 @@ export class BlockSvg
233233
* @internal
234234
*/
235235
recomputeAriaLabel() {
236-
if (this.isSimpleReporter()) {
237-
const field = Array.from(this.getFields())[0];
238-
if (field.isFullBlockField() && field.isCurrentlyEditable()) return;
239-
}
236+
if (this.isSimpleReporter(true, true)) return;
240237

241238
aria.setState(
242239
this.getFocusableElement(),
@@ -1964,13 +1961,8 @@ export class BlockSvg
19641961

19651962
/** See IFocusableNode.getFocusableElement. */
19661963
getFocusableElement(): HTMLElement | SVGElement {
1967-
if (this.isSimpleReporter()) {
1968-
const field = Array.from(this.getFields())[0];
1969-
if (field && field.isFullBlockField() && field.isCurrentlyEditable()) {
1970-
return field.getFocusableElement();
1971-
}
1972-
}
1973-
return this.pathObject.svgPath;
1964+
const singletonField = this.getSingletonFullBlockField(true, true);
1965+
return singletonField?.getFocusableElement() ?? this.pathObject.svgPath;
19741966
}
19751967

19761968
/** See IFocusableNode.getFocusableTree. */

core/contextmenu_items.ts

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,6 @@ import {Coordinate} from './utils/coordinate.js';
2626
import * as svgMath from './utils/svg_math.js';
2727
import type {WorkspaceSvg} from './workspace_svg.js';
2828

29-
function isFullBlockField(block?: BlockSvg) {
30-
if (!block || !block.isSimpleReporter()) return false;
31-
const firstField = block.getFields().next().value;
32-
return firstField?.isFullBlockField();
33-
}
34-
3529
/**
3630
* Option to undo previous action.
3731
*/
@@ -377,7 +371,7 @@ export function registerComment() {
377371
// Either block already has a comment so let us remove it,
378372
// or the block isn't just one full-block field block, which
379373
// shouldn't be allowed to have comments as there's no way to read them.
380-
(block.hasIcon(CommentIcon.TYPE) || !isFullBlockField(block))
374+
(block.hasIcon(CommentIcon.TYPE) || !block.isSimpleReporter(true))
381375
) {
382376
return 'enabled';
383377
}

core/field.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -330,12 +330,9 @@ export abstract class Field<T = any>
330330
this.initModel();
331331
this.applyColour();
332332

333-
const id =
334-
this.isFullBlockField() &&
335-
this.isCurrentlyEditable() &&
336-
this.sourceBlock_?.isSimpleReporter()
337-
? idGenerator.getNextUniqueId()
338-
: `${this.sourceBlock_?.id}_field_${idGenerator.getNextUniqueId()}`;
333+
const id = this.sourceBlock_?.isSimpleReporter(true, true)
334+
? idGenerator.getNextUniqueId()
335+
: `${this.sourceBlock_?.id}_field_${idGenerator.getNextUniqueId()}`;
339336
this.fieldGroup_.setAttribute('id', id);
340337
}
341338

core/keyboard_nav/field_navigation_policy.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,7 @@ export class FieldNavigationPolicy implements INavigationPolicy<Field<any>> {
6565
current.canBeFocused() &&
6666
current.isVisible() &&
6767
(current.isClickable() || current.isCurrentlyEditable()) &&
68-
!(
69-
current.getSourceBlock()?.isSimpleReporter() &&
70-
current.isFullBlockField()
71-
) &&
68+
!current.getSourceBlock()?.isSimpleReporter(true, true) &&
7269
current.getParentInput().isVisible()
7370
);
7471
}

0 commit comments

Comments
 (0)