Some FieldDropdown changes don't fire BlockChange events

[cpcallen]

Check for duplicates

• I have searched for similar issues before opening a new one.

Description

Sometimes changing the value of a FieldDropdown results in a BlockChange event being fired, but other times it does not.

Reproduction steps

1. Go to playground.
2. Open browser console.
3. Enable "Log main workspace events" tickbox.
4. From the "Lists" category of the toolbox, drag a lists_getIndex block ("in list ____ get # ___") block to the workspace.
5. Change the WHERE field from "#" to "last"; observe a BlockChange event is logged.
6. Change the WHERE field from "last" to "#"; observe no event is logged.
7. Change the WHERE field from "#" to "last", then to "# from end", then to "#"; observe that in each case a BlockChange event is logged.

Additional info

I have verified that this is not due to any problem with events being mishandled by the enque / filter / dispatch code in events/utils.ts: the fire function is not being called at all in step 6 above.

[cpcallen]

Hey @johnnesky: this might be of interest to you.

[abhinavjha0239]

@cpcallen @BeksOmega hey I wanted to work on this.

[cpcallen]

@abhinavjha0239: Sure, by all means see if you can figure out what's going on. A PR is welcome, but even a comment explaining exactly why it sometimes fails to fire an event would be super useful.

[johnnesky]

I've done a little investigation already. The lists_getIndex block type (and a few other block types) has a complex dropdown field validator that destroys and recreates the field being validated.

The block has an input called 'AT', with an attached dropdown field called 'WHERE'. The 'AT' input is sometimes a value input, meaning the user has a place where they can put a numerical block representing the list index, but the 'AT' input is also sometimes a dummy input, depending on whether its 'WHERE' field has one of the values that supports specifying an index. The top two of the possible values for 'WHERE' support an index, including the default value '#' and the second value '# from end'. The other possible values do not support an index.

The 'WHERE' dropdown field has a validator callback function that, in addition to validating updates to the field's value, modifies the block itself. If the field initially had one of the values that supports an index but is being changed to one that does not, or vice versa, then the input and field being modified are both destroyed and recreated in order to change the input type. This results in the field's value being set two more times, for a total of three times that the setValue method gets called:

1. The setValue method is initially called with the value that was directly selected by the user.
2. If necessary, the validator instantiates a replacement field, and setValue is implicitly called with the default value.
3. After the replacement field is instantiated, the validator explicitly calls setValue, this time providing the value selected by the user again so that the new field will have the expected value.

The BlockChange event is sometimes fired in step 1, and sometimes in step 3. If the input and field did not need to be replaced, then the validator will allow the initial setValue call to proceed, which will fire the event. However, if the input and field needed to be replaced, then the validator will prevent the initial call from proceeding. I don't think call in step 2 ever fires a BlockChange event. However, step 3 can result in firing the event, but only if it is changing the field to a value that is different from the default value it had after step 2.

This explains why changing the value from '# from end' to '#' successfully fires an event (because nothing is being replaced and standard value changes can proceed) but changing it from 'last' to '#' does not fire an event (because the replacement field starts with the default field value '#' and trying to set it to '#' again won't be a change in value).

I suggest that we permanently detach the 'WHERE' field from the 'AT' input. We can have a separate dummy input that is inserted before the 'AT' and doesn't ever need to be replaced, and we can attach the 'WHERE' field to this input. We can simplify the validator, which no longer needs to create and update a replacement for this field, and thus does not need to interrupt the normal value change flow. Instead, the field can be created only once, in the block init function.

This code change should be applied to the block types lists_getIndex, lists_setIndex, and lists_getSublist in the file blocks/lists.ts and the block type text_getSubstring in the filed blocks/text.ts.

Each of these block type definitions will need to have these functions modified: init and updateAt_.

If the Blockly team approves of this proposal, then @abhinavjha0239 is welcome to try implementing it.

[rachel-fenichel]

@abhinavjha0239 are you still working on this?

If not, I will hand it over to @johnnesky to work on next.

[abhinavjha0239]

@rachel-fenichel yeah fine. Please hand it over to @johnnesky