Fix CdoFieldDropdown bug #51879

fisher-alice · 2023-05-13T21:26:05Z

This PR fixes a bug that resulted in the recently refactored CdoFieldDropdown from this PR.
The bug showed up in Project Beats - the toolbox flyout was not working properly. See video below

Before update:

before-update-toolbox.mp4

Here is the error message in the dev console:

blockly_compressed.js:480 Uncaught Error: textToDom was unable to parse: 1
    at textToDom$$module$build$src$core$utils$xml (blockly_compressed.js:480:1)
    at CdoFieldDropdown.loadLegacyState (blockly_compressed.js:7703:1)
    at CdoFieldDropdown.loadState (blockly_compressed.js:8394:1)
    at loadFields$$module$build$src$core$serialization$blocks (blockly_compressed.js:1988:1)
    at appendPrivate$$module$build$src$core$serialization$blocks (blockly_compressed.js:1929:1)
    at appendInternal$$module$build$src$core$serialization$blocks (blockly_compressed.js:1901:1)
    at append$$module$build$src$core$serialization$blocks (blockly_compressed.js:1884:1)
    at VerticalFlyout.createFlyoutBlock_ (blockly_compressed.js:22299:1)
    at VerticalFlyout.createFlyoutInfo_ (blockly_compressed.js:22246:1)
    at VerticalFlyout.show (blockly_compressed.js:22205:1)

I saw that there were calls to loadLegacyState and loadState so I hopped over to field_dropdown.ts from mainline Blockly. loadState is defined as:

override loadState(state: AnyDuringMigration) {
    if (this.loadLegacyState(FieldDropdown, state)) {
      return;
    }
    if (this.isOptionListDynamic()) {
      this.getOptions(false);
    }
    this.setValue(state);
  }

Initially, I overrode this function in cdoFieldDropdown.js so that the first parameter for loadLegacyState is CdoFieldDropdown instead of FieldDropdown. This fixed the problem in music lab, but caused a regression back in Dance Lab with the field that contained a config attribute. Once the block was dropped in the workspace, the dropdown options went back to the original state listing all 12 options instead of just 'cat' and 'sloth'. See video below:

may15.mp4

In loadState, loadLegacyState is called. This function is where the music lab error occurs. Because fromXml was overridden by the PR that provides support for the config attribute, the following condition is now set to true.

callingClass.prototype.loadState === this.loadState && callingClass.prototype.fromXml !== this.fromXml

When this.fromXml(utilsXml.textToDom(state) is called, an error is thrown because state in music lab is assigned the value of the dropdown field, e.g., 1, and not a stringified xml element.
Thus, I override the loadState function to accommodate for the differences in what state stores in Google Blockly labs.

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

fisher-alice · 2023-05-13T21:44:23Z

apps/src/blockly/addons/cdoFieldDropdown.js

@@ -32,6 +32,16 @@ export default class CdoFieldDropdown extends GoogleBlockly.FieldDropdown {
    }
  }

+  loadState(state) {


Overridden from mainline Blockly.

breville

Not very familiar with this code, but confirmed it at least seems to fix Music Lab.

breville · 2023-05-16T08:00:18Z

apps/src/blockly/addons/cdoFieldDropdown.js

+   * For music lab, `state` is the value of the field.
+   * For other labs, `state` is stringified xml.


In the future, will this only be a difference between Music Lab and all other labs? Or could other labs that use Blockly in the same way as Music Lab also fall into the former case?

(The underlying question here is whether Music Lab is doing something it shouldn't, or is it actually doing things as expected?)

I think this is more of a question of whether the source for the block is XML or JSON, but both are supported. From the documentation:

saveState and loadState are serialization hooks that work with the new JSON serialization system.

In some cases you do not need to provide these, because the default implementations will work. If (1) your field is a direct subclass of the base Blockly.Field class, (2) your value is a JSON serializable type, and (3) you only need to serialize the value, then the default implementation will work just fine!

Otherwise, your saveState function should return a JSON serializable object/value which represents the state of the field. And your loadState function should accept the same JSON serializable object/value, and apply it to the field.

Also, I believe it's fine for state to be stringified xml i this case and that it's actually an intentional part of mainline's backwards compatibility with XML..

Thanks Mike! Music lab is doing what the Blockly team now recommends (using json) and Blocky's implementation of the fields is to support both json and xml. But they didn't anticipate this scenario where sometimes state is xml and sometimes it's stored in json as it is in our code base.
I think this was a super useful bug since we're migrating to using json, but from my understanding, we'll always have to support xml to some degree (levelbuilder, student projects).

This reverts commit 6eeb05e.

…to alice/fix-dropdown-bug

mikeharv · 2023-05-16T13:29:30Z

apps/src/blockly/addons/cdoFieldDropdown.js

+    if (field) {
+      this.setValue(field.textContent);
+    } else {
+      this.setValue(state); // music lab


This line actually looks closest to the standard implementation in the documentation, which makes me wonder if our code should treat it as the general rule rather than the exception. Either way, I'm wondering if it makes sense to do all the xml parsing above if state is just the value. Is there a way we could check at the top of this function which type of state we have and only parse the xml if necessary?

Sure. I can first check to see if state is not a stringified xml (checking first character). How does that sound?

Discussed in Slack - using regex.

apps/src/blockly/addons/cdoFieldDropdown.js

mikeharv

This looks good, Alice!

mikeharv · 2023-05-16T15:20:48Z

apps/src/blockly/addons/cdoFieldDropdown.js

+   *
+   * @param state The state we want to apply to the field.
+   * @override because different labs store `state` in different ways.
+   * For music lab, `state` is the value of the field.


nit: Can we update this to clarify that this isn't a "music lab" thing so much as it is a JSON serialization thing?

Sure - pushed the update!

mikeharv · 2023-05-16T15:22:17Z

apps/src/blockly/addons/cdoFieldDropdown.js

    if (this.config) {
      element.setAttribute('config', this.config);
    }
+    super.toXml(element);


Can you explain why we wanted to move this down?

Sure!
Short answer: to be consistent with fromXml.
Long answer: super.toXml assigns to the element (textContent) the value of the field. We override to support the config attribute if it exists and the placement of the call to super doesn't matter in this function.
But the order does matter in fromXml above. The call to super.fromXml must happen after we check the config. So I moved this line just to be consistent with fromXml.

Perfect, thanks!

fisher-alice · 2023-05-16T23:11:17Z

apps/src/blockly/addons/cdoFieldDropdown.js

+      if (this.isOptionListDynamic()) {
+        this.getOptions(false);
+      }
+      // TODO: handle config if stored in json


At this point, when state is stored as json. it is assigned only the value. At some point, we may choose support config in json so leaving comment here.

fisher-alice · 2023-05-16T23:12:13Z

apps/src/blockly/addons/cdoFieldDropdown.js

+    // Check if state is not stringified xml, i.e., value from json.
+    const fieldTagRegEx = /<field/;
+    if (!fieldTagRegEx.test(state)) {
+      if (this.isOptionListDynamic()) {


~~Although we currently don't generate options using a function, we may choose to do so in the future.~~
Re-wrote comment explaining current state - that we only support config in xml and not in json.

fisher-alice · 2023-05-16T23:12:45Z

apps/src/blockly/addons/cdoFieldDropdown.js

+      return;
+    }
+    const field = GoogleBlockly.utils.xml.textToDom(state);
+    this.fromXml(field);


Handles state when state is stored as xml.

Fix CdoFieldDropdown bug

override loadState

d1f4b66

fisher-alice commented May 13, 2023

View reviewed changes

fisher-alice marked this pull request as ready for review May 13, 2023 21:44

fisher-alice requested review from mikeharv and breville May 13, 2023 21:44

breville approved these changes May 13, 2023

View reviewed changes

fisher-alice marked this pull request as draft May 14, 2023 00:10

fisher-alice added 6 commits May 15, 2023 08:31

add saveState

2f3fed2

add saveLegacyState and refactor class

d91d851

clean up functions, add comments

6b6d684

remove print

bf7b6ac

remove unnec saveLgacyState

d5595a5

use this.config

06ceb57

fisher-alice requested a review from breville May 16, 2023 05:44

fisher-alice marked this pull request as ready for review May 16, 2023 05:44

breville reviewed May 16, 2023

View reviewed changes

mikeharv and others added 3 commits May 16, 2023 08:56

Revert "add separate field dropdown class for music lab (#51878)"

a56a963

This reverts commit 6eeb05e.

Merge branch 'staging' into alice/fix-dropdown-bug

8c1f2ff

Merge branch 'revert-51878-mike/separate-field-dropdown-for-music' in…

d81481e

…to alice/fix-dropdown-bug

mikeharv reviewed May 16, 2023

View reviewed changes

apps/src/blockly/addons/cdoFieldDropdown.js Show resolved Hide resolved

fisher-alice added 4 commits May 16, 2023 09:32

refactor loadState

d8c1699

fix regex

09f118a

remove print

47c04f8

fix typo

16ee1df

fisher-alice requested a review from mikeharv May 16, 2023 15:19

mikeharv approved these changes May 16, 2023

View reviewed changes

fisher-alice added 2 commits May 16, 2023 18:04

refactor loadState to call on fromXml

589e38b

refactor loadState

37d12fd

fisher-alice commented May 16, 2023

View reviewed changes

fisher-alice requested a review from mikeharv May 16, 2023 23:12

update comment

3df2214

mikeharv approved these changes May 17, 2023

View reviewed changes

fisher-alice merged commit 643465d into staging May 17, 2023
2 checks passed

fisher-alice deleted the alice/fix-dropdown-bug branch May 17, 2023 19:54

pablo-code-org pushed a commit that referenced this pull request May 25, 2023

Fix CdoFieldDropdown bug (#51879)

fdef2b9

Fix CdoFieldDropdown bug

snickell pushed a commit that referenced this pull request Feb 3, 2024

Fix CdoFieldDropdown bug (#51879)

2078582

Fix CdoFieldDropdown bug

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CdoFieldDropdown bug #51879

Fix CdoFieldDropdown bug #51879

fisher-alice commented May 13, 2023 •

edited

fisher-alice May 13, 2023

breville left a comment

breville May 16, 2023

mikeharv May 16, 2023

mikeharv May 16, 2023

fisher-alice May 17, 2023

mikeharv May 16, 2023

fisher-alice May 16, 2023

fisher-alice May 16, 2023

mikeharv left a comment

mikeharv May 16, 2023

fisher-alice May 17, 2023

mikeharv May 16, 2023

fisher-alice May 16, 2023

mikeharv May 16, 2023

fisher-alice May 16, 2023

fisher-alice May 16, 2023 •

edited

fisher-alice May 16, 2023

		* For music lab, `state` is the value of the field.
		* For other labs, `state` is stringified xml.

Fix CdoFieldDropdown bug #51879

Fix CdoFieldDropdown bug #51879

Conversation

fisher-alice commented May 13, 2023 • edited

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Choose a reason for hiding this comment

breville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisher-alice May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisher-alice commented May 13, 2023 •

edited

fisher-alice May 16, 2023 •

edited