Skip to content

Fix handling of checkbox values in developer tools#2684

Merged
mikeharv merged 1 commit intoRaspberryPiFoundation:mainfrom
viliml:patch-checkbox
Apr 10, 2026
Merged

Fix handling of checkbox values in developer tools#2684
mikeharv merged 1 commit intoRaspberryPiFoundation:mainfrom
viliml:patch-checkbox

Conversation

@viliml
Copy link
Copy Markdown
Contributor

@viliml viliml commented Apr 9, 2026

The Checkbox field's values are not booleans but strings 'TRUE' or 'FALSE'.
The string 'FALSE' is truthy in Javascript, so Image field definitions copied from this block creator would always inadvertently have flipRtl set to true.
This patch adds conversion from the checkbox string to its boolean value to ensure intended behavior.
The Checkbox field accepts both 'TRUE'/'FALSE' strings and booleans so it works in practice, but the Typescript API specifies that the JSON config value should be boolean, so I added boolean conversion there too.

The string `'FALSE'` is truthy in Javascript, so Image field definitions copied from this block creator would always inadvertently have `flipRtl` set to `true`.
The Checkbox field accepts both `'TRUE'`/`'FALSE'` strings and booleans so it works in practice, but the Typescript API specifies that the JSON config value should be boolean, so I added boolean conversion there too.
@viliml viliml requested a review from a team as a code owner April 9, 2026 13:23
@viliml viliml requested review from mikeharv and removed request for a team April 9, 2026 13:23
Copy link
Copy Markdown
Contributor

@mikeharv mikeharv left a comment

Choose a reason for hiding this comment

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

Hello Vilim,
I was able to reproduce this bug by creating a block with an image field that shouldn't flip for RTL. As you found, it seems that image does flip unexpectedly. Thanks for the fix!

Just a small clarification regarding the checkbox field: when the field is created, checked sets its initial value (not a config value). TypeScript enforces that this values is CheckboxBool which intentionally includes 'TRUE' | 'FALSE' | boolean.
The image field version is different (and was broken before your fix) because flipRtl is a strictly boolean config value.

Could you please resubmit with only the changes for the image field?

@viliml
Copy link
Copy Markdown
Contributor Author

viliml commented Apr 9, 2026

Hello Vilim, I was able to reproduce this bug by creating a block with an image field that shouldn't flip for RTL. As you found, it seems that image does flip unexpectedly. Thanks for the fix!

Just a small clarification regarding the checkbox field: when the field is created, checked sets its initial value (not a config value). TypeScript enforces that this values is CheckboxBool which intentionally includes 'TRUE' | 'FALSE' | boolean. The image field version is different (and was broken before your fix) because flipRtl is a strictly boolean config value.

Could you please resubmit with only the changes for the image field?

Look here: https://github.com/RaspberryPiFoundation/blockly/blob/cdcdaf32869cc1c04a237d416d46a9e507424bdc/packages/blockly/core/field_checkbox.ts#L248. It is fine because it later gets forwarded into a function that accepts CheckboxBool, but it's still typed as requiring checked to be a bool when coming from json like in https://github.com/RaspberryPiFoundation/blockly-samples/pull/2684/changes#diff-02a72210435eaabd5c8693bf44013983f84d6b9dd981e8b0725ee278886efa9cR33. If you want to keep this direct forwarding, the type of checked in the FieldCheckboxFromJsonConfig interface should also be changed to CheckboxBool to minimize confusion.

The conversion before the constructor call in https://github.com/RaspberryPiFoundation/blockly-samples/pull/2684/changes#diff-02a72210435eaabd5c8693bf44013983f84d6b9dd981e8b0725ee278886efa9cR43 is not necessary, but in my opinion, it would be better to explicitly convert checkbox values into booleans always, to prevent mistakes like with the image field from slipping by.

@viliml viliml requested a review from mikeharv April 10, 2026 14:09
Copy link
Copy Markdown
Contributor

@mikeharv mikeharv left a comment

Choose a reason for hiding this comment

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

Thanks. This makes sense, and I agree there's a bit of a mismatch in the typing here.

I agree the JSON generator should convert checked to a boolean, since the JSON config interface expects a boolean there.

For the JavaScript generator, the current behavior is already valid (FieldCheckbox accepts 'TRUE' | 'FALSE' | boolean), so the conversion isn't strictly necessary. That said, I'm fine with updating it for consistency between the JSON and JS outputs.

@mikeharv mikeharv merged commit 882072d into RaspberryPiFoundation:main Apr 10, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants